Bug 109966 – Early out of RenderLayer::updateCanBeStackingContainer more often

• Status: UNCONFIRMED
• Component: WebCore Misc.

[Glenn Hartmann]

We can early-out of this function more often if we maintain a dirty bit for the m_canBePromotedToStackingcontainer property.

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.

[Glenn Hartmann]

Created attachment 191080 [details]
Patch

[Simon Fraser (smfr)]

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.

[Glenn Hartmann]

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.

[Build Bot]

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

[Build Bot]

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.