Summary: | Early out of RenderLayer::updateCanBeStackingContainer more often | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Glenn Hartmann <hartmanng> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | vollick | ||||||||||||||
Status: | UNCONFIRMED --- | ||||||||||||||||
Severity: | Normal | CC: | buildbot, eric, esprehn+autocc, ojan.autocc, rniwa, simon.fraser, vollick, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 109591 | ||||||||||||||||
Attachments: |
|
Description
Glenn Hartmann
2013-02-15 13:24:59 PST
More context: If we don't scroll overflow, then the value of m_canBeStackingContainer isn't useful. It would be nice to avoid computing it if we don't have to. For this to work reliably, we need to dirty this value whenever we start scrolling overflow, and freshen it in RenderLayer::styleChanged. Created attachment 191080 [details]
Patch
Comment on attachment 191080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191080&action=review > Source/WebCore/rendering/RenderBox.cpp:360 > + if (layer()) > + layer()->dirtyAncestorStackingContextDescendantsAreContiguousInStackingOrder(); I think this should call a function on RenderLayer with a simpler name that describes when it's called, but I can't tell from this call site what actually matters. > Source/WebCore/rendering/RenderLayer.cpp:710 > + // Overflow are a box concept. is a? > Source/WebCore/rendering/RenderLayer.cpp:721 > + bool overflowXHasChanged = overflowX == OSCROLL && (!oldStyle || oldStyle->overflowX() != OSCROLL); > + bool overflowYHasChanged = overflowY == OSCROLL && (!oldStyle || oldStyle->overflowY() != OSCROLL); > + if (overflowXHasChanged || overflowYHasChanged) > + dirtyAncestorStackingContextDescendantsAreContiguousInStackingOrder(); It's odd that you're dirtying a flag related to stacking order when overflow changes. Sure, it's related to composited overflow, but you're making the assumption that only composited overflow cares about the flag. > Source/WebCore/rendering/RenderLayer.cpp:1140 > + ScopedNeedsCompositedScrollingUpdater(this); This seems like an odd place for a Scope thing. Won't it be destroyed two lines below? > Source/WebCore/rendering/RenderLayer.h:1319 > + RenderLayer* m_nextLayerToUpdateNeedsCompositedScrolling; > + RenderLayer* m_lastLayerToUpdateNeedsCompositedScrolling; It seems wrong to take up space on RenderLayer with these. Created attachment 191282 [details] Patch (In reply to comment #3) > (From update of attachment 191080 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191080&action=review > > > Source/WebCore/rendering/RenderBox.cpp:360 > > + if (layer()) > > + layer()->dirtyAncestorStackingContextDescendantsAreContiguousInStackingOrder(); > > I think this should call a function on RenderLayer with a simpler name that describes when it's called, but I can't tell from this call site what actually matters. done. > > Source/WebCore/rendering/RenderLayer.cpp:710 > > + // Overflow are a box concept. > > is a? yup, fixed. > > Source/WebCore/rendering/RenderLayer.cpp:721 > > + bool overflowXHasChanged = overflowX == OSCROLL && (!oldStyle || oldStyle->overflowX() != OSCROLL); > > + bool overflowYHasChanged = overflowY == OSCROLL && (!oldStyle || oldStyle->overflowY() != OSCROLL); > > + if (overflowXHasChanged || overflowYHasChanged) > > + dirtyAncestorStackingContextDescendantsAreContiguousInStackingOrder(); > > It's odd that you're dirtying a flag related to stacking order when overflow changes. Sure, it's related to composited overflow, but you're making the assumption that only composited overflow cares about the flag. I've abstracted this out into a new overflowDidChange() function, it should be more generalizable. > > Source/WebCore/rendering/RenderLayer.cpp:1140 > > + ScopedNeedsCompositedScrollingUpdater(this); > > This seems like an odd place for a Scope thing. Won't it be destroyed two lines below? Yes, that's intentional. This works together with the other scoped objects further up in the stack to add itself to its stacking context's m_nextLayerToUpdateNeedsCompositedScrolling list. > > Source/WebCore/rendering/RenderLayer.h:1319 > > + RenderLayer* m_nextLayerToUpdateNeedsCompositedScrolling; > > + RenderLayer* m_lastLayerToUpdateNeedsCompositedScrolling; > > It seems wrong to take up space on RenderLayer with these. I've modified this to be a LIFO list instead of FIFO, which allows us to use only one pointer instead of two. Alternatively, instead of storing a list of layers that need updating, we could store a dirty bit on each layer, and then walk the subtree from the stacking context and update any dirty layers when ScopedNeedsCompositedScrollingUpdater goes out of scope. Created attachment 196242 [details]
Patch
Created attachment 196246 [details] Patch (In reply to comment #6) > Is this all really worth the benefit? Ack, I'm sorry. I think I misunderstood you. I think you were asking if all of these optimizations were really worth the benefit. Turns out they weren't. I went ahead and implemented them, but I found that even with the naive approach, we only spend about 0.3ms in ::hasOutOfFlowPositionedDescendant while loading gmail on my desktop machine. This simplifies things a lot. This patch includes the changes for 109966. It doesn't perform correctly without the fixes in that patch. Created attachment 196247 [details]
Patch
Sorry for the noise. That last patch was attached to the wrong bug.
Comment on attachment 196247 [details] Patch Attachment 196247 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17357944 Comment on attachment 196247 [details] Patch Attachment 196247 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17323729 Created attachment 196262 [details]
Patch
Fixing mac ews bots.
|