[chromium] Avoid allocating render pass textures that have no content
Created attachment 151119 [details] Patch
Created attachment 151120 [details] Patch
Comment on attachment 151120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review This feels a little over-engineered to me. There's only two types of culling here, and they both could iterate in the same way, starting from the root, yeah? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487 > // We are changing the vector in the middle of reverse iteration. > // We are guaranteed that any data from iterator to the end will not change. > // Capture the iterator position from the end, and restore it after the change. > - int positionFromEnd = passes.size() - passIndex; > - removeRenderPassesRecursive(passes, passIndex, renderPassQuad->renderPass(), skippedPasses); > - passIndex = passes.size() - positionFromEnd; > - ASSERT(passIndex >= 0); > + int positionFromEnd = frame.renderPasses.size() - it; > + removeRenderPassesRecursive(frame.renderPasses, it, renderPassQuad->renderPass(), frame.skippedPasses); > + it = frame.renderPasses.size() - positionFromEnd; > + ASSERT(it >= 0); How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0.
(In reply to comment #3) > (From update of attachment 151120 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review > > This feels a little over-engineered to me. There's only two types of culling here, and they both could iterate in the same way, starting from the root, yeah? I think no :/ In the case of culling empty lists, we want to go in dependency order, because making one empty can make its parent empty. In the cached texture case, dropping the root should drop its subtree. The relationship in the empty case is reversed. We drop the parent based on the decision to drop its child (does dropping its child make the parent empty?). > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487 > > // We are changing the vector in the middle of reverse iteration. > > // We are guaranteed that any data from iterator to the end will not change. > > // Capture the iterator position from the end, and restore it after the change. > > - int positionFromEnd = passes.size() - passIndex; > > - removeRenderPassesRecursive(passes, passIndex, renderPassQuad->renderPass(), skippedPasses); > > - passIndex = passes.size() - positionFromEnd; > > - ASSERT(passIndex >= 0); > > + int positionFromEnd = frame.renderPasses.size() - it; > > + removeRenderPassesRecursive(frame.renderPasses, it, renderPassQuad->renderPass(), frame.skippedPasses); > > + it = frame.renderPasses.size() - positionFromEnd; > > + ASSERT(it >= 0); > > How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0. Ya, hm. Lemme look at that again.
Comment on attachment 151120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487 >>> + ASSERT(it >= 0); >> >> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0. > > Ya, hm. Lemme look at that again. The code isn't looking to see if renderPass[i] is empty. Instead it looks for all renderPassQuads at index renderPass[i]. renderPass[0] cannot have any renderPassQuads since it is the first one to be drawn, so this code works okay in the forward case as well.
(In reply to comment #5) > (From update of attachment 151120 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review > > >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487 > >>> + ASSERT(it >= 0); > >> > >> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0. > > > > Ya, hm. Lemme look at that again. > > The code isn't looking to see if renderPass[i] is empty. Instead it looks for all renderPassQuads at index renderPass[i]. renderPass[0] cannot have any renderPassQuads since it is the first one to be drawn, so this code works okay in the forward case as well. More specifically, to remove renderPass[0], you have to be at position >= 1. At 1, you get: size = N positionFromEnd = size - 1 = N - 1 size = size - 1 = N - 1 it = size - positionFromEnd = N-1 - N-1 = 0
Comment on attachment 151120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review In that case, looks good, except for needing some more comments: >>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487 >>>>> + ASSERT(it >= 0); >>>> >>>> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0. >>> >>> Ya, hm. Lemme look at that again. >> >> The code isn't looking to see if renderPass[i] is empty. Instead it looks for all renderPassQuads at index renderPass[i]. renderPass[0] cannot have any renderPassQuads since it is the first one to be drawn, so this code works okay in the forward case as well. > > More specifically, to remove renderPass[0], you have to be at position >= 1. > > At 1, you get: > size = N > positionFromEnd = size - 1 = N - 1 > size = size - 1 = N - 1 > it = size - positionFromEnd = N-1 - N-1 = 0 Ok, I'll buy that. Sorry for the misunderstanding. :) I think this comment needs to be updated then, since it's not about changing something in the middle of reverse iteration, it's about removeRenderPassesRecursive only removing render passes at a lower index. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:199 > + // Iterates in draw order. Can you explain the 'why' and not just the 'what' for each of these culling functor classes?
Comment on attachment 151120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151120&action=review >>>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:487 >>>>>> + ASSERT(it >= 0); >>>>> >>>>> How does this work with the forward iteration in CullRenderPassesWithNoQuads? I think the index would be -1 if you removed pass 0. >>>> >>>> Ya, hm. Lemme look at that again. >>> >>> The code isn't looking to see if renderPass[i] is empty. Instead it looks for all renderPassQuads at index renderPass[i]. renderPass[0] cannot have any renderPassQuads since it is the first one to be drawn, so this code works okay in the forward case as well. >> >> More specifically, to remove renderPass[0], you have to be at position >= 1. >> >> At 1, you get: >> size = N >> positionFromEnd = size - 1 = N - 1 >> size = size - 1 = N - 1 >> it = size - positionFromEnd = N-1 - N-1 = 0 > > Ok, I'll buy that. Sorry for the misunderstanding. :) > > I think this comment needs to be updated then, since it's not about changing something in the middle of reverse iteration, it's about removeRenderPassesRecursive only removing render passes at a lower index. Yep done. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:199 >> + // Iterates in draw order. > > Can you explain the 'why' and not just the 'what' for each of these culling functor classes? yep!
Created attachment 151337 [details] Patch
Comment on attachment 151337 [details] Patch Attachment 151337 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13170288
Created attachment 151345 [details] Patch Hm, linked okay locally :/ Added defines for the each version of the template function to the cpp to try linking again on the bots.
Comment on attachment 151345 [details] Patch R=me.
Comment on attachment 151345 [details] Patch Clearing flags on attachment: 151345 Committed r122252: <http://trac.webkit.org/changeset/122252>
All reviewed patches have been landed. Closing bug.