Consider adding StyleDisplay() values to nsIFrame

NEW
Assigned to

Status

()

Core
Layout
10 months ago
a month ago

People

(Reporter: jet, Assigned: jet, NeedInfo)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 affected)

Details

(Whiteboard: [qf:p3])

(Assignee)

Description

10 months ago
StyleDisplay() gets called a lot in Gecko profiles, which results in a lot of pointer-chasing to get to the relevant structs. This bug is about short-circuiting that and keeping the relevant values on the nsiFrame
(Assignee)

Updated

10 months ago
Whiteboard: [qf:?]
Another approach that may make sense here is not to store the actual nsStyleDisplay* on the frame, but instead to add another 64 bits of booleans to nsIFrame (probably as bitfield member variables), and then use a few of them to store the things from nsStyleDisplay that we need (in nsFrame::DidSetStyleContext, perhaps), such as:
 * the result of IsThemed (for a ton of functions)
 * the result of the first test in nsIFrame::Extend3DContext, so that test can be replaced with a bit test
 * the same for the entirety of nsIFrame::BackfaceIsHidden
See Also: → bug 1352056
Whiteboard: [qf:?] → [qf]

Updated

10 months ago
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 2

9 months ago
(In reply to David Baron :dbaron: ⌚️UTC+8 from comment #1)
> Another approach that may make sense here is not to store the actual
> nsStyleDisplay* on the frame, but instead to add another 64 bits of booleans
> to nsIFrame (probably as bitfield member variables), and then use a few of
> them to store the things from nsStyleDisplay that we need (in
> nsFrame::DidSetStyleContext, perhaps), such as:
>  * the result of IsThemed (for a ton of functions)
>  * the result of the first test in nsIFrame::Extend3DContext, so that test
> can be replaced with a bit test
>  * the same for the entirety of nsIFrame::BackfaceIsHidden

It looks like bug 1352056 fixes this for IsTransformed & BackfaceIsHidden but not for IsThemed. (i.e., we've fixed 2 out of the 3 issues listed here.)

Neerja: can you file a new bug for a similar fix for IsThemed?
Flags: needinfo?(npancholi)
Assignee: nobody → npancholi
Flags: needinfo?(npancholi)
Depends on: 1352056
Depends on: 1361207
(In reply to Jet Villegas (:jet) from comment #2)
> It looks like bug 1352056 fixes this for IsTransformed & BackfaceIsHidden
> but not for IsThemed. (i.e., we've fixed 2 out of the 3 issues listed here.)

Technically, bug 1352056 & similar bug 1361207 (which Neerja filed) don't do any of the things that were originally suggested here in comment 0 & comment 1 -- i.e. neither bug caches StyleDisplay info on the frame. Those bugs' patches are just about coalescing redundant calls to StyleDisplay *within a given function & its helpers* into a single helper local-variable, and adjusting some helper-functions to allow optionally passing in that helper-variable.

Maybe that's enough, though? I guess we'll see in profiles going forward.
Whiteboard: [qf:p1] → [qf:p1][maybe largely fixed by bug 1352056 & bug 1361207?]
Jet, do you recall which profiles/sites in particular were showing significant time spent in StyleDisplay(), in the profiles that spawned this bug?

We should probably reprofile some of the sites, to see if the numbers have changed appreciably as a result of the helper bugs here.  That would help determine if there's anything left to do here or not [i.e. whether it's still worth doing what we originally planned in comment 0].
Flags: needinfo?(bugs)
(Assignee)

Comment 5

8 months ago
Amazon (e.g., bug 1357621) shows a lot of StyleDisplay() in profile stacks. Jonathan may have new info re: perf effects from the bugs fixed here.
Flags: needinfo?(bugs) → needinfo?(jfkthame)
I'm not sure this makes a huge difference for Amazon in bug 1357621.

I'm profiling on the Acer hardware on Windows using both Firefox 53.0 release (which doesn't have our dependent bugs' fixes) and in current Nightly (which does).   I'm not seeing ::StyleDisplay() show up appreciably in the profile at all.  Especially if I limit the time range to the period of time where checkLineFit is spiking -- that's the JS function we're concerned with over there -- I see less than 2ms spent in ::StyleDisplay
Here are two profiles from loading bug 1357621's Amazon page, and filtering for ::StyleDisplay.

Nightly 55:
===========
https://perf-html.io/public/38c78d139cdd59b4a35582e7210d05fc7ef56bb5/calltree/?search=%3A%3Astyledisplay&thread=3
* Total time spent in ::StyleDisplay is 7.2ms, over the full 14 second profile.
* If I filter for checkLineFit and narrow to just that time range, then I only see 1.8ms of ::StyleDisplay

Firefox 53.0:
=============
https://perf-html.io/public/9eb015f1a5254bc68c99b85e3d74fcabb99c212f/calltree/?search=%3A%3Astyledisplay&thread=2
* Total time spent in ::StyleDisplay is 3.0ms, over the full 20 second profile.
* If I filter for checkLineFit and narrow to just that time range, then I only see 0.6ms of ::StyleDisplay

So, StyleDisplay() time seems to be negligible at bug 1357621's Amazon link, in both Firefox release & Nightly, on my Acer hardware.


(Looking at ehsan's profiles from that bug and filtering for ::StyleDisplay, I see numbers that are a bit larger -- 23ms and 18ms, respectively, after zooming in on the region that contains the three checkLineFit spikes. I'm curious why his profiles are showing somewhat higher StyleDisplay times than mine are. Still: even his larger value isn't an especially large fraction of the reflow time -- it's on the order of 5% of the time we're spending in Amazon's checkLineFit function, and it's not clear to me that there's much of a win to be gotten by squishing that 5%.)
Summary: Consider adding StyleDisplay() values to nsiFrame → Consider adding StyleDisplay() values to nsIFrame
(In reply to Daniel Holbert [:dholbert] (AFK May 3-8, 13-21) from comment #7)
> So, StyleDisplay() time seems to be negligible at bug 1357621's Amazon link,
> in both Firefox release & Nightly, on my Acer hardware.
> 
> 
> (Looking at ehsan's profiles from that bug and filtering for ::StyleDisplay,
> I see numbers that are a bit larger -- 23ms and 18ms, respectively, after
> zooming in on the region that contains the three checkLineFit spikes. I'm
> curious why his profiles are showing somewhat higher StyleDisplay times than
> mine are. Still: even his larger value isn't an especially large fraction of
> the reflow time -- it's on the order of 5% of the time we're spending in
> Amazon's checkLineFit function, and it's not clear to me that there's much
> of a win to be gotten by squishing that 5%.)

Profiling my local opt build (on macOS), I see figures comparable to Ehsan's, with around ~20ms under DoGetStyleDisplay (though it varies quite a bit between runs; e.g. https://perfht.ml/2pzhKbc has 17ms). I doubt it's worth the memory cost of adding a field to nsIFrame just to squeeze this.

We can, I think, go a little further with the approach of bug 1352056 & bug 1361207, passing the nsStyleDisplay pointer down into lower-level methods when it is already in hand rather than re-fetching it. In particular, I'm seeing DoGetStyleDisplay used in methods called during ReflowInput, even though the ReflowInput itself has already cached the value in its mStyleDisplay field (so it could pass it). The other relatively heavy user seems to be FinishAndStoreOverflow, called at the end of reflow; when calling this, too, we usually have a ReflowInput struct on hand so we could pass its mStyleDisplay.

Here's a profile with local patches that address these areas: https://perfht.ml/2qkAcBC. This does seem to show somewhat less DoGetStyleDisplay time (9ms), so perhaps it's worth doing; I'll clean up the patches a bit and post to a separate dependent bug. But any wins here are really quite marginal (and increasingly so as we chase these pointers into ever more obscure corners...)

Something else to consider, perhaps as a followup, would be to make the optional (default-null) nsStyleDisplay* param that's being added to low-level methods in bug 1352056 & bug 1361207 non-optional and non-null, requiring all callers to pass the real pointer. This would involve more churn across the codebase, obviously, as we'd then have to update every call site instead of only touching specifically-identified hotspots, but the benefit is that the methods would no longer need to call StyleDisplayWithOptionalParam(), which adds a null-check and branch, they'd simply be able to use the given pointer unconditionally.
Flags: needinfo?(jfkthame)
Depends on: 1365356
(Assignee)

Updated

8 months ago
Assignee: npancholi → bugs
Flags: needinfo?(dbaron)

Updated

8 months ago
Whiteboard: [qf:p1][maybe largely fixed by bug 1352056 & bug 1361207?] → [qf:p2][maybe largely fixed by bug 1352056 & bug 1361207?]
Now that the dependent bugs are resolved can you please reprofile? If it is no longer a significant problem we can make it p3.
Flags: needinfo?(jfkthame)
Whiteboard: [qf:p2][maybe largely fixed by bug 1352056 & bug 1361207?] → [qf:p1][maybe largely fixed by bug 1352056 & bug 1361207?]
I tried re-profiling the original Amazon testcase, and I'm seeing a negligible amount of DoGetStyleDisplay() remaining in the profile. Unless we see examples of other profiles where this still seems to be an issue, I don't think there's much more to do here.
Flags: needinfo?(jfkthame)

Updated

a month ago
Whiteboard: [qf:p1][maybe largely fixed by bug 1352056 & bug 1361207?] → [qf:p3]
You need to log in before you can comment on or make changes to this bug.