Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399733002:
DrawingBuffer cleanup: Part 1 of many (Closed)
|
Created:
4 years, 2 months ago by ccameron Modified:
4 years, 2 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, Justin Novosad, Rik, f(malita), blink-reviews, piman+watch_chromium.org, danakj+watch_chromium.org, ajuma+watch_chromium.org, Stephen Chennney, rwlbuis, erikchen Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDrawingBuffer cleanup: Part 1 of many
DrawingBuffer is actually a buffer queue that recycles buffers.
This buffer queue shrinks the recycle queue when it is asked to
pull an element from the recycle queue. It makes more sense to
do this when the elements are put in the recycle queue.
The buffer queue also has a function to resize a recycled
buffer, instead of just deleting it and creating a new one (perhaps
under the idea the creating and destroying GL texture names
is expensive). Remove this functionality because it just complicates
things.
While we're in the neighborhood, inline the resize functions for
the multisample and depth and stencil functions (there's no point
to having a short helper function that is called in only one place,
it gives a feeling of more generality than actually exists).
BUG=648707
Committed: https://crrev.com/0ba2258a4d613f70ab966f52249209d6156136cd
Cr-Commit-Position: refs/heads/master@{#423631}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix unittests #
Total comments: 2
Patch Set 3 : Fix size update #
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== DrawingBuffer cleanup: Part 1 of many BUG= ========== to ========== DrawingBuffer cleanup: Part 1 of many BUG=648707 ==========
Description was changed from ========== DrawingBuffer cleanup: Part 1 of many BUG=648707 ========== to ========== DrawingBuffer cleanup: Part 1 of many DrawingBuffer is actually a buffer queue that recycles buffers. This buffer queue shrinks the recycle queue when it is asked to pull an element from the recycle queue. It makes more sense to do this when the elements are put in the recycle queue. The buffer queue also has a function to resize a recycled buffer, instead of just deleting it and creating a new one (perhaps under the idea the creating and destroying GL texture names is expensive). Remove this functionality because it just complicates things. BUG=648707 ==========
ccameron@chromium.org changed reviewers: + pdr@google.com
Description was changed from ========== DrawingBuffer cleanup: Part 1 of many DrawingBuffer is actually a buffer queue that recycles buffers. This buffer queue shrinks the recycle queue when it is asked to pull an element from the recycle queue. It makes more sense to do this when the elements are put in the recycle queue. The buffer queue also has a function to resize a recycled buffer, instead of just deleting it and creating a new one (perhaps under the idea the creating and destroying GL texture names is expensive). Remove this functionality because it just complicates things. BUG=648707 ========== to ========== DrawingBuffer cleanup: Part 1 of many DrawingBuffer is actually a buffer queue that recycles buffers. This buffer queue shrinks the recycle queue when it is asked to pull an element from the recycle queue. It makes more sense to do this when the elements are put in the recycle queue. The buffer queue also has a function to resize a recycled buffer, instead of just deleting it and creating a new one (perhaps under the idea the creating and destroying GL texture names is expensive). Remove this functionality because it just complicates things. While we're in the neighborhood, inline the resize functions for the multisample and depth and stencil functions (there's no point to having a short helper function that is called in only one place, it gives a feeling of more generality than actually exists). BUG=648707 ==========
I gave up on trying to make big changes to DrawingBuffer, and instead I'm going to just make a sequence of several smaller changes. This is a small move-things-around and delete-functionality change that that will make the diffs in future patches easier to manage.
ccameron@chromium.org changed reviewers: + kbr@chromium.org - pdr@google.com
Switching reviewer to kbr@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for working on cleaning this up. This looks good overall. It should have the same steady-state behavior as before, i.e., continuing to use recycled mailboxes and avoiding new IOSurface and texture allocations, correct? Has this been verified with performance tests? Are there any unit tests that assert that recycled mailboxes are retained? CC'ing erikchen@ for any comments. https://codereview.chromium.org/2399733002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2399733002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:597: DCHECK(mailboxInfo->size == m_size); Am I understanding correctly that this invariant is now maintained by: - Discarding recycled mailboxes in gpuMailboxReleased if it has a different size - Throwing out all of the recycled mailboxes in DrawingBuffer::reset ? Just trying to understand the situation.
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Yes, there are some unit tests. I'd missed them earlier -- I had to update them because the time of destruction is shuffled around a bit (and the times move a bit in future patches, too). They do indeed verify that recycling behavior is unchanged (also found out because I broke a test). https://codereview.chromium.org/2399733002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2399733002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:597: DCHECK(mailboxInfo->size == m_size); On 2016/10/06 03:47:52, Ken Russell wrote: > Am I understanding correctly that this invariant is now maintained by: > > - Discarding recycled mailboxes in gpuMailboxReleased if it has a different > size > - Throwing out all of the recycled mailboxes in DrawingBuffer::reset > > ? Just trying to understand the situation. Yes, exactly.
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/2399733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2399733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:936: m_size = adjustedSize; if adjustedSize is empty, then recycled mailboxes won't be freed, despite being a different size from m_size.
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Thanks -- updated. https://codereview.chromium.org/2399733002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2399733002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:936: m_size = adjustedSize; On 2016/10/06 17:08:25, erikchen wrote: > if adjustedSize is empty, then recycled mailboxes won't be freed, despite being > a different size from m_size. Oops! Moved the "free" to be right next to the m_size = line. It'll potentially get called multiple times, but it makes the dependency clear.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Good catch Erik. LGTM too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks!
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DrawingBuffer cleanup: Part 1 of many DrawingBuffer is actually a buffer queue that recycles buffers. This buffer queue shrinks the recycle queue when it is asked to pull an element from the recycle queue. It makes more sense to do this when the elements are put in the recycle queue. The buffer queue also has a function to resize a recycled buffer, instead of just deleting it and creating a new one (perhaps under the idea the creating and destroying GL texture names is expensive). Remove this functionality because it just complicates things. While we're in the neighborhood, inline the resize functions for the multisample and depth and stencil functions (there's no point to having a short helper function that is called in only one place, it gives a feeling of more generality than actually exists). BUG=648707 ========== to ========== DrawingBuffer cleanup: Part 1 of many DrawingBuffer is actually a buffer queue that recycles buffers. This buffer queue shrinks the recycle queue when it is asked to pull an element from the recycle queue. It makes more sense to do this when the elements are put in the recycle queue. The buffer queue also has a function to resize a recycled buffer, instead of just deleting it and creating a new one (perhaps under the idea the creating and destroying GL texture names is expensive). Remove this functionality because it just complicates things. While we're in the neighborhood, inline the resize functions for the multisample and depth and stencil functions (there's no point to having a short helper function that is called in only one place, it gives a feeling of more generality than actually exists). BUG=648707 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== DrawingBuffer cleanup: Part 1 of many DrawingBuffer is actually a buffer queue that recycles buffers. This buffer queue shrinks the recycle queue when it is asked to pull an element from the recycle queue. It makes more sense to do this when the elements are put in the recycle queue. The buffer queue also has a function to resize a recycled buffer, instead of just deleting it and creating a new one (perhaps under the idea the creating and destroying GL texture names is expensive). Remove this functionality because it just complicates things. While we're in the neighborhood, inline the resize functions for the multisample and depth and stencil functions (there's no point to having a short helper function that is called in only one place, it gives a feeling of more generality than actually exists). BUG=648707 ========== to ========== DrawingBuffer cleanup: Part 1 of many DrawingBuffer is actually a buffer queue that recycles buffers. This buffer queue shrinks the recycle queue when it is asked to pull an element from the recycle queue. It makes more sense to do this when the elements are put in the recycle queue. The buffer queue also has a function to resize a recycled buffer, instead of just deleting it and creating a new one (perhaps under the idea the creating and destroying GL texture names is expensive). Remove this functionality because it just complicates things. While we're in the neighborhood, inline the resize functions for the multisample and depth and stencil functions (there's no point to having a short helper function that is called in only one place, it gives a feeling of more generality than actually exists). BUG=648707 Committed: https://crrev.com/0ba2258a4d613f70ab966f52249209d6156136cd Cr-Commit-Position: refs/heads/master@{#423631} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0ba2258a4d613f70ab966f52249209d6156136cd Cr-Commit-Position: refs/heads/master@{#423631} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
