Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
206283003:
Avoid tree walks when computing RenderLayer::scrollParent (Closed)
|
Created:
6 years, 9 months ago by Ian Vollick Modified:
6 years, 9 months ago Reviewers:
abarth-chromium CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, abarth-chromium, blink-layers+watch_chromium.org, jchaffraix+rendering, pdr., hartmanng Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAvoid tree walks when computing RenderLayer::scrollParent
This CL uses some memoization to avoid unnecessary tree walking when
computing scroll parent. Why memoization and not just passing down the
values? Because sad. It turns out that we recur on the stacking tree (we
have to - squashing demands processing stuff in paint order), but we
walk up the render tree asking questions. Different tree topologies; we
could very easily visit a RenderLayer before its great great
grandparent.
No functionality change -- covered by existing tests.
BUG=None
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169741
Patch Set 1 : . #Patch Set 2 : . #
Total comments: 9
Patch Set 3 : . #
Total comments: 4
Patch Set 4 : . #
Total comments: 6
Patch Set 5 : . #Patch Set 6 : Fix build errors. #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:1138: ASSERT(isInCompositingUpdate()); Changed the assert to enforce access only during comp-update. https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:1144: return m_ancestorDependentPropertyCache->ancestorCompositedScrollingLayer; The old code made no use of the containing block. Use it here to avoid recomputing the CB when the cache is fresh. https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:1682: ASSERT(isInCompositingUpdate()); Again, enforce that we access scrollParent only during comp-update.
https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:1150: const_cast<RenderLayer*>(this)->m_ancestorDependentPropertyCache = adoptPtr(new AncestorDependentPropertyCache()); The pattern we usually use for this is a function called ensureAncestorDependentPropertyCache() That does this work. You can mark m_ancestorDependentPropertyCache mutable to remove the const_cast. Then you can put the ASSERT(isInCompositingUpdate()) inside that function. https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:1705: return m_ancestorDependentPropertyCache->scrollParent; Isn't it possible for m_ancestorDependentPropertyCache to be created without scrollParent being populated? It seems like we need to check something more here. https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:3547: return true; Doesn't this mean these functions could be called while assert disablers are on the stack? In that case, the cache could be stale, right?
Thanks for the quick reviews! Other than the changes I've made in response to your comments, I've also moved the clearing of the cache to be driven by RenderLayerCompositor as an explicit phase after compositing updates. https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:1150: const_cast<RenderLayer*>(this)->m_ancestorDependentPropertyCache = adoptPtr(new AncestorDependentPropertyCache()); On 2014/03/20 20:04:54, abarth wrote: > The pattern we usually use for this is a function called > > ensureAncestorDependentPropertyCache() > > That does this work. You can mark m_ancestorDependentPropertyCache mutable to > remove the const_cast. Then you can put the ASSERT(isInCompositingUpdate()) > inside that function. Done. https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:1705: return m_ancestorDependentPropertyCache->scrollParent; On 2014/03/20 20:04:54, abarth wrote: > Isn't it possible for m_ancestorDependentPropertyCache to be created without > scrollParent being populated? It seems like we need to check something more > here. It is possible, yes. If we've computed a valid containingBlock, the scrollParent should also be valid. I've added this check. https://codereview.chromium.org/206283003/diff/80001/Source/core/rendering/Re... Source/core/rendering/RenderLayer.cpp:3547: return true; On 2014/03/20 20:04:54, abarth wrote: > Doesn't this mean these functions could be called while assert disablers are on > the stack? In that case, the cache could be stale, right? Removed the ability to disable this. Since scrollParent() is accessed in the wrong phase due to chicken and egg, I've also updated the code so as not to memoize unless we're in the correct phase. I've also added ASSERTs to ensure that if we even have a non-NULL cache, that we're in the comp-update phase.
https://codereview.chromium.org/206283003/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/206283003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1145: m_ancestorDependentPropertyCache = adoptPtr(new AncestorDependentPropertyCache()); Should we just return if the cache already exists? In this version, we'll blow it away if it already exists. https://codereview.chromium.org/206283003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1721: if (m_ancestorDependentPropertyCache && m_ancestorDependentPropertyCache->containingBlock) Why is checking for containingBlock correct here? containingBlock could be populated in RenderLayer::ancestorCompositedScrollingLayer, which doesn't set scrollParent...
https://codereview.chromium.org/206283003/diff/100001/Source/core/rendering/R... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/206283003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1145: m_ancestorDependentPropertyCache = adoptPtr(new AncestorDependentPropertyCache()); On 2014/03/20 21:36:45, abarth wrote: > Should we just return if the cache already exists? In this version, we'll blow > it away if it already exists. Done. https://codereview.chromium.org/206283003/diff/100001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1721: if (m_ancestorDependentPropertyCache && m_ancestorDependentPropertyCache->containingBlock) On 2014/03/20 21:36:45, abarth wrote: > Why is checking for containingBlock correct here? containingBlock could be > populated in RenderLayer::ancestorCompositedScrollingLayer, which doesn't set > scrollParent... Oof. It isn't! It was only correct if you call scrollParent before ancestorCompositedScrollLayer (if you did that, scrollParent would invoke ancestorCompositedScrollLayer, indirectly creating the cache, and the rest of the scrollParent function would ensure that the cached scrollParent was corret). If you called ancestorCompositedScrollLayer first, then this is busted. I was using containingBlock essentially as a dirty bit, but this is cryptic and misleading. I've switched this to two, clearly named dirty bits. Hopefully this time the dirty bit checking is correct. Thanks for catching this.
lgtm https://codereview.chromium.org/206283003/diff/120001/Source/core/rendering/R... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/206283003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1177: m_ancestorDependentPropertyCache->ancestorCompositedScrollingLayerDirty = false; I probably would have made this a setter that encapsulates this work. https://codereview.chromium.org/206283003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1739: if (ancestorCache->ancestorCompositedScrollingLayer == scrollParent) { Don't we need to check the dirty bit here? https://codereview.chromium.org/206283003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1748: m_ancestorDependentPropertyCache->scrollParentDirty = false; ditto
https://codereview.chromium.org/206283003/diff/120001/Source/core/rendering/R... File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/206283003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1177: m_ancestorDependentPropertyCache->ancestorCompositedScrollingLayerDirty = false; On 2014/03/20 22:49:15, abarth wrote: > I probably would have made this a setter that encapsulates this work. Done. https://codereview.chromium.org/206283003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1739: if (ancestorCache->ancestorCompositedScrollingLayer == scrollParent) { On 2014/03/20 22:49:15, abarth wrote: > Don't we need to check the dirty bit here? Fixed. While I was adding the setters in response to your other comments, I also added some getters with ASSERTs to ensure we don't try and grab dirty data. https://codereview.chromium.org/206283003/diff/120001/Source/core/rendering/R... Source/core/rendering/RenderLayer.cpp:1748: m_ancestorDependentPropertyCache->scrollParentDirty = false; On 2014/03/20 22:49:15, abarth wrote: > ditto Done.
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/206283003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/206283003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on mac_blink_compile_dbg
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/206283003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by vollick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vollick@chromium.org/206283003/180001
Message was sent while issue was closed.
Change committed as 169741 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
