Bug 109966 - Early out of RenderLayer::updateCanBeStackingContainer more often
Summary: Early out of RenderLayer::updateCanBeStackingContainer more often
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on:
Blocks: 109591
  Show dependency treegraph
 
Reported: 2013-02-15 13:24 PST by Glenn Hartmann
Modified: 2013-04-02 17:55 PDT (History)
8 users (show)

See Also:


Attachments
Patch (26.14 KB, patch)
2013-03-01 17:27 PST, Glenn Hartmann
no flags Details | Formatted Diff | Diff
Patch (25.78 KB, patch)
2013-03-04 12:00 PST, Glenn Hartmann
no flags Details | Formatted Diff | Diff
Patch (48.80 KB, patch)
2013-04-02 15:37 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (76.49 KB, patch)
2013-04-02 15:58 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (48.80 KB, patch)
2013-04-02 16:00 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (48.71 KB, patch)
2013-04-02 17:55 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Glenn Hartmann 2013-02-15 13:24:59 PST
We can early-out of this function more often if we maintain a dirty bit for the m_canBePromotedToStackingcontainer property.
Comment 1 vollick 2013-02-15 13:28:28 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.
Comment 2 Glenn Hartmann 2013-03-01 17:27:23 PST
Created attachment 191080 [details]
Patch
Comment 3 Simon Fraser (smfr) 2013-03-01 17:57:00 PST
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.
Comment 4 Glenn Hartmann 2013-03-04 12:00:32 PST
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.
Comment 5 vollick 2013-04-02 15:37:14 PDT
Created attachment 196242 [details]
Patch
Comment 6 vollick 2013-04-02 15:58:39 PDT
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.
Comment 7 vollick 2013-04-02 16:00:40 PDT
Created attachment 196247 [details]
Patch

Sorry for the noise. That last patch was attached to the wrong bug.
Comment 8 Build Bot 2013-04-02 16:52:35 PDT
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 9 Build Bot 2013-04-02 17:14:35 PDT
Comment on attachment 196247 [details]
Patch

Attachment 196247 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17323729
Comment 10 vollick 2013-04-02 17:55:10 PDT
Created attachment 196262 [details]
Patch

Fixing mac ews bots.