1205473 - nsDisplayLayerEventRegions::AddFrames takes a lot of time

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Most of the time here is spent within nsLayoutUtils::HasNonZeroCorner, even though very few frames actually have rounded corners.

A simple solution is to cache a bit on the frame (as a state bit or somewhere else) that records when we can take a simplified path (that uses the common choice for all decisions based on style data) through this function and invalidate it when we get a style change. That removes about 2/3 of the time spent in this function.

Even better would be to know when we've been reflowed/moved and/or our parents have changed, in which case we could start avoiding the border box and clipping calculations too.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Matt, can you describe a bit more what needs to be done here? I'm happy to do the work but don't really know much about the style structs and such. I can add a style bit but I'm not sure how to detect when the style changes to invalidate the bit.

Fixing this would help significantly on the TART test (and possibly others) with APZ enabled. Most of the time is spent in HasNonZeroCorner so even caching just that result would be fine.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Simple optimization that doesn't really help

Even this helps a bit on TART, taking the result down from 8.356388508072508 to 7.88058395895081 on my OS X machine (~5.69% improvement). For reference, with APZ disabled the result is 7.3380653620997975 so this fixes half of the regression. Presumably the full version that you described would help even more.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8727676 [details] [diff] [review]
Simple optimization that doesn't really help

Review of attachment 8727676 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +3447,5 @@
>    borderBox += aBuilder->ToReferenceFrame(aFrame);
>  
>    const DisplayItemClip* clip = aBuilder->ClipState().GetCurrentCombinedClip(aBuilder);
> +  bool borderBoxHasRoundedCorners = false;
> +  if (aFrame->StyleBorder()->mBorderRadius != nsStyleCorners()) {

Could just move this check into the start of HasNonZeroCorner and get the win for all callers.

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> Matt, can you describe a bit more what needs to be done here? I'm happy to
> do the work but don't really know much about the style structs and such. I
> can add a style bit but I'm not sure how to detect when the style changes to
> invalidate the bit.
> 
> Fixing this would help significantly on the TART test (and possibly others)
> with APZ enabled. Most of the time is spent in HasNonZeroCorner so even
> caching just that result would be fine.

The simplest thing to do would be to add a new frame state bit, something like NS_FRAME_SIMPLE_EVENT regions and set it whenever we take the most common path (You'd have to confirm what that is) through the checks that only rely on style data (nothing that uses reflow data).

From then on, whenever that bit is set, we can avoid all the style checks and know what the result is.

We'd need to add code to nsFrame::DidSetStyleContext (called any time styles are changed for the frame) to remove the flag.

The other option for invalidation is figure out exactly which set of style changes are relevant, add a new change hint (nsChangeHint_***), set it in the appropriate CalcDifference functions, and then add a handler for it in the RestyleManager. nsChangeHint_UpdateTransformLayer would be a good guide here.

The latter is much more accurate and will stop us invalidating the cache for irrelevant style changes, but I suspect it's overkill here.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Hm, the patch I posted doesn't seem to help the talos scores much on try [1]. I'll try what you suggested in comment 4.

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a4929411c0aa&newProject=try&newRevision=35ef70a10445&framework=1&showOnlyImportant=0

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Ignore comment 5, I stoopidly ran non-e10s talos jobs where this has no impact. I'll trigger the e10s jobs and see what happens. I also did the same mistake on the updated version at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba71ccf516d0

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Ok, so my "simple optimization" still doesn't seem to impact anything for e10s [1]. The updated version with the bitflag does a little better [2] with a bunch of tests getting small reductions although even there perfherder doesn't actually think any of those are significant. I'm a little skeptical of perfherder's notion of importance (filed bug 1255001) but any win is good so I'll put the patch up.

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a4929411c0aa&newProject=try&newRevision=35ef70a10445&framework=1&filter=e10s&showOnlyImportant=0
[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a4929411c0aa&newProject=try&newRevision=ba71ccf516d0&framework=1&filter=e10s&showOnlyImportant=0

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Add a state bit for "simple" event regions

There's really only two style checks that we can put behind this flag. The GetEffectivePointerEvents one looks at other frames in some cases so unless I import some of that logic I don't think we can do it. If there's other things we can guard with this flag that I'm missing let me know.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b42c22e14ebc

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b42c22e14ebc

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I don't think this provided enough benefit to warrant uplifting. If we really need the bit for something else we could consider backing this out as well, but I'll leave it for now.