1385896 - stylo: Move nsStyleContext::mParent to GeckoStyleContext.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

Should be doable now, and will unblock bug 1368290.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1385896: Move nsStyleContext::mParent to GeckoStyleContext.

Review commit: https://reviewboard.mozilla.org/r/163238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163238/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I was meant to post this a fair while ago, and missed it, whoops. TL;DR: This needs to remove the "don't do change hints for unknown structs". We can't implement it _right now_.

We could implement it with a weak pointer once bug 1368290, but if we remove it we can also clean up a fair amount of code, so I'd like to properly measure whether it's useful today. In any case this is ready for review, we can re-introduce that optimization in bug 1368290 if we think it's useful.we

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch to measure the optimization.

This is the patch I've used to measure the "preserve hints" optimization.

From my testing in a normal browsing session, it has so far helped avoid a few "Repaint" hints, mostly (and I suspect these would be clobbered by more concrete repaints in ancestors or descendants, since they were definitely visual changes).

On youtube, it also helps avoid some UpdateParentOverflow and RecomputePosition hints.

I need to split it, also, between hints on reset and inherited structs, since keeping the optimization working on reset structs is trivial without the parent pointer.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: updated measuring patch.

I used this patch to distinguish reset and inherit hints, and most of the hints (and the "strongest" ones) we avoid are from reset structs.

Still it's not too frequent to hit the optimization, but on Twitter, Github and Youtube this still gives us most of the hits, and the most important ones, actually. We'd only lose the optimization for a few NeutralChanges (which don't matter, really), and very sporadically a few RepaintFrames, which as I told above I think they're really subsumed anyway.

Cam, do you think it's reasonable to keep the optimization only for reset structs? This would allow to keep it working in most and the most important cases, and still avoid the weak parent pointer from the text style to the parent, and allow simplifying a lot of the code.

Comment 5

[Cameron McCormack (:heycam)]

Thanks for doing the checking.  I haven't looked at the patch in detail yet.  Why is it that the lack of parent pointer makes it harder to do this optimization, and why is it that it's easier for reset structs?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Basically because we need this hack for text styles right now. And that effectively only affects inherit structs.

http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/layout/style/nsStyleContextInlines.h#125

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8892291 [details]
Bug 1385896: Move nsStyleContext::mParent to GeckoStyleContext.

https://reviewboard.mozilla.org/r/163238/#review169616

::: layout/style/GeckoStyleContext.cpp:648
(Diff revision 1)
>    // Here we set up various style bits for both the Gecko and Servo paths.
>    // _Only_ change the bits here.  For fixups of the computed values, you can
>    // add to ApplyStyleFixups in Gecko and StyleAdjuster as part of Servo's
>    // cascade.

Please update this comment now that we don't call it for Servo contexts.

::: layout/style/nsStyleContext.cpp:91
(Diff revision 1)
> -  if (auto gecko = GetAsGecko())
> -    gecko->RemoveChild(aChild->AsGecko());
> -}
> -
> -void
>  nsStyleContext::FinishConstruction()

Please remove this unused function, although we probably want to preserve the static assertions it's doing.

::: layout/style/nsStyleContext.cpp:183
(Diff revision 1)
>    // Servo's optimization to stop the cascade when there are no style changes
>    // that children need to be recascade for relies on comparing all of the
>    // structs, not just those that are returned from PeekStyleData, although
>    // if PeekStyleData does return null we still don't want to accumulate
>    // any change hints for those structs.

Please update this comment to point out that we're no longer doing this "still don't accumulate any change hints" thing, and why.  (And maybe that we can re-add the optimization for reset structs in a later patch?)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8892291 [details]
Bug 1385896: Move nsStyleContext::mParent to GeckoStyleContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163238/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db88d543de14
Move nsStyleContext::mParent to GeckoStyleContext. r=heycam

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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