stylo: Overhaul dirty descendants propagation in preparation for restyle roots

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(6 attachments)

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.
Created attachment 8896115 [details] [diff] [review]
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
Attachment #8896115 - Flags: review?(emilio+bugs)
Created attachment 8896116 [details] [diff] [review]
Part 2 - Generalize C++ NoteDirtyDescendants logic and add equivalent APIs for the animation bit. v1

MozReview-Commit-ID: 8K0uDibfoZS
Attachment #8896116 - Flags: review?(emilio+bugs)
Created attachment 8896117 [details] [diff] [review]
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
Attachment #8896117 - Flags: review?(emilio+bugs)
Created attachment 8896118 [details] [diff] [review]
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
Attachment #8896118 - Flags: review?(emilio+bugs)
Created attachment 8896119 [details] [diff] [review]
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
Attachment #8896119 - Flags: review?(emilio+bugs)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2689e9212d1e5d35c6ba3eb6d62882c388bc5cf
Attachment #8896115 - Flags: review?(emilio+bugs) → review+
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.
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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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...
Attachment #8896116 - Flags: review?(emilio+bugs) → review+
Attachment #8896117 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8896118 - Flags: review?(emilio+bugs) → review+
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.
Attachment #8896119 - Flags: review?(emilio+bugs) → review+
(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.
Created attachment 8896423 [details] [diff] [review]
Part 6 - Move NoteDirty* to Element. v1

Need to run this through try.

MozReview-Commit-ID: KvKAEuYkssx
Attachment #8896423 - Flags: review?(emilio+bugs)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=782c491c24abadc263b8c3f6dbfaf20c769ecb3e
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
Attachment #8896423 - Flags: review?(emilio+bugs) → review+
Blocks: 1389681
Blocks: 1388031

Comment 17

4 months ago
Pushed by bholley@mozilla.com:
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
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
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Looks like the push from this bug improves the runtime of mochitest-e10s-9 task by 20%!
Oh, nvm, maybe I said too fast. It seems the duration fluctuates a lot...
You need to log in before you can comment on or make changes to this bug.