1385437 - DisplayList building time on GMail is quite high

Status: RESOLVED INVALID

(Core :: Layout, defect, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Spinning this off from bug 1381385.

Here's a set of samples from a profile: https://perfht.ml/2w7Bbqg

We seem to consistently spend around 10 - 11ms building DisplayLists for GMail.

A more zoomed out snapshot: https://perfht.ml/2w7HQ3E

Bug 1352499 will probably help with these, but not for the first DisplayList that builds. We should try to figure out why it takes so long.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Is this a good example for retailed display lists?

Comment 2

[Bas Schouten (:bas.schouten)]

I don't think there's a 'why this takes so long' here? These are not 'exceptional' numbers for displaylist building. If you enable layers.acceleration.draw-fps, you'll notice the youtube front page (on my machine) at a 1920x1080 window takes about 15ms. Hotmail inbox about 7ms, NYTimes 6ms, etc. Some pages like CNN happen to be somewhat faster because of how they're structured.

DisplayList building is just slow. I will see how much effect the patches I currently have will have on gmail. But don't expect miracles. This is just an issue I try to work on from time to time, making -first paint- display list building fast is not something we're really working on right now.

Comment 3

[Bas Schouten (:bas.schouten)]

Bug 1387399 will help a little here.

Comment 4

[Bas Schouten (:bas.schouten)]

I was just grabbing more profiles of displaylist building. nsStyleContext::IsGecko(), nsStyleContext::AsGecko() and nsStyleContext::GetAsGecko are all showing up significantly in the profiles, over 3% of the total time, and over 10% of displaylist building, is this a known issue?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bas Schouten (:bas.schouten) from comment #4)
> I was just grabbing more profiles of displaylist building.
> nsStyleContext::IsGecko(), nsStyleContext::AsGecko() and
> nsStyleContext::GetAsGecko are all showing up significantly in the profiles,
> over 3% of the total time, and over 10% of displaylist building, is this a
> known issue?

So, the AsGecko bit would be bug 1381844. Basically we landed some diagnostic asserts when we cast to the concrete style context type (Gecko or Servo) so that we can catch any bugs in crash stats. Might also be the IsGecko() call, depending on whether it's called from within the diagnostic assert or elsewhere. Wouldn't help in the GetAsGecko call.

We could turn off the diagnostic assertions if we needed to. Though if you're seeing this in profiles, it would be nice if you could have a look and see what hot path this is happening on, and whether you can hoist the typecast further up the stack, and have the relevant code operate on GeckoStyleContext/ServoStyleContext.

Comment 6

[Bas Schouten (:bas.schouten)]

This is happening on basically all the calls accessing the style context from nsIFrame and such. These are mostly individual calls to the StyleContext so I'm not sure any typecast could be 'hoisted out', but Matt would know better than me.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Yeah, I don't think we can those ones... I'd expect the compiler to be able to figure out that the assertion inside AsGecko for:

  IsGecko() ? AsGecko() : nullptr;

Always holds, and thus is removable, but maybe it doesn't?

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas.schouten) from comment #6)
> This is happening on basically all the calls accessing the style context
> from nsIFrame and such. These are mostly individual calls to the
> StyleContext so I'm not sure any typecast could be 'hoisted out', but Matt
> would know better than me.

We've had some patches to retrieve the style context once, and then pass it as a parameter to functions that need it to avoid some of this, we could go further with that if needed.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> Yeah, I don't think we can those ones... I'd expect the compiler to be able
> to figure out that the assertion inside AsGecko for:
> 
>   IsGecko() ? AsGecko() : nullptr;
> 
> Always holds, and thus is removable, but maybe it doesn't?

I doubt it will unfortunately, could we add an AsGeckoSafe() or similar function to use at callsites where we're 100% confident of the outcome?

Comment 9

[Bobby Holley (:bholley)]

I just backed out the diagnostic assertions in bug 1381844. NI Bas to reprofile and determine whether we still see any of this. I would expect the number to be reduced, but if it's not entirely eliminated it would definitely be worth doing the "retrieve once" stuff Matt suggested in comment 8.

Comment 10

[Bas Schouten (:bas.schouten)]

Bug 1363922 will likely help with this issue as well. Will reprofile.

Comment 11

[Bas Schouten (:bas.schouten)]

I had a look, nsStyleContext::IsGecko is the only bit that's still showing up significantly, AsGecko and GetAsGecko have both been eliminated as significant cost. I'm not sure whether there's anything more that can be done in that department.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Bas Schouten (:bas.schouten) from comment #11)
> I had a look, nsStyleContext::IsGecko is the only bit that's still showing
> up significantly, AsGecko and GetAsGecko have both been eliminated as
> significant cost. I'm not sure whether there's anything more that can be
> done in that department.

Well, it depends on the stacks - do you have any from your profiles? Ideally it would just be a few hot places where we could track the style backend higher up the stack and eliminate the check.

Comment 13

[Bas Schouten (:bas.schouten)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12)
> (In reply to Bas Schouten (:bas.schouten) from comment #11)
> > I had a look, nsStyleContext::IsGecko is the only bit that's still showing
> > up significantly, AsGecko and GetAsGecko have both been eliminated as
> > significant cost. I'm not sure whether there's anything more that can be
> > done in that department.
> 
> Well, it depends on the stacks - do you have any from your profiles? Ideally
> it would just be a few hot places where we could track the style backend
> higher up the stack and eliminate the check.

Roughly half is 'DoGetStyleDisplay', the other half from a myriad of other DoGetStyle* functions. No stack is particularly hot with relation to it, the biggest contributer with a little under 10% of the time in IsGecko() is nsDisplayListBuilder::FindReferenceFrameFor. So I doubt there's too much room there.

Comment 14

[Bobby Holley (:bholley)]

Ok. I kind of suspect the IsGecko call is just paying the L2 cache miss on the style context, which would be paid slightly later (looking up the style struct) without this call.

I am rather surprised you're seeing this as a bonafide stack frame in the profiler though, given that it's just a bit check. I wonder why it's not getting inlined.

Comment 15

[Daniel Holbert [:dholbert]]

Does it still make sense for this to be qf:p1? (and there anything actionable we can do here for 57?)

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> Does it still make sense for this to be qf:p1? (and there anything
> actionable we can do here for 57?)

I have a couple of patches that improve this by bits and pieces, but they're in different bugs.

Comment 17

[Jean Gong :jgong]

This bug won't be addressed in 57; moving it to P2 for post 57 work.

Comment 18

[Bas Schouten (:bas.schouten)]

We've landed a lot of stuff to improve these types of things, this should probably be retested.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

I think we can close this as INVALID at this point. The profiles are all out of date, and Google is going to roll out a new UI anyways, which probably will invalidate a bunch of analysis here.