1383332 - stylo: Maintain a restyle root and use it to cull the traversal

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

I have patches for this which seem to shave off a ton of overhead in the large-dom+small-invalidation ("tiny traversal") cases.

On the microbenchmark at [1], we have the following numbers (Gecko baseline = 1200ms):
* Without my patches: Stylo-Sequential=2200ms, Stylo-Parallel=4800ms
* With my patches: Gecko=1200ms, Stylo-Sequential=1500ms, Stylo-Parallel=1600ms

I'm assuming these wins will translate to speedometer (Inferno), whose characteristics were the model for the microbenchmark. However, my patches appear to trigger a panic on the testrunner, so presumably I still have a bug or two to fix. :-)

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a5883007cbcbeef39f226e26beb30a7a04f4015

[1] https://github.com/heycam/style-perf-tests/blob/master/perf-reftest/tiny-traversal-singleton.html

Comment 1

[Bobby Holley (:bholley)]

(Actually, I'll bet those parallel numbers are inaccurate given the logic we have to use the sequential traversal when not starting from the root. I'll need to fix that and remeasure).

Comment 2

[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 3

[Bobby Holley (:bholley)]

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

MozReview-Commit-ID: 8K0uDibfoZS

Comment 4

[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 5

[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 6

[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 7

[Bobby Holley (:bholley)]

Whoops, I meant to upload these to bug 1389385.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Track the restyle root and use it to do less work during the traversal. v1

MozReview-Commit-ID: A8O3JOpsv4E

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Justo curious, before I review this today... Do you have any numbers?

Comment 10

[Bobby Holley (:bholley)]

Yes. Sequential remains similar to comment 0. Parallel is in the 3000s iirc. More work to do there, but this patch helps a lot.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8898634 [details] [diff] [review]
Track the restyle root and use it to do less work during the traversal. v1

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

Looks reasonable over all... I think we should kill the RestyleForReconstruct stuff to be honest...

I think I have enough comments for this patch to deserve another round of review. I really think I prefer the correct fix for the invalidator stuff instead of the workaround.

::: dom/base/Element.cpp
@@ +4171,5 @@
> +  // is necessary for correctness, since we invoke ClearServoData in various
> +  // places where an element's flattened tree parent changes, and such a change
> +  // may also make an element invalid to be used as a restyle root.
> +  nsIPresShell* shell = GetComposedDoc()->GetShell();
> +  if (shell && shell->GetServoRestyleRoot() == this) {

How does this play with all the "lazily clear presshell state if the doc is in the bfcache" stuff?

@@ +4233,4 @@
>    }
>  };
>  
>  struct AnimationOnlyDirtyDescendantsBit {

These structs don't seem used anymore.

@@ +4265,5 @@
> +static inline Element*
> +PropagateBits(Element* aElement, uint32_t aBits, nsINode* aStopAt)
> +{
> +  Element* curr = aElement;
> +  while (curr && (curr->GetFlags() & aBits) != aBits) {

nit: Add a HasAllFlags method, and use it here?

@@ +4345,5 @@
> +    //
> +    // We check for a frame to reduce the cases where we need to go digging for
> +    // the style.
> +    if (!parent->GetPrimaryFrame()) {
> +      RefPtr<ServoStyleContext> parentStyle =

Should we just add a Servo_IsInStyledTree or something like that, that avoids addreffing / releasing the style context?

@@ +4381,5 @@
> +    // now need to find the nearest common ancestor, so climb up from the
> +    // existing root, extending bits along the way.
> +    Element* commonAncestor = PropagateBitsFromParent(existingRoot, existingBits, aElement);
> +
> +    if (commonAncestor) {

nit: can merge using `if (Element* commonAncestor = ...)`.

@@ +4394,5 @@
> +    } else {
> +      // We didn't find a common ancestor element. That means we're descended
> +      // from two different document style roots, so the common ancestor is the
> +      // document.
> +      shell->SetServoRestyleRoot(doc, existingBits | aBit);

nit: use either `aBit | existingBits` or `existingBits | aBit` consistently,

::: dom/base/Element.h
@@ +467,5 @@
>    bool GetBindingURL(nsIDocument* aDocument, css::URLValue **aResult);
>  
>    Directionality GetComputedDirectionality() const;
>  
> +  static const uint32_t kServoDescendantBits =

nit: Maybe `kAllServoDescendantBits`?

::: layout/base/nsCSSFrameConstructor.cpp
@@ +7332,3 @@
>    if (mozilla::GeckoRestyleManager* geckoRM = RestyleManager()->GetAsGecko()) {
> +    while (parent &&
> +           !parent->HasFlag(NODE_DESCENDANTS_NEED_FRAMES)) {

nit: This fits on the same line.

::: layout/base/nsIPresShell.h
@@ +1860,5 @@
> +  // Restyle root for servo's style system.
> +  //
> +  // We also track which "descendant" bits (normal/animation-only/lazy-fc) the
> +  // root corresponds to.
> +  nsCOMPtr<nsINode> mServoRestyleRoot;

Please comment on why a node instead of an element (which is IIUC because we can set the doc).

::: layout/base/nsIPresShellInlines.h
@@ +44,5 @@
>    }
>  }
>  
> +nsINode*
> +nsIPresShell::GetServoRestyleRoot()

This doesn't need to be in this file.

@@ +50,5 @@
> +  return mServoRestyleRoot;
> +}
> +
> +uint32_t
> +nsIPresShell::GetServoRestyleRootDirtyBits()

nor this.

@@ +58,5 @@
> +  return mServoRestyleRootDirtyBits;
> +}
> +
> +void
> +nsIPresShell::ClearServoRestyleRoot()

nor this

::: layout/style/ServoStyleSet.cpp
@@ +866,5 @@
>    // NAC subtree roots.
>    bool postTraversalRequired = false;
> +
> +  Element* rootElement = mPresContext->Document()->GetRootElement();
> +  bool isInitialForPrimary = rootElement && !rootElement->HasServoData();

What is `isInitialForPrimary` supposed to mean? You mean `isInitialDocumentStyling` or something of that sort? I'm not sure what is the `forPrimary` supposed to mean.

@@ +1027,5 @@
>    MOZ_ASSERT(aRoot->HasServoData());
> +
> +  // If the restyle root is beneath |aRoot|, there won't be any descendants bit
> +  // leading us to aRoot. In this case, we need to traverse from the restyle
> +  // root instead.

I get this, but this feels like a hack... :(

@@ +1399,5 @@
>  bool
>  ServoStyleSet::MayTraverseFrom(Element* aElement)
>  {
>    MOZ_ASSERT(aElement->IsInComposedDoc());
> +  if (aElement->HasServoData()) {

This deserves a comment or explanation... Why is this needed?

::: servo/components/style/dom.rs
@@ +487,5 @@
> +    /// Notes that an element and/or its descendants have style invalidations,
> +    /// flagging a style flush and updating the restyle root as appropriate.
> +    ///
> +    /// Only safe to call on the main thread.
> +    unsafe fn note_dirty_element(&self) {}

As I said in other comment, I think this doesn't belong here at all.

::: servo/components/style/gecko/wrapper.rs
@@ +530,5 @@
> +               self.needs_frame(),
> +               self.borrow_data().unwrap().restyle);
> +
> +        let has_flag =
> +            self.flags() & (ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 |

is this really just a micro-optimization to avoid checking each flag one by one? Doesn't seem too worth to me, and seems to hurt readability.

::: servo/components/style/invalidation/stylesheets.rs
@@ +119,5 @@
>      pub fn flush<E>(&mut self, document_element: Option<E>)
>          where E: TElement,
>      {
>          if let Some(e) = document_element {
> +            let have_invalidations = self.process_invalidations(e);

Instead of this, and adding a method to TElement that is only used for something very Gecko-specific, and that does nothing for Servo, let's propagate this all the way up, and do this after the flush_stylesheets call.

::: servo/components/style/traversal.rs
@@ +598,5 @@
>  
> +    // FIXME(bholley): We should ideally be able to assert that the dirty bits
> +    // are not set here. However, TreeStyleInvalidator::invalidate_child() can
> +    // propagate dirty bits even if the child is unstyled, meaning we can set
> +    // these bits on display:none roots that don't get styled.

display: none roots do get styled, do you mean restyled?

Also, it seems to me that the fix for this is easier than the workaround. In particular, it should be enough with replacing:

if invalidated_child {

with:

if invalidated_child && self.data.is_some() {

Shouldn't it? (and ditto for the other similar condition where it sets dirty bits)

@@ +617,5 @@
> +
> +fn clear_state_after_traversing<E>(
> +    element: E,
> +    data: &mut ElementData,
> +    flags: TraversalFlags)

nit: Move the parent to the next line, the where to the first column:

)
where
    E: TElement,
{

Comment 12

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> Comment on attachment 8898634 [details] [diff] [review]
> Track the restyle root and use it to do less work during the traversal. v1
> 
> Review of attachment 8898634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable over all... I think we should kill the
> RestyleForReconstruct stuff to be honest...
> 
> I think I have enough comments for this patch to deserve another round of
> review. I really think I prefer the correct fix for the invalidator stuff
> instead of the workaround.
> 
> ::: dom/base/Element.cpp
> @@ +4171,5 @@
> > +  // is necessary for correctness, since we invoke ClearServoData in various
> > +  // places where an element's flattened tree parent changes, and such a change
> > +  // may also make an element invalid to be used as a restyle root.
> > +  nsIPresShell* shell = GetComposedDoc()->GetShell();
> > +  if (shell && shell->GetServoRestyleRoot() == this) {
> 
> How does this play with all the "lazily clear presshell state if the doc is
> in the bfcache" stuff?

Per discussion, I'm going to move this to the document.

> 
> @@ +4233,4 @@
> >    }
> >  };
> >  
> >  struct AnimationOnlyDirtyDescendantsBit {
> 
> These structs don't seem used anymore.

Good catch.

> 
> @@ +4265,5 @@
> > +static inline Element*
> > +PropagateBits(Element* aElement, uint32_t aBits, nsINode* aStopAt)
> > +{
> > +  Element* curr = aElement;
> > +  while (curr && (curr->GetFlags() & aBits) != aBits) {
> 
> nit: Add a HasAllFlags method, and use it here?

Nice idea.

> 
> @@ +4345,5 @@
> > +    //
> > +    // We check for a frame to reduce the cases where we need to go digging for
> > +    // the style.
> > +    if (!parent->GetPrimaryFrame()) {
> > +      RefPtr<ServoStyleContext> parentStyle =
> 
> Should we just add a Servo_IsInStyledTree or something like that, that
> avoids addreffing / releasing the style context?

I'm going to add a Servo_Element_IsDisplayNone.

> 
> @@ +4381,5 @@
> > +    // now need to find the nearest common ancestor, so climb up from the
> > +    // existing root, extending bits along the way.
> > +    Element* commonAncestor = PropagateBitsFromParent(existingRoot, existingBits, aElement);
> > +
> > +    if (commonAncestor) {
> 
> nit: can merge using `if (Element* commonAncestor = ...)`.

Fixed.

> 
> @@ +4394,5 @@
> > +    } else {
> > +      // We didn't find a common ancestor element. That means we're descended
> > +      // from two different document style roots, so the common ancestor is the
> > +      // document.
> > +      shell->SetServoRestyleRoot(doc, existingBits | aBit);
> 
> nit: use either `aBit | existingBits` or `existingBits | aBit` consistently,
> 
> ::: dom/base/Element.h
> @@ +467,5 @@
> >    bool GetBindingURL(nsIDocument* aDocument, css::URLValue **aResult);
> >  
> >    Directionality GetComputedDirectionality() const;
> >  
> > +  static const uint32_t kServoDescendantBits =
> 
> nit: Maybe `kAllServoDescendantBits`?

Ok.

> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +7332,3 @@
> >    if (mozilla::GeckoRestyleManager* geckoRM = RestyleManager()->GetAsGecko()) {
> > +    while (parent &&
> > +           !parent->HasFlag(NODE_DESCENDANTS_NEED_FRAMES)) {
> 
> nit: This fits on the same line.

Ok.

> 
> ::: layout/base/nsIPresShell.h
> @@ +1860,5 @@
> > +  // Restyle root for servo's style system.
> > +  //
> > +  // We also track which "descendant" bits (normal/animation-only/lazy-fc) the
> > +  // root corresponds to.
> > +  nsCOMPtr<nsINode> mServoRestyleRoot;
> 
> Please comment on why a node instead of an element (which is IIUC because we
> can set the doc).

Done.

> 
> ::: layout/base/nsIPresShellInlines.h
> @@ +44,5 @@
> >    }
> >  }
> >  
> > +nsINode*
> > +nsIPresShell::GetServoRestyleRoot()
> 
> This doesn't need to be in this file.
> 
> @@ +50,5 @@
> > +  return mServoRestyleRoot;
> > +}
> > +
> > +uint32_t
> > +nsIPresShell::GetServoRestyleRootDirtyBits()
> 
> nor this.
> 
> @@ +58,5 @@
> > +  return mServoRestyleRootDirtyBits;
> > +}
> > +
> > +void
> > +nsIPresShell::ClearServoRestyleRoot()
> 
> nor this

These were all here because I was previously doing more fancy stuff in them. Anyway, moot point since I'm moving it all to the doc.

> 
> ::: layout/style/ServoStyleSet.cpp
> @@ +866,5 @@
> >    // NAC subtree roots.
> >    bool postTraversalRequired = false;
> > +
> > +  Element* rootElement = mPresContext->Document()->GetRootElement();
> > +  bool isInitialForPrimary = rootElement && !rootElement->HasServoData();
> 
> What is `isInitialForPrimary` supposed to mean? You mean
> `isInitialDocumentStyling` or something of that sort? I'm not sure what is
> the `forPrimary` supposed to mean.

I called it isInitialForMainDoc and added a comment explaining the distinction.

> 
> @@ +1027,5 @@
> >    MOZ_ASSERT(aRoot->HasServoData());
> > +
> > +  // If the restyle root is beneath |aRoot|, there won't be any descendants bit
> > +  // leading us to aRoot. In this case, we need to traverse from the restyle
> > +  // root instead.
> 
> I get this, but this feels like a hack... :(

Yeah. We should get rid of this eventually, but probably not in the 57 timeframe.

> 
> @@ +1399,5 @@
> >  bool
> >  ServoStyleSet::MayTraverseFrom(Element* aElement)
> >  {
> >    MOZ_ASSERT(aElement->IsInComposedDoc());
> > +  if (aElement->HasServoData()) {
> 
> This deserves a comment or explanation... Why is this needed?

Just as an optimization, back when I was calling this in non-debug scenarios. I'll just remove it.

> 
> ::: servo/components/style/dom.rs
> @@ +487,5 @@
> > +    /// Notes that an element and/or its descendants have style invalidations,
> > +    /// flagging a style flush and updating the restyle root as appropriate.
> > +    ///
> > +    /// Only safe to call on the main thread.
> > +    unsafe fn note_dirty_element(&self) {}
> 
> As I said in other comment, I think this doesn't belong here at all.
> 
> ::: servo/components/style/gecko/wrapper.rs
> @@ +530,5 @@
> > +               self.needs_frame(),
> > +               self.borrow_data().unwrap().restyle);
> > +
> > +        let has_flag =
> > +            self.flags() & (ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
> 
> is this really just a micro-optimization to avoid checking each flag one by
> one? Doesn't seem too worth to me, and seems to hurt readability.
> 
> ::: servo/components/style/invalidation/stylesheets.rs
> @@ +119,5 @@
> >      pub fn flush<E>(&mut self, document_element: Option<E>)
> >          where E: TElement,
> >      {
> >          if let Some(e) = document_element {
> > +            let have_invalidations = self.process_invalidations(e);
> 
> Instead of this, and adding a method to TElement that is only used for
> something very Gecko-specific, and that does nothing for Servo, let's
> propagate this all the way up, and do this after the flush_stylesheets call.
> 
> ::: servo/components/style/traversal.rs
> @@ +598,5 @@
> >  
> > +    // FIXME(bholley): We should ideally be able to assert that the dirty bits
> > +    // are not set here. However, TreeStyleInvalidator::invalidate_child() can
> > +    // propagate dirty bits even if the child is unstyled, meaning we can set
> > +    // these bits on display:none roots that don't get styled.
> 
> display: none roots do get styled, do you mean restyled?
> 
> Also, it seems to me that the fix for this is easier than the workaround. In
> particular, it should be enough with replacing:
> 
> if invalidated_child {
> 
> with:
> 
> if invalidated_child && self.data.is_some() {
> 
> Shouldn't it? (and ditto for the other similar condition where it sets dirty
> bits)

Ok, I will try it. Going to separate these changes out into a few iterations on try to keep regression ranges manageable.

> 
> @@ +617,5 @@
> > +
> > +fn clear_state_after_traversing<E>(
> > +    element: E,
> > +    data: &mut ElementData,
> > +    flags: TraversalFlags)
> 
> nit: Move the parent to the next line, the where to the first column:
> 
> )
> where
>     E: TElement,
> {

Ok, but this is different from how we've been doing it. Can you point me to where this style is documented?

Comment 13

[Bobby Holley (:bholley)]

So on friday I got this all green on linux64, but then hit some a11y failures on the other platforms. I debugged those today (which was a long progress, unfortunately).

It turns out that we were clearing NODE_NEEDS_FRAME on display:none roots, which caused us to avoid coalescing the entire set of siblings for lazy frame construction, which caused us to enter the frame constructor with ContentRangeInserted rather than ContentAppended. And ContentRangeInserted, unlike ContentAppended for some reason, does not propagate hasNoXBLChildren to the item list, which means that we miss the empty-text-frame-skipping optimization at http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/base/nsCSSFrameConstructor.cpp#6372 , which means that the accessibility tree ends up with a node for it, which messes up all the offsets that the test depends on.

There were also a few odd-looking WPT failures on non-linux platforms, but I'm really hoping this fix took care of them, because this is long overdue to land.

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

Comment 14

[Bobby Holley (:bholley)]

Rebase compilation bustage on the try push above.

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

Comment 15

[Bobby Holley (:bholley)]

Attachment: Track the restyle root and use it to do less work during the traversal. v2

MozReview-Commit-ID: A8O3JOpsv4E

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8899686 [details] [diff] [review]
Track the restyle root and use it to do less work during the traversal. v2

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

Looks good, with the comments addressed.

::: dom/base/Element.cpp
@@ +4170,5 @@
> +  // servo data either, so we can forget restyles rooted at this element. This
> +  // is necessary for correctness, since we invoke ClearServoData in various
> +  // places where an element's flattened tree parent changes, and such a change
> +  // may also make an element invalid to be used as a restyle root.
> +  if (aDoc && aDoc->GetServoRestyleRoot() == this) {

Shouldn't always be a document here, if there was any actual data? Can we assert it? if not, why not?

@@ +4247,5 @@
>  }
>  #endif
>  
> +static inline Element*
> +PropagateBits(Element* aElement, uint32_t aBits, nsINode* aStopAt)

needs docs.

@@ +4262,5 @@
> +  return curr;
> +}
> +
> +static inline Element*
> +PropagateBitsFromParent(nsINode* aNode, uint32_t aBits, nsINode* aStopAt)

nit: needs docs.

@@ +4349,5 @@
>    }
>  
> +  // There is an existing restyle root - walk up the tree from our element,
> +  // propagating bits as wel go.
> +  bool reachedDocRoot = !parent || !PropagateBits(parent, aBit, existingRoot);

nit: const.

@@ +4351,5 @@
> +  // There is an existing restyle root - walk up the tree from our element,
> +  // propagating bits as wel go.
> +  bool reachedDocRoot = !parent || !PropagateBits(parent, aBit, existingRoot);
> +
> +  if (!reachedDocRoot) {

Could this check || existingRoot == doc? That'd make that case marginally faster, and you could assert that `existingRoot` is an element in PropagateBitsFromParent, right?

@@ +4354,5 @@
> +
> +  if (!reachedDocRoot) {
> +      // We're a descendant of the existing root. All that's left to do is to
> +      // make sure the bit we propagated is also registered on the root.
> +      doc->SetServoRestyleRoot(existingRoot, existingBits | aBit);

nit: This wasn't addressed from my previous review, use `aBit | existingBits` or `existingBits | aBit` consistently across the function.

@@ +4363,5 @@
> +    if (Element* commonAncestor = PropagateBitsFromParent(existingRoot, existingBits, aElement)) {
> +      // We found a common ancestor. Make that the new style root, and clear the
> +      // bits between the new style root and the document root.
> +      doc->SetServoRestyleRoot(commonAncestor, existingBits | aBit);
> +      Element* curr = commonAncestor;

It's slightly unfortunate that we need to end up walking all the way up again, but I can't think of anything particularly better.

nit: This would be more consistent if you did `curr = commonAncestor->GetFlattenedTreeParentElementForStyle();` here, and then at the end of the loop, though it'd be a line more, so I don't feel too strongly about it.

::: dom/base/nsIDocument.h
@@ +3029,5 @@
>    virtual bool IsThirdParty() = 0;
>  
>    bool IsScopedStyleEnabled();
>  
> +  nsINode* GetServoRestyleRoot()

nit: You may want these to be const?

::: layout/base/PresShell.cpp
@@ +1336,5 @@
>    // This shell must be removed from the document before the frame
>    // hierarchy is torn down to avoid finding deleted frames through
>    // this presshell while the frames are being torn down
>    if (mDocument) {
> +    mDocument->ClearServoRestyleRoot();

nit: I'd move it after the assertion for consistency, but no big deal.

::: layout/base/ServoRestyleManager.cpp
@@ +802,5 @@
>        styleFrame, aElement, aRestyleState.ChangeList());
>    }
>  
>    const bool traverseElementChildren =
> +    aElement->GetFlags() & Element::kAllServoDescendantBits;

nit: Maybe worth to add a HasAnyOfFlags?

@@ +999,5 @@
>    if (mRestyleForCSSRuleChanges) {
>      aFlags |= ServoTraversalFlags::ForCSSRuleChanges;
>    }
>  
> +  while (doc->GetServoRestyleRoot()) {

what about:

  while (doc->GetServoRestyleRoot() && styleSet->StyleDocument(aFlags))

Given we clear the restyle root anyway after the loop?

Or, maybe just making StyleDocument() check this and early return false?

::: layout/style/ServoStyleSet.cpp
@@ +868,5 @@
> +
> +  nsIDocument* doc = mPresContext->Document();
> +  Element* rootElement = doc->GetRootElement();
> +  // NB: We distinguish between the main document and document-level NAC here.
> +  bool isInitialForMainDoc = rootElement && !rootElement->HasServoData();

const?

::: servo/components/style/invalidation/stylesheets.rs
@@ +126,1 @@
>          if let Some(e) = document_element {

`let have_invalidations = match ...` instead? Your call

::: servo/components/style/traversal.rs
@@ +613,5 @@
> +    data: &mut ElementData,
> +    flags: TraversalFlags
> +)
> +where
> +    E: TElement,

(Oh, btw, forgot to reply before, but this is because of https://github.com/rust-lang-nursery/fmt-rfcs/)

@@ +865,5 @@
>                  }
>              }
>          }
> +        if p == root {
> +            // Make sure not to clear NODE_NEEDS_FRAME on the root.

That feels kind of like a one-off hack, but... I guess.

Maybe it's a bit nicer to read like:

while ... {
    // ...
    if p != root {
        p.clear_dirty_bits();
    }
}

root.clear_descendants_bits();

wdyt?

Comment 17

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> Comment on attachment 8899686 [details] [diff] [review]
> Track the restyle root and use it to do less work during the traversal. v2
> 
> Review of attachment 8899686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, with the comments addressed.
> 
> ::: dom/base/Element.cpp
> @@ +4170,5 @@
> > +  // servo data either, so we can forget restyles rooted at this element. This
> > +  // is necessary for correctness, since we invoke ClearServoData in various
> > +  // places where an element's flattened tree parent changes, and such a change
> > +  // may also make an element invalid to be used as a restyle root.
> > +  if (aDoc && aDoc->GetServoRestyleRoot() == this) {
> 
> Shouldn't always be a document here, if there was any actual data? Can we
> assert it? if not, why not?

I definitely hit that condition, though it _may_ have been in a situation where we didn't have servo data to begin with. I'm going to try to clean this up in a followup patch.

 
> @@ +4247,5 @@
> >  }
> >  #endif
> >  
> > +static inline Element*
> > +PropagateBits(Element* aElement, uint32_t aBits, nsINode* aStopAt)
> 
> needs docs.

Done.

> 
> @@ +4262,5 @@
> > +  return curr;
> > +}
> > +
> > +static inline Element*
> > +PropagateBitsFromParent(nsINode* aNode, uint32_t aBits, nsINode* aStopAt)
> 
> nit: needs docs.

Done.

> 
> @@ +4349,5 @@
> >    }
> >  
> > +  // There is an existing restyle root - walk up the tree from our element,
> > +  // propagating bits as wel go.
> > +  bool reachedDocRoot = !parent || !PropagateBits(parent, aBit, existingRoot);
> 
> nit: const.

That's not very idiomatic, but I guess it improves readability. I'll do it.

> 
> @@ +4351,5 @@
> > +  // There is an existing restyle root - walk up the tree from our element,
> > +  // propagating bits as wel go.
> > +  bool reachedDocRoot = !parent || !PropagateBits(parent, aBit, existingRoot);
> > +
> > +  if (!reachedDocRoot) {
> 
> Could this check || existingRoot == doc? That'd make that case marginally
> faster, and you could assert that `existingRoot` is an element in
> PropagateBitsFromParent, right?

I'll do that in the followup to avoid another try push.

> 
> @@ +4354,5 @@
> > +
> > +  if (!reachedDocRoot) {
> > +      // We're a descendant of the existing root. All that's left to do is to
> > +      // make sure the bit we propagated is also registered on the root.
> > +      doc->SetServoRestyleRoot(existingRoot, existingBits | aBit);
> 
> nit: This wasn't addressed from my previous review, use `aBit |
> existingBits` or `existingBits | aBit` consistently across the function.

Whoops. Fixed.

> 
> @@ +4363,5 @@
> > +    if (Element* commonAncestor = PropagateBitsFromParent(existingRoot, existingBits, aElement)) {
> > +      // We found a common ancestor. Make that the new style root, and clear the
> > +      // bits between the new style root and the document root.
> > +      doc->SetServoRestyleRoot(commonAncestor, existingBits | aBit);
> > +      Element* curr = commonAncestor;
> 
> It's slightly unfortunate that we need to end up walking all the way up
> again, but I can't think of anything particularly better.
> 
> nit: This would be more consistent if you did `curr =
> commonAncestor->GetFlattenedTreeParentElementForStyle();` here, and then at
> the end of the loop, though it'd be a line more, so I don't feel too
> strongly about it.
> 
> ::: dom/base/nsIDocument.h
> @@ +3029,5 @@
> >    virtual bool IsThirdParty() = 0;
> >  
> >    bool IsScopedStyleEnabled();
> >  
> > +  nsINode* GetServoRestyleRoot()
> 
> nit: You may want these to be const?

Ok.

> 
> ::: layout/base/PresShell.cpp
> @@ +1336,5 @@
> >    // This shell must be removed from the document before the frame
> >    // hierarchy is torn down to avoid finding deleted frames through
> >    // this presshell while the frames are being torn down
> >    if (mDocument) {
> > +    mDocument->ClearServoRestyleRoot();
> 
> nit: I'd move it after the assertion for consistency, but no big deal.

Ok.

> 
> ::: layout/base/ServoRestyleManager.cpp
> @@ +802,5 @@
> >        styleFrame, aElement, aRestyleState.ChangeList());
> >    }
> >  
> >    const bool traverseElementChildren =
> > +    aElement->GetFlags() & Element::kAllServoDescendantBits;
> 
> nit: Maybe worth to add a HasAnyOfFlags?

Ok.

> 
> @@ +999,5 @@
> >    if (mRestyleForCSSRuleChanges) {
> >      aFlags |= ServoTraversalFlags::ForCSSRuleChanges;
> >    }
> >  
> > +  while (doc->GetServoRestyleRoot()) {
> 
> what about:
> 
>   while (doc->GetServoRestyleRoot() && styleSet->StyleDocument(aFlags))
> 
> Given we clear the restyle root anyway after the loop?

Followup.

> 
> Or, maybe just making StyleDocument() check this and early return false?
> 
> ::: layout/style/ServoStyleSet.cpp
> @@ +868,5 @@
> > +
> > +  nsIDocument* doc = mPresContext->Document();
> > +  Element* rootElement = doc->GetRootElement();
> > +  // NB: We distinguish between the main document and document-level NAC here.
> > +  bool isInitialForMainDoc = rootElement && !rootElement->HasServoData();
> 
> const?
> 
> ::: servo/components/style/invalidation/stylesheets.rs
> @@ +126,1 @@
> >          if let Some(e) = document_element {
> 
> `let have_invalidations = match ...` instead? Your call

Done.

> 
> ::: servo/components/style/traversal.rs
> @@ +613,5 @@
> > +    data: &mut ElementData,
> > +    flags: TraversalFlags
> > +)
> > +where
> > +    E: TElement,
> 
> (Oh, btw, forgot to reply before, but this is because of
> https://github.com/rust-lang-nursery/fmt-rfcs/)

I don't see anything in that guide describing the style you're proposing, but maybe I'm missing it. Can you link me to it?

> 
> @@ +865,5 @@
> >                  }
> >              }
> >          }
> > +        if p == root {
> > +            // Make sure not to clear NODE_NEEDS_FRAME on the root.
> 
> That feels kind of like a one-off hack, but... I guess.
> 
> Maybe it's a bit nicer to read like:
> 
> while ... {
>     // ...
>     if p != root {
>         p.clear_dirty_bits();
>     }
> }
> 
> root.clear_descendants_bits();
> 
> wdyt?

Followup.

Comment 18

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/18196

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86b793bcbcd0
Track the restyle root and use it to do less work during the traversal. r=emilio

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bfd9f6f99c3
Use correct annotation syntax. r=me

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8c9ddf22abe
Disable test on stylo. r=me

Comment 22

[Emilio Cobos Álvarez (:emilio)]

That is https://github.com/rust-lang-nursery/fmt-rfcs/issues/38, and the style I suggested is how rustfmt formats it currently.

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/86b793bcbcd0
https://hg.mozilla.org/mozilla-central/rev/6bfd9f6f99c3
https://hg.mozilla.org/mozilla-central/rev/c8c9ddf22abe