1385658 - stylo: ::first-line could be a serious performance problem

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing slow pageload with first-line on a long block

OK, this is my fault: I had forgotten a detail of how first-line works.  I thought it only did reparenting when things were moved to/from the first line in reflow, which is _sort_ of true.

There are two ways in which that's not true.  First, when we initially create the first-line frame, we stick all the leading inline kids of the block in it.  That means that we reparent them all, and then during reflow reparent all the ones that don't fit again, as we push them to the second and later lines.  Stylo's reparent is a lot slower than Gecko's, because Gecko's uses the style context tree and its cache of kids to do reparenting _really_ fast (with style context sharing) when we have lots of things with similar styles.  The thing I did in bug 1324619 just resolves a new ComputedValues, and I end up with profiles like https://perfht.ml/2eX3lkA (servo on the attached testcase) which shows a bunch of time under Servo_ReparentStyle and also lots of time under Servo_StyleContext_Release and style context destruction.

I tried dealing with that by changing frame construction to only stick the first inline in the first-line and stick the rest into a first-line continuation.  But that actually makes no difference, because nsFirstLineFrame::Reflow does this:

  if (nullptr == GetPrevInFlow()) {
    // XXX This is pretty sick, but what we do here is to pull-up, in
    // advance, all of the next-in-flows children. We re-resolve their
    // style while we are at at it so that when we reflow they have
    // the right style.
    //
    // All of this is so that text-runs reflow properly.

and then it does just what it says: pulls everything from all the continuing lines up into the first line, reparents them into the ::first-line, then does a reflow and pushes a bunch of them back, reparenting them _again_.

So the upshot is that reflow performance for a block with a large number of leading inlines and ::first-line is pretty terrible.  It's not great with Gecko either, because while the reparenting is fast we do end up rebuilding the textruns all the time, but stylo with the bug 1324619 patches is a good bit worse.

I'm not quite sure yet to what extent this is a problem in the wild or how best to address it if it is.  Maybe we can do some sort of style reparenting cache to get a behavior closer to gecko's, at least.  But this is generally pretty sucky.  Gecko is a lot slower on the attached testcase than Blink is, for example.  Granted, it's a pretty pathological testcase.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly modified testcase showing even slower reframing

I'm actually not sure why this is slower (with both Gecko and Stylo) than the full pageload... but it is.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I think this hopefully isn't a huge problem in practice, but I agree it kinda sucks... :(

FWIW what blink does is not having a special frame in the frame tree for first-line, they just lazily compute the first-line style, and query it from the reflow code in the appropriate places, so they will generally have better perf than us for first-line dynamic changes, I'm pretty sure.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> FWIW what blink does is not having a special frame in the frame tree for
> first-line, they just lazily compute the first-line style, and query it from
> the reflow code in the appropriate places, so they will generally have
> better perf than us for first-line dynamic changes, I'm pretty sure.

Oh, and the fact that they move all this to the frame tree of course means that they incorrectly skip display: contents nodes wrt first-line inheritance, but...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Gecko has issues with display:contents in first-line too, fwiw:

  <style>
    div { color: red }
    div::first-line { color: green; }
    span { display: contents; }
  </style>
  <div>Is this <span>text all</span> green?</div>

You _have_ to do first-line on the frame tree; there is no other option, really.

I expect Blink is buggy in the textrun cases that our code I cited worries about, but last I checked it's buggy in those cases in non-first-line cases too (e.g. don't do ligatures across element boundaries)....

Comment 5

[Bobby Holley (:bholley)]

Boris, can you confirm that it's ok to ship with this?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think so, yes.  ::first-line styles are not that common, and they're especially rare on super-long blocks, I think: most cases which use them are places that care about typography, and having a really long block is not something that looks good in general.

Comment 7

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals