Change Deque<RecycledBitmap> to Vector in DrawingBuffer

Change Deque<RecycledBitmap> to Vector in DrawingBuffer

Right now in DrawingBuffer class, we keep a Deque of RecycledBitmap to
keep a set of shared memory bitmaps that were released by compositor and
can be reused. However, there is a for loop in createOrRecycleBitmap()
that is not safe at this moment.

This CL changes the Deque structure to Vector and rewrite the for loop
to use size_t as index instead of using iterator.

BUG=647896
Committed: https://crrev.com/09b9264503a9ce8f36005d2a3136cd922ea397b4
Cr-Commit-Position: refs/heads/master@{#420088}

[xidachen, 2016-09-20 15:35:07]

xidachen@chromium.org changed reviewers:
+ danakj@chromium.org, kbr@chromium.org

[xidachen, 2016-09-20 15:35:08]

PTAL

[xidachen, 2016-09-20 15:39:14]

Description was changed from

==========
Change Deque<RecycledBitmap> to Vector

Right now in DrawingBuffer class, we keep a Deque of RecycledBitmap to
keep a set of shared memory bitmaps that were released by compositor and
can be reused. However, there is a for loop in createOrRecycleBitmap()
that is not safe at this moment.

This CL changes the Deque structure to Vector and rewrite the for loop
to use size_t as index instead of using iterator.

BUG=647896
==========

to

==========
Change Deque<RecycledBitmap> to Vector in DrawingBuffer

Right now in DrawingBuffer class, we keep a Deque of RecycledBitmap to
keep a set of shared memory bitmaps that were released by compositor and
can be reused. However, there is a for loop in createOrRecycleBitmap()
that is not safe at this moment.

This CL changes the Deque structure to Vector and rewrite the for loop
to use size_t as index instead of using iterator.

BUG=647896
==========

[Ken Russell (switch to Gerrit), 2016-09-20 22:55:37]

LGTM though I'd like danakj@ to take a look if possible.

https://codereview.chromium.org/2356803002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2356803002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:258:
m_recycledBitmap.removeLast();
Is there any more efficient way of implementing this?

Is it worth adding a takeLast() operation to wtf/Vector.h?

[xidachen, 2016-09-21 00:08:42]

xidachen@chromium.org changed reviewers:
+ junov@chromium.org

[xidachen, 2016-09-21 00:08:42]

adding junov@ who was investigating a independent bug that happens to be caused
by the same problem.

junov@: in the bug, danakj@ mentioned to change the Deque to Vector. WDYT?

[Justin Novosad, 2016-09-21 14:11:32]

This change is good, but you are missing a test. In fact, I was looking at
DrawingBufferTest.cpp and I found that it is generally lacking unit test
coverage for mailbox usage in software compositing mode. I have a good WIP CL
with a test. Why don't you go ahead and land this, and I will land my test on
top of it.

lgtm

[xidachen, 2016-09-21 14:13:30]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 14:13:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xidachen, 2016-09-21 14:18:15]

https://codereview.chromium.org/2356803002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2356803002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:258:
m_recycledBitmap.removeLast();
On 2016/09/20 22:55:37, Ken Russell wrote:
> Is there any more efficient way of implementing this?
> 
> Is it worth adding a takeLast() operation to wtf/Vector.h?

When I looked at Deque::takeLast(), it includes the same steps as here:
T oldLast = std::move(last());
removeLast();
return oldLast;

[xidachen, 2016-09-21 15:43:25]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-09-21 15:43:39]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-09-21 15:44:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 17:20:23]

Description was changed from

==========
Change Deque<RecycledBitmap> to Vector in DrawingBuffer

Right now in DrawingBuffer class, we keep a Deque of RecycledBitmap to
keep a set of shared memory bitmaps that were released by compositor and
can be reused. However, there is a for loop in createOrRecycleBitmap()
that is not safe at this moment.

This CL changes the Deque structure to Vector and rewrite the for loop
to use size_t as index instead of using iterator.

BUG=647896
==========

to

==========
Change Deque<RecycledBitmap> to Vector in DrawingBuffer

Right now in DrawingBuffer class, we keep a Deque of RecycledBitmap to
keep a set of shared memory bitmaps that were released by compositor and
can be reused. However, there is a for loop in createOrRecycleBitmap()
that is not safe at this moment.

This CL changes the Deque structure to Vector and rewrite the for loop
to use size_t as index instead of using iterator.

BUG=647896
==========

[commit-bot: I haz the power, 2016-09-21 17:20:25]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-21 17:23:13]

Description was changed from

==========
Change Deque<RecycledBitmap> to Vector in DrawingBuffer

Right now in DrawingBuffer class, we keep a Deque of RecycledBitmap to
keep a set of shared memory bitmaps that were released by compositor and
can be reused. However, there is a for loop in createOrRecycleBitmap()
that is not safe at this moment.

This CL changes the Deque structure to Vector and rewrite the for loop
to use size_t as index instead of using iterator.

BUG=647896
==========

to

==========
Change Deque<RecycledBitmap> to Vector in DrawingBuffer

Right now in DrawingBuffer class, we keep a Deque of RecycledBitmap to
keep a set of shared memory bitmaps that were released by compositor and
can be reused. However, there is a for loop in createOrRecycleBitmap()
that is not safe at this moment.

This CL changes the Deque structure to Vector and rewrite the for loop
to use size_t as index instead of using iterator.

BUG=647896

Committed: https://crrev.com/09b9264503a9ce8f36005d2a3136cd922ea397b4
Cr-Commit-Position: refs/heads/master@{#420088}
==========

[commit-bot: I haz the power, 2016-09-21 17:23:14]

Patchset 1 (id:??) landed as
https://crrev.com/09b9264503a9ce8f36005d2a3136cd922ea397b4
Cr-Commit-Position: refs/heads/master@{#420088}

[danakj, 2016-09-21 22:47:55]

LGTM thanks for figuring out the reason for the crash

[esprehn, 2016-09-28 23:41:05]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-09-28 23:41:06]

https://codereview.chromium.org/2356803002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2356803002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:252:
m_recycledBitmap.remove(i); // Removed this position so iterate on it again.
This is causing a bunch of vector resizing and copying. I think you want to
create a new Vector on the stack, copy into it all the values which do match
m_size, and then swap() with the m_recycledBitmap member. That's the more
idiomatic way to do this in blink.

[xidachen, 2016-09-29 00:08:49]

On 2016/09/28 23:41:06, esprehn wrote:
>
https://codereview.chromium.org/2356803002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
(right):
> 
>
https://codereview.chromium.org/2356803002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:252:
> m_recycledBitmap.remove(i); // Removed this position so iterate on it again.
> This is causing a bunch of vector resizing and copying. I think you want to
> create a new Vector on the stack, copy into it all the values which do match
> m_size, and then swap() with the m_recycledBitmap member. That's the more
> idiomatic way to do this in blink.

Thanks for pointing it out, I will take deeper look.