1389385 - stylo: Overhaul dirty descendants propagation in preparation for restyle roots

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

Now we're getting to some of the patches I wrote in July for this. The final restyle root patch still needs some work to take advantage of bug 1389347, but all this stuff can land and stop bitrotting.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Move NoteDirtyDescendantsForServo out of line. v1

This function is large enough that it doesn't really make sense to have inline,
and we'll be adding more to it in the coming patches.

MozReview-Commit-ID: AnDfzwsMvNy

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Generalize C++ NoteDirtyDescendants logic and add equivalent APIs for the animation bit. v1

MozReview-Commit-ID: 8K0uDibfoZS

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Do all descendant bit propagation from Gecko over FFI. v1

Deduplicating code is nice, and it will help us when we make the bit
propagation more complicated in upcoming patches.

MozReview-Commit-ID: KIQnNJVayrM

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Rearrange dirty noting to operate on the element rather than the parent. v1

This will allow us to scope restyle roots more tightly.

MozReview-Commit-ID: 2t2lp5sKBHH

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Require a presshell to set descendant bits. v1

We're going to start tracking the restyle root on the presshell, so we'll need
one. This should be fine, since if the presshell doesn't exist yet we can't
have done the initial style, and if it's already been destroyed we don't need
restyle state anymore.

MozReview-Commit-ID: EfNVloI9ENQ

Comment 6

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2689e9212d1e5d35c6ba3eb6d62882c388bc5cf

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896116 [details] [diff] [review]
Part 2 - Generalize C++ NoteDirtyDescendants logic and add equivalent APIs for the animation bit. v1

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

I don't understand this and the previous patch, why not doing an FFI call to servo anyway, is this for the presShell stuff? We can check that making SetOwnerDocumentNeedsStyleFlush returning a boolean or something.

::: dom/base/Element.h
@@ +498,5 @@
> +  void SetHasAnimationOnlyDirtyDescendantsForServo() {
> +    MOZ_ASSERT(IsStyledByServo());
> +    SetFlags(ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO);
> +  }
> +

nit: Extra newline.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Can I get more info on why moving it all to the C++ side instead of the other way around? The later seemed easier off-hand. Is there something I'm missing?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Can I get more info on why moving it all to the C++ side instead of the
> other way around? The later seemed easier off-hand. Is there something I'm
> missing?

Gecko dom manipulation seems like a reasonable thing to do in gecko. Patches get more complex and involve presshell when restyle roots are involved. Water under the bridge at this point, unless the proposal is to rewrite all the patches to flip things around.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896116 [details] [diff] [review]
Part 2 - Generalize C++ NoteDirtyDescendants logic and add equivalent APIs for the animation bit. v1

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

Ok... whatever. I guess...

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896118 [details] [diff] [review]
Part 4 - Rearrange dirty noting to operate on the element rather than the parent. v1

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

r=me, with those moved to Element. Please re-request review, or ni? before landing, if that can be done for any reason.

::: dom/base/nsIContent.h
@@ +980,5 @@
>  
>    virtual bool OwnedOnlyByTheDOMTree() { return false; }
> +
> +  void NoteDirtyForServo();
> +  void NoteAnimationOnlyDirtyForServo();

I think these should move to Element. I can't think of a reason why we should mark an element of a just-inserted text-node as dirty unconditionally.

The ContentInserted stuff is supposed to take care of it, and lazy frame construction uses the NODE_DESCENDANTS_NEED_FRAMES flag.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896119 [details] [diff] [review]
Part 5 - Require a presshell to set descendant bits. v1

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

::: dom/base/Element.cpp
@@ +4206,5 @@
>  {
> +  MOZ_ASSERT(aContent->IsInComposedDoc());
> +  nsIDocument* doc = aContent->GetComposedDoc();
> +  nsIPresShell* shell = doc->GetShell();
> +  NS_ENSURE_TRUE_VOID(shell);

This is a super obscure way to say:

if (!shell) {
  return;
}

...

I think I prefer the second, but no huge deal.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8896119 [details] [diff] [review]
> Part 5 - Require a presshell to set descendant bits. v1
> 
> Review of attachment 8896119 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Element.cpp
> @@ +4206,5 @@
> >  {
> > +  MOZ_ASSERT(aContent->IsInComposedDoc());
> > +  nsIDocument* doc = aContent->GetComposedDoc();
> > +  nsIPresShell* shell = doc->GetShell();
> > +  NS_ENSURE_TRUE_VOID(shell);
> 
> This is a super obscure way to say:

Well, not super-obscure from within Gecko's DOM. ;-) But yeah, the advantage is that it's 1-line and that it warns.

I'll try the nsIContent->Element thing next.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 6 - Move NoteDirty* to Element. v1

Need to run this through try.

MozReview-Commit-ID: KvKAEuYkssx

Comment 15

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=782c491c24abadc263b8c3f6dbfaf20c769ecb3e

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896423 [details] [diff] [review]
Part 6 - Move NoteDirty* to Element. v1

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

::: dom/base/Element.cpp
@@ +4201,5 @@
>  #endif
>  
>  template<typename Traits>
>  void
> +NoteDirtyElement(nsIContent* aContent)

nit: Element* aElement

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3342b1c0d5ec
Move NoteDirtyDescendantsForServo out of line. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6f84fdcb7573
Generalize C++ NoteDirtyDescendants logic and add equivalent APIs for the animation bit. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f9eb00885da1
Do all descendant bit propagation from Gecko over FFI. r=emilio
https://hg.mozilla.org/integration/autoland/rev/4a977fea2799
Rearrange dirty noting to operate on the element rather than the parent. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a3184c7e748a
Require a presshell to set descendant bits. r=emilio

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3342b1c0d5ec
https://hg.mozilla.org/mozilla-central/rev/6f84fdcb7573
https://hg.mozilla.org/mozilla-central/rev/f9eb00885da1
https://hg.mozilla.org/mozilla-central/rev/4a977fea2799
https://hg.mozilla.org/mozilla-central/rev/a3184c7e748a

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Looks like the push from this bug improves the runtime of mochitest-e10s-9 task by 20%!

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Oh, nvm, maybe I said too fast. It seems the duration fluctuates a lot...