Closed Bug 1363805 Opened 7 years ago Closed 7 years ago

Only flush styles on nsComputedDOMStyle::UpdateCurrentStyleSources if we know that the child or one of its ancestors need a restyle.

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Project Flags:
Performance Impact medium
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: emilio, Assigned: wcpan)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1363805 - Part 1: Add a flag to nsIDocument::FlushPendingNotifications.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363805 - Part 3: Do lazy flushing if possible.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363805 - Part 4: Fix tests that need style flushing.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363805 - Part 5: Add testcase for restyle generation.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1363805 - Part 2: Add a flag to note that a child has LaterSiblings hint.
59 bytes, text/x-review-board-request
heycam
: review+
Details
benchmark.html
6.86 KB, text/html
Details 
Blink does this, and could be a nice perf win.
A few things that come to mind:

* There might be tests that call getComputedStyle on some element, assuming that's sufficient to flush all styles, invalidating the test (but not causing it to fail).  Not sure what the best way to check for that would be -- we maybe want to know whether a test did a getComputedStyle call and threw away the result (which would be the most common pattern).

* Can we do anything for properties that need layout flushed too?  Probably not.

* We'll still need to do the parent document layout flush in nsDocument::FlushPendingNotifications.
Boris, what's the reason here for flushing styles on the document from the pres shell we got passed in rather than on mContent's document?

http://searchfox.org/mozilla-central/rev/d66b9f27d5630a90b2fce4d70d4e9050f43df9b4/layout/style/nsComputedDOMStyle.cpp#761

I guess that means that with:

  <iframe src="somewhere.html"></iframe>
  <script>
    getComputedStyle(document.querySelector("iframe").contentDocument.querySelector("div")).color
  </script>

we'll flush in the other document and not in the inner document.  Why is that what we want to do?
Flags: needinfo?(bzbarsky)
Or, with the actual test I meant to paste:

<!DOCTYPE html>
<script>
function run() {
  var p = document.querySelector("iframe").contentDocument.querySelector("p");
  p.style.color = "green";
  alert(getComputedStyle(p).color);
}
</script>
<iframe srcdoc="<p>Hello</p>" onload="run()"></iframe>
> There might be tests that call getComputedStyle on some element, assuming that's sufficient to flush all styles

True.  Ideally people would be computing the style the actually care to make sure is flushed, but they might be pretty bad at it...

We could try to audit for this or something.  Not sure how feasible that would be.

> * Can we do anything for properties that need layout flushed too?  Probably not.

Probably not...

> what's the reason here for flushing styles on the document from the pres shell we got passed in rather than on mContent's document

Ah, the fun of CSSOM.

So per spec, when you somewindow.getComputedStyle(someelement), that means resolve style for someelement using the CSS rules from _somewindow_'s document.  How inline style interacts with all this is not very clear.

What UAs _actually_ do is also unclear.  For elements which do not have a box, I believe they more or less follow the spec as written.  For elements which do have a box, I think UAs tend to return the styles of that box, which has nothing to do with the spec.  Certainly that's what we do.  Note that for the properties where the resolved value is the used value it's really not clear what the spec's language would even mean; are we supposed to do layout on the one document with CSS rules from the other document or something?

I was pretty sure these issues had all been raised on the spec, but I can't locate them right now.  It's possible they didn't get migrated from the multiple previous issue trackers the spec had (including the www-style mailing list).  Someone should probably get these filed; it would be ideal if this were someone who has the bandwidth to follow up on them, which is not me right now.

In any case, we could flush _both_ documents in cases when they differ, I guess, to fix testcases like comment 3.  But in practice, using a document other than the ownerDocument of the element for getComputedStyle will mostly lead people to grief on the web.  :(
Flags: needinfo?(bzbarsky)
OK.  So perhaps for the purposes of making getComputedStyle flushing skippable, we don't need to try to optimize the case where the window getComputedStyle was called on is different from the window the element came from.  But we should probably file a bug to sort out what we really should be doing in those cases.

Per discussion with wcpan just now, it sounds like this improvement to getComputedStyle is good enough for the use cases that bug 1358495 had in mind for getComputedStyleWithoutFlushing, and then we don't have to find a solution for the "pres shell has disappeared so we really ought to flush to get it back" issue from that bug.  Wei-Cheng, do you want to take this bug?
Flags: needinfo?(wpan)
I'll take a look, as this bug could probably solve bug 1358495 in a saner way.
Flags: needinfo?(wpan)
Assignee: nobody → wpan
Priority: -- → P2
Nominate because this is the proper fix for bug 1358495, which is a [qf:p1]
Whiteboard: [qf]
Inherited from QF bug 1358495.
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [qf] → [qf:p3]
Boris is this bug only applicable if stylo happens or is it a fix for with and without stylo? If it is dependent on stylo and only if stylo is enabled we think it may not be a QF p1.
Flags: needinfo?(bzbarsky)
(In reply to Naveed Ihsanullah [:naveed] from comment #10)
> Boris is this bug only applicable if stylo happens or is it a fix for with
> and without stylo? If it is dependent on stylo and only if stylo is enabled
> we think it may not be a QF p1.

It should be independent of stylo, though I initially filed it with stylo in mind.
Flags: needinfo?(bzbarsky)
Summary: stylo: Only flush styles on nsComputedDOMStyle::UpdateCurrentStyleSources if we know that the child or one of its ancestors need a restyle. → Only flush styles on nsComputedDOMStyle::UpdateCurrentStyleSources if we know that the child or one of its ancestors need a restyle.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> (In reply to Naveed Ihsanullah [:naveed] from comment #10)
> > Boris is this bug only applicable if stylo happens or is it a fix for with
> > and without stylo? If it is dependent on stylo and only if stylo is enabled
> > we think it may not be a QF p1.
> 
> It should be independent of stylo, though I initially filed it with stylo in
> mind.

I disagree with the triage decision based on this.  Putting it back in the queue.  :-)
Whiteboard: [qf:p3] → [qf]
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #12)

> I disagree with the triage decision based on this.  Putting it back in the
> queue.  :-)

Especially as bug 1358495 has been wontfixed due to this being expected to happen soon.
Given independence of this bug and stylo, and given that bug 1358495 was [qf:p1], I'm going to qf:p1 this bug.
Whiteboard: [qf] → [qf:p1]
All tests passed, but I'm not sure why I got some failure in bug 1379267.
I think it should not related to these patches.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cecba1334a1c17bb5a4a78469e4e73c41e0e1a6
Comment on attachment 8889295 [details]
Bug 1363805 - Part 1: Add a flag to nsIDocument::FlushPendingNotifications.

https://reviewboard.mozilla.org/r/160346/#review166126

r=me with this.

::: commit-message-5928d:1
(Diff revision 1)
> +Bug 1363805 - Part 1: Add a flag to nsIDocument::FlushPendingNotification. r?heycam

"FlushPendingNotifications"

::: dom/base/FlushType.h:41
(Diff revision 1)
> +enum class FlushTarget : uint8_t {
> +  Normal = 0,
> +  ParentOnly = 1,

Please document the purpose of the class and the Normal and ParentOnly values.

::: dom/base/FlushType.h:44
(Diff revision 1)
>  };
>  
> +enum class FlushTarget : uint8_t {
> +  Normal = 0,
> +  ParentOnly = 1,
> +  Count

If the Count enum is not used anyway, please remove it.

::: dom/base/nsIDocument.h:1582
(Diff revision 1)
> -  virtual void FlushPendingNotifications(mozilla::FlushType aType) = 0;
> +  virtual void FlushPendingNotifications(mozilla::FlushType aType,
> +                                         mozilla::FlushTarget aTarget) = 0;

I think the second argument should take FlushTarget::Normal as a default value, since that's the case for 99% of FlushPendingNotification calls.  Then this patch can shrink a fair bit.
Attachment #8889295 - Flags: review?(cam) → review+
wcpan and I discussed the second patch yesterday, and we concluded that while it doesn't introduce incorrect behavior for stylo, it doesn't help much, since it will always end up looking at the root element's ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO flag, which means if there is any element in the document with a pending restyle, we'll flush styles.  (And I think for Gecko it will be incorrect, since we don't use the restyle bits in the same way.)

What we want to know is whether any pending restyle could affect the subject element.  The problem (of course) is sibling combinators.  Without those, we can just look up the tree to see if there is a snapshot or an explicitly noted restyle hint on an ancestor.  I don't think there is any state in the DOM we can look at to determine that, since we only know if elements will be invalidated due to sibling combinators when actually expanding the snapshots and propagating those invalidations.

So, I'm wondering if it would make sense to allow nsComputedDOMStyle to call into Rust to expand all snapshots and process invalidations but not actually do any restyling, so that we can then check all ancestors to find if they have any restyle hints?  This is work that would need to be done when performing a proper style flush anyway, so hopefully we could store some state to know that we have done the snapshot expansion / invalidation processing so we can avoid doing it again unless some other snapshots have been taken.

Emilio, do you forsee any problems with this?


For Gecko, we'd need to do something similar, looking at all of the RestyleData in the RestyleTracker, expanding later siblings, and (potentially) processing SomeDescendants hints by traversing descendants.  But this isn't as easily separable from the rest of the restyle code as Servo's is, and we may just want to do something cheaper (and more pessimistic) in Gecko.  Or not do anything, and just do it in Stylo.
Flags: needinfo?(emilio+bugs)
Comment on attachment 8889296 [details]
Bug 1363805 - Part 3: Do lazy flushing if possible.

https://reviewboard.mozilla.org/r/160348/#review166134

::: layout/style/nsComputedDOMStyle.cpp:105
(Diff revision 1)
>  
>    return valueList.forget();
>  }
>  
> +static bool
> +IsContentNeedsRestyle(nsIContent* aContent)

Nit: maybe just "ContentNeedsRestyle"?

::: layout/style/nsComputedDOMStyle.cpp:107
(Diff revision 1)
> +  MOZ_ASSERT(aContent);
> +  nsCOMPtr<nsIContent> node = aContent;

This doesn't need to be an nsCOMPtr, since aContent will hold on to its parent, and thus all its ancestors.  So just use an nsIContent* to avoid the refcount traffic.
Attachment #8889296 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #20)
> What we want to know is whether any pending restyle could affect the subject
> element.  The problem (of course) is sibling combinators.  Without those, we
> can just look up the tree to see if there is a snapshot or an explicitly
> noted restyle hint on an ancestor.  I don't think there is any state in the
> DOM we can look at to determine that, since we only know if elements will be
> invalidated due to sibling combinators when actually expanding the snapshots
> and propagating those invalidations.

Right, that's somewhat annoying...

> So, I'm wondering if it would make sense to allow nsComputedDOMStyle to call
> into Rust to expand all snapshots and process invalidations but not actually
> do any restyling, so that we can then check all ancestors to find if they
> have any restyle hints?  This is work that would need to be done when
> performing a proper style flush anyway, so hopefully we could store some
> state to know that we have done the snapshot expansion / invalidation
> processing so we can avoid doing it again unless some other snapshots have
> been taken.
> 
> Emilio, do you forsee any problems with this?

That sounds fine, off-hand. I'm somewhat worried about having multiple entry paths for the invalidation stuff, because it makes it a little harder to reason about, but assuming it's clear enough, that should do it.

Actually, I wonder if we could just process invalidations on the main thread in the general case...

We used to not be able to afford it because it used to match against a gazillion of complex selectors that was pretty expensive to do, but that should be no longer the case, and we don't use a bloom filter to match selectors after that anyway.

That way we'd avoid these style traversals we do now, and all the snapshot stuff (we could still have the concept of a "snapshot" in the short-term to reuse the code a bit more, but now it can just be an "attribute override", or "state override" on a single element, instead of having all this SnapshotTable stuff).

Of course there would be two other problems I think:

 * What to do when stylesheets have changed. We could probably just update the stylist if needed...
 * We need to ensure all this setup works on servo. I think it wouldn't be extremely hard, and I could help with that.

Perhaps making this change would be better as a followup to this bug instead of a requirement? I don't know, wdyt, cam?
Flags: needinfo?(emilio+bugs) → needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #19)
> Comment on attachment 8889295 [details]
> ::: dom/base/nsIDocument.h:1582
> (Diff revision 1)
> > -  virtual void FlushPendingNotifications(mozilla::FlushType aType) = 0;
> > +  virtual void FlushPendingNotifications(mozilla::FlushType aType,
> > +                                         mozilla::FlushTarget aTarget) = 0;
> 
> I think the second argument should take FlushTarget::Normal as a default
> value, since that's the case for 99% of FlushPendingNotification calls. 
> Then this patch can shrink a fair bit.

I was trying to not do so because default parameters are determined by static type of the pointer, so all derived classes should have the same default value.
But since a) nsDocument is the only derived class, and b) it seems that we do not care about that so much (by searching virtual.*=.* in the code-base), I guess it's OK.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> Actually, I wonder if we could just process invalidations on the main thread
> in the general case...

Oh, I didn't actually realize that we process the invalidations during the traversal, and not up front.

> [...]
> Perhaps making this change would be better as a followup to this bug instead
> of a requirement? I don't know, wdyt, cam?

It seems nice that we can process the invalidations during traversal, and not have to do it on the main thread.  That should be noticeably faster, shouldn't it?

But yes I think changing how we process invalidations in general should be out of scope for this bug.

Emilio, how hard would it be to use the traversal machinery to not restyle but only to process invalidations?  A new restyle hint perhaps?  Would you have time to write the Servo-side patch for exposing an API that allows us to process all invalidations in the document, which Wei-Cheng can then use in his patches here?


To answer a question I posed myself earlier:

(In reply to Cameron McCormack (:heycam) from comment #20)
> For Gecko, we'd need to do something similar, looking at all of the
> RestyleData in the RestyleTracker, expanding later siblings, and
> (potentially) processing SomeDescendants hints by traversing descendants. 
> But this isn't as easily separable from the rest of the restyle code as
> Servo's is, and we may just want to do something cheaper (and more
> pessimistic) in Gecko.  Or not do anything, and just do it in Stylo.

I'm leaning towards not solving this in Gecko and just making it work in Stylo.  Though if it turns out it's a lot of chrome:// documents we need this optimization for, it may be worth writing the Gecko-specific code for 57.
Flags: needinfo?(cam) → needinfo?(emilio+bugs)
(In reply to Cameron McCormack (:heycam) from comment #24)
> Emilio, how hard would it be to use the traversal machinery to not restyle
> but only to process invalidations?  A new restyle hint perhaps?  Would you
> have time to write the Servo-side patch for exposing an API that allows us
> to process all invalidations in the document, which Wei-Cheng can then use
> in his patches here?

No need for the traversal machinery at all, we have all the elements in a hashtable in the C++ side, so it'd be a matter of doing something like:

  for (auto iter = mSnapshots.Iter(); !iter.Done(); iter.Next())
    Servo_ProcessInvalidations(iter.Key(), &mSnapshots);
  ClearSnapshots();

> It seems nice that we can process the invalidations during traversal, and not have to do it on the main thread.  That should be noticeably faster, shouldn't it?

Not necessarily. In particular, when the invalidations end up not generating any hints, we do a whole traversal for nothing, that we could've avoided otherwise... So I guess you could find better and worst cases for both situations.

Anyway, I can write the patch to add a ServoRestyleManager::ProcessAllPendingAttributeAndStateInvalidations() API that would do that, sure. With that we can also measure and profile easily whether doing them on the main thread vs. doing a full traversal is worth it or not.
Flags: needinfo?(emilio+bugs)
Depends on: 1388298
(In reply to Emilio Cobos Álvarez [:emilio] from comment #25)
> No need for the traversal machinery at all, we have all the elements in a
> hashtable in the C++ side, so it'd be a matter of doing something like:
> 
>   for (auto iter = mSnapshots.Iter(); !iter.Done(); iter.Next())
>     Servo_ProcessInvalidations(iter.Key(), &mSnapshots);
>   ClearSnapshots();

Of course, handy!

> > It seems nice that we can process the invalidations during traversal, and not have to do it on the main thread.  That should be noticeably faster, shouldn't it?
> 
> Not necessarily. In particular, when the invalidations end up not generating
> any hints, we do a whole traversal for nothing, that we could've avoided
> otherwise... So I guess you could find better and worst cases for both
> situations.

Yeah, fair enough.

> Anyway, I can write the patch to add a
> ServoRestyleManager::ProcessAllPendingAttributeAndStateInvalidations() API
> that would do that, sure. With that we can also measure and profile easily
> whether doing them on the main thread vs. doing a full traversal is worth it
> or not.

Thanks!
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Tracking the blocker now.
Keywords: stale-bug
This patch needs the function added in bug 1397168, which is waiting for servo revendoring.
Flags: needinfo?(cam)
Comment on attachment 8889296 [details]
Bug 1363805 - Part 3: Do lazy flushing if possible.

https://reviewboard.mozilla.org/r/160348/#review182200

I think this looks good, but a couple of things:

* It would be good to avoid traversing up the tree looking for restyle bits or Servo restyle state if we know that there are no restyles at all.  So would it work in the Stylo case to check if we have a restyle root first (and if not, we can skip the call to HasPendingRestyleAncestor -- or put that logic inside HasPendingRestyleAncestor), and for the Gecko case, check if GeckoRestyleManager::mPendingRestyles.Count() is non-zero?

* Can you write a couple of microbenchmarks just to ensure that (a) we don't regress performance noticeable when looking up computed style values in a tight loop, and (b) verifying that we do speed up cases where we look up computed styles in a tight loop and also update say style="" on some element that can't affect our one?

::: layout/style/nsComputedDOMStyle.cpp:843
(Diff revision 2)
> +  }
> +  nsCOMPtr<nsIPresShell> shell = aDocument->GetShell();
> +  if (!shell) {
> +    return FlushTarget::Normal;
> +  }
> +  // Unfortunately we don't know if the sheet change effects mContent or not, so

affects

::: layout/style/nsComputedDOMStyle.cpp:856
(Diff revision 2)
> +    // Stylo only works for Element.
> +    if (!mContent->IsElement()) {
> +      return FlushTarget::Normal;
> +    }

I think mContent will only ever be an Element anyway, since it's only initialized in nsComputedDOMStyle::nsComputedDOMStyle with an Element.  So you can drop this check.

::: layout/style/nsComputedDOMStyle.cpp:860
(Diff revision 2)
> +
> +    // Stylo only works for Element.
> +    if (!mContent->IsElement()) {
> +      return FlushTarget::Normal;
> +    }
> +    if (restyleManager->HasPendingRestyleAncestor(mContent->AsElement())) {

Can you add a comment in this function saying why we need to check different things in Gecko and Stylo?

::: layout/style/nsComputedDOMStyle.cpp:996
(Diff revision 2)
>      // No need to re-get the generation, even though GetStyleContext
>      // will flush, since we flushed style at the top of this function.

Please update this comment to say why we don't need to check it when we only flushed the parent.

::: layout/style/nsComputedDOMStyle.cpp:998
(Diff revision 2)
> +    if (target == FlushTarget::Normal) {
> -    NS_ASSERTION(mPresShell &&
> +      NS_ASSERTION(mPresShell &&
> -                 currentGeneration ==
> +                   currentGeneration ==
> -                   mPresShell->GetPresContext()->GetUndisplayedRestyleGeneration(),
> +                     mPresShell->GetPresContext()->GetUndisplayedRestyleGeneration(),

Probably better to put the condition in the assertion:

  NS_ASSERTION(target == FlushTarget::Parent ||
               (mPresShell ...
Attachment #8889296 - Flags: review?(cam)
I did some microbenchmarks, in general this does not regress or improve that much, because most of time we will clear mStyleContext, so this only works for very specific case.

I'll see if I can safely get an style context without restyling.
Flags: needinfo?(cam)
So if I can get the style context from the mContent (for now there are some test cases fail for transitions), with this benchmark (e1 and e2 are located in different subtree, class hidden will change the display to none):

      let cs = getComputedStyle(e1);
      console.time('test');
      for (let i = 0; i < 1000 * 1000; ++i) {
        cs.color;
      }
      console.timeEnd('test');
      console.time('test');
      for (let i = 0; i < 1000 * 1000; ++i) {
        e2.classList.toggle('hidden');
        cs.color;
      }
      console.timeEnd('test');

m-c (stylo):
1080ms
34771ms

m-c (gecko):
1060ms
24542ms

with patches (stylo):
1180ms
3222ms

If there is nothing to restyle at all, it will slow about 10%, but if there is any irrelative subtree needs restyle, it will improve a lot.
In general I think optimizing only the case where the object returned by getComputedStyle is reused is not worth it, we should be able to avoid the flush all the time.
WIP: trying to get the style context from the element. So far this has at least two problems:

1. Sometimes I invalidated elements at a wrong state (e.g.: some elements does not have snapshot in servo but I request the invalidation in gecko)
2. There is no restyle root and dirty bits (include animation ones) during transition.

So I probably shouldn't use DoGetStyleContextNoFlush for the style context.
Hmm ... tried ResolveStyleLazily but got similar result.
Did the same benchmark again: (both based on a5f163da8a9be5d2e86138c57d59be69723b5457)

m-c (first run)
test: 1165.23ms
test: 34131.85ms
m-c (second run)
test: 1146.72ms
test: 35337.05ms

after patch (first run)
test: 1140.5ms
test: 3309.1ms
after patch (second run)
test: 1155.18ms
test: 3358.99ms

Not sure why m-c looks worse this time.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e7eca0bde2e46c918cdd43fde895f593a3d7e7

Weird that layout/style/test/test_bug534804.html failed on try's non-stylo build, but it passed on my local build ...
Flags: needinfo?(cam)
Comment on attachment 8907455 [details]
Bug 1363805 - Part 2: Add a flag to note that a child has LaterSiblings hint.

https://reviewboard.mozilla.org/r/179140/#review184808

::: dom/base/Element.h:137
(Diff revision 1)
> +  // Set if a child element has later-sibling restyle hint (only used in Gecko).
> +  ELEMENT_HAS_POTENTIAL_DIRTY_DESCENDANTS_FOR_COMPUTING = ELEMENT_FLAG_BIT(5),

Can you mention in this comment that we use this bit to help implement an optimization in nsComputedDOMStyle to decide whether we need to flush styles?

I think more straightforward name might be better, too.  How about ELEMENT_HAS_CHILD_WITH_LATER_SIBLINGS_HINT?

::: dom/base/Element.h:155
(Diff revision 1)
>    // ELEMENT_FLAG_BIT(5) is currently unused
>  

You can remove this comment.

::: layout/base/RestyleTracker.h:360
(Diff revision 1)
>        }
>      }
>    }
>  
> +  // If we need to restyle later siblings, we will need a flag on parent to note
> +  // that some descendents need restyle for nsComputedDOMStyle.

s/descendants/children/

::: layout/base/RestyleTracker.cpp:317
(Diff revision 1)
>            // Unset the restyle bits now, so if they get readded later as we
>            // process we won't clobber that adding of the bit.
>            element->UnsetFlags(RestyleBit() |
>                                RootBit() |
> -                              ConditionalDescendantsBit());
> +                              ConditionalDescendantsBit() |
> +                              ELEMENT_HAS_POTENTIAL_DIRTY_DESCENDANTS_FOR_COMPUTING);

I don't think we need to clear this here -- clearing it once we expand the LaterSiblings hints on the children should be enough.

::: layout/base/RestyleTracker.cpp:415
(Diff revision 1)
> +    nsIContent* parent = aElement->GetFlattenedTreeParent();
> +    if (parent && parent->IsElement()) {
> +      parent->UnsetFlags(ELEMENT_HAS_POTENTIAL_DIRTY_DESCENDANTS_FOR_COMPUTING);
> +    }

I don't think this is the right place to do this.  When we are in here, it means that the processing of some restyles caused a new LaterSiblings hint to be added back to the table.

I think instead we want to clear the bit on the parent after we expand LaterSiblings for a given element, i.e. in the loop over laterSiblingArr.

If we do get back in here, with a re-added LaterSiblings hint, it doesn't matter, since we won't have a chance to call getComputedStyle before we finish processing all the pending restyles.
Attachment #8907455 - Flags: review?(cam) → review-
benchmark again the latest m-c:

m-c
no_change: 1084.48ms
toggle_sibling_class: 34803.71ms
no_change: 1088.8ms
toggle_sibling_class: 36132.31ms
no_change: 1087.85ms
toggle_sibling_class: 35288.81ms

patch
no_change: 1150.59ms
toggle_sibling_class: 3022.67ms
no_change: 1159.13ms
toggle_sibling_class: 3028.33ms
no_change: 1144.55ms
toggle_sibling_class: 3138.45ms

So the patch increased 6% in the best case.
Comment on attachment 8889296 [details]
Bug 1363805 - Part 3: Do lazy flushing if possible.

https://reviewboard.mozilla.org/r/160348/#review184810

::: commit-message-d54f3:3
(Diff revision 5)
> +Bug 1363805 - Part 3: Do lazy flushing if possible. r?heycam
> +
> +Skips flushing current document if the style cache can be safely reused.

I don't think the "style cache" is the right thing to mention here.  We are skipping flushing the current document if we know that the target of the getComputedStyle cannot be affected by any pending restyles.

::: dom/animation/EffectCompositor.cpp:572
(Diff revision 5)
> +bool
> +EffectCompositor::HasPendingStyleUpdatesFor(Element* aElement) const

We don't actually seem to call this function from anywhere?

::: layout/base/ServoRestyleManager.cpp:1200
(Diff revision 5)
> +    // Servo data for the element might be reseted. (e.g. by removing
> +    // from its document)

s/be reseted/have been dropped/

::: layout/style/nsComputedDOMStyle.h:718
(Diff revision 5)
>    void BoxValuesToString(nsAString& aString,
>                           const nsTArray<nsStyleCoord>& aBoxValues);
>    void BasicShapeRadiiToString(nsAString& aCssText,
>                                 const nsStyleCorners& aCorners);
>  
> +  mozilla::FlushTarget GetFlushTarget(const nsIDocument* aDocument) const;

Please add a comment to describe what this function is doing.

::: layout/style/nsComputedDOMStyle.cpp:105
(Diff revision 5)
> +static bool
> +ContentNeedsRestyle(nsIContent* aContent)

Please add a comment here pointing out that this function returns whether there is any pending restyle for the element or any of its ancestors.  (Or maybe rename the function, since currently it sounds like it is just checking the element itself.)

::: layout/style/nsComputedDOMStyle.cpp:111
(Diff revision 5)
> +ContentNeedsRestyle(nsIContent* aContent)
> +{
> +  MOZ_ASSERT(aContent);
> +  nsIContent* node = aContent;
> +  while (node) {
> +    if (node->HasFlag(ELEMENT_ALL_RESTYLE_FLAGS | ELEMENT_HAS_POTENTIAL_DIRTY_DESCENDANTS_FOR_COMPUTING)) {

Please wrap this so we keep within 80 columns.

Can you also please add a comment explaining how checking these bits is the right thing to do for Gecko and Servo?

::: layout/style/nsComputedDOMStyle.cpp:127
(Diff revision 5)
> +  nsIPresShell* shell = aDocument->GetShell();
> +  if (!shell) {
> +    return true;
> +  }
> +  // Unfortunately we don't know if the sheet change affects mContent or not, so
> +  // just assume it will and need to flush normally.

s/and need/and that we need/

::: layout/style/nsComputedDOMStyle.cpp:134
(Diff revision 5)
> +  if (context->EffectCompositor()->HasPendingStyleUpdates()) {
> +    return true;
> +  }

I guess this is where you should be calling HasPendingStyleUpdatesForElement.

::: layout/style/nsComputedDOMStyle.cpp:138
(Diff revision 5)
> +    // For Servo, we need to process the restyle-hint-invalidations first, to
> +    // keep the restyle hint up-to-date.

Please explain here that the reason we need to do this is because without this call, the LaterSiblings hints won't be expanded yet, so we wouldn't be able to just look at whether our ancestors need restyling.

::: layout/style/nsComputedDOMStyle.cpp:872
(Diff revision 5)
>  }
>  
> +FlushTarget
> +nsComputedDOMStyle::GetFlushTarget(const nsIDocument* aDocument) const
> +{
> +  // Disconnected elements should not rely on cached style context.

I don't know why we're talking about cached style contexts in here.  Whether we need to flush the document isn't related to whether we have a cached mStyleContext, right?  So what's the real reason we need to flush the document if the element isn't in a document?

::: layout/style/nsComputedDOMStyle.cpp:876
(Diff revision 5)
> +  // If mContent is not in the same document, it is hard to cache the right
> +  // value, so just flush normally.
> +  nsIDocument* contentDocument = mContent->GetOwnerDocument();
> +  if (contentDocument != aDocument) {
> +    return FlushTarget::Normal;
> +  }

I don't quite understand this case either.  Can you give an example?

::: layout/style/nsComputedDOMStyle.cpp:889
(Diff revision 5)
> +    if (DocumentNeedsRestyle(parentDocument, element)) {
> +      return FlushTarget::Normal;
> +    }
> +    contentDocument = parentDocument;

If we need to take parent document iframe size changes into account, then really we need to check whether there is a reflow or a restyle pending.  Although my testing shows that we don't immediately respond to iframe size changes for @media query results anyway.  So it might just be that the only thing we care about is whether the iframe became display:none or stopped being display:none...

::: layout/style/nsComputedDOMStyle.cpp:906
(Diff revision 5)
> +  // If we need a layout flush, we must recompute the property.
> +  FlushTarget target = aNeedsLayoutFlush ? FlushTarget::Normal : GetFlushTarget(document);

"If the property we are computing relies on layout, then we must flush."

::: layout/style/nsComputedDOMStyle.cpp:1026
(Diff revision 5)
>        return;
>      }
>  
>      // No need to re-get the generation, even though GetStyleContext
>      // will flush, since we flushed style at the top of this function.
> -    NS_ASSERTION(mPresShell &&
> +    // We don't need this assertion if we only flushed the parent.

s/this assertion/to check this/
Attachment #8889296 - Flags: review?(cam)
Comment on attachment 8906995 [details]
Bug 1363805 - Part 4: Fix tests that need style flushing.

https://reviewboard.mozilla.org/r/178750/#review184832
Attachment #8906995 - Flags: review?(cam) → review+
Comment on attachment 8906996 [details]
Bug 1363805 - Part 5: Add testcase for restyle generation.

https://reviewboard.mozilla.org/r/178752/#review184834

::: layout/style/test/test_computed_style_no_flush.html:32
(Diff revision 2)
> +container.classList.toggle('black');
> +cs.backgroundColor;
> +isnot(utils.restyleGeneration, currentRestyleGeneration,
> +      "Should have restyled something");
> +

Also you should check that if you toggle a class name on foo, for example, that getting computed style on container doesn't need a flush.

Also please add a test with one of these later siblings cases.
Attachment #8906996 - Flags: review?(cam) → review+
benchmark for Gecko:

gecko m-c
no_change: 1090.5ms
toggle_sibling_class: 18814.47ms
no_change: 1083.85ms
toggle_sibling_class: 18854.28ms
no_change: 1090.65ms
toggle_sibling_class: 18947.4ms

gecko patch
no_change: 1082.12ms
toggle_sibling_class: 4363.3ms
no_change: 1081.58ms
toggle_sibling_class: 4336.5ms
no_change: 1087.54ms
toggle_sibling_class: 4349.98ms
Looks like the hotspot is ServoRestyleManager::ProcessAllPendingAttributeAndStateInvalidations.
If I remove the profile marker, and test mSnapshots.Count() first (to avoid iterator creation), the numbers are pretty close to m-c:

no_change: 1095.05ms
toggle_sibling_class: 3188.93ms
no_change: 1098ms
toggle_sibling_class: 3177.56ms
no_change: 1096.64ms
toggle_sibling_class: 3194.62ms

It regressed about 0.9%~1%.
Thanks for testing.  I think those numbers look OK, then.  Can you attach the benchmarks for reference?
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #55)
> ::: layout/style/nsComputedDOMStyle.cpp:872
> (Diff revision 5)
> >  }
> >  
> > +FlushTarget
> > +nsComputedDOMStyle::GetFlushTarget(const nsIDocument* aDocument) const
> > +{
> > +  // Disconnected elements should not rely on cached style context.
> 
> I don't know why we're talking about cached style contexts in here.  Whether
> we need to flush the document isn't related to whether we have a cached
> mStyleContext, right?  So what's the real reason we need to flush the
> document if the element isn't in a document?

Yes, I forgot to update the comment ...

> ::: layout/style/nsComputedDOMStyle.cpp:876
> (Diff revision 5)
> > +  // If mContent is not in the same document, it is hard to cache the right
> > +  // value, so just flush normally.
> > +  nsIDocument* contentDocument = mContent->GetOwnerDocument();
> > +  if (contentDocument != aDocument) {
> > +    return FlushTarget::Normal;
> > +  }
> 
> I don't quite understand this case either.  Can you give an example?
> 
As per comment 4, it's possible to do something like this:

getComputedStyle(iframe.contentDocumet.body)

In this case, I think mContent's document is not the one nsComputedDOMStyle is using.
Attached file benchmark.html — 
Just a randomly chosen real webpage. Call measure() will do the benchmark.
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #61)
> As per comment 4, it's possible to do something like this:
> 
> getComputedStyle(iframe.contentDocumet.body)
> 
> In this case, I think mContent's document is not the one nsComputedDOMStyle
> is using.

OK.  So in theory we could do some checks on the current window's document, to see if for example we have style sheets to flush, and ignore pending restyles in this document since they won't affect that other document.  But we're not bothering to try to optimize this case.  Is that right?  If so, please mention something like "we could check blah blah, but we don't bother" in the comment.
This bug won't be fixed by 57. I am changing it from qf:p1 to qf:p2 for post 57 work.
Whiteboard: [qf:p1] → [qf:p2]
Comment on attachment 8907455 [details]
Bug 1363805 - Part 2: Add a flag to note that a child has LaterSiblings hint.

https://reviewboard.mozilla.org/r/179140/#review185848
Attachment #8907455 - Flags: review?(cam) → review+
Comment on attachment 8889296 [details]
Bug 1363805 - Part 3: Do lazy flushing if possible.

https://reviewboard.mozilla.org/r/160348/#review185850

Looks good, thanks!

::: layout/base/ServoRestyleManager.cpp:1199
(Diff revisions 5 - 6)
>  }
>  
>  void
>  ServoRestyleManager::ProcessAllPendingAttributeAndStateInvalidations()
>  {
> -  AutoTimelineMarker marker(mPresContext->GetDocShell(),
> +  if (mSnapshots.Count() == 0) {

Nit: mSnapshots.IsEmpty()

::: layout/style/nsComputedDOMStyle.cpp:115
(Diff revisions 5 - 6)
> +    if (node->HasFlag(ELEMENT_ALL_RESTYLE_FLAGS |
> +                      ELEMENT_HAS_CHILD_WITH_LATER_SIBLINGS_HINT)) {

Ah, this new flag is not part of ALL_RESTYLE_FLAGS, which reminds me: let's clear this flag in nsINode::UnsetRestyleFlagsIfGecko too.  (I'm not too sure whether we should put this flag in ELEMENT_ALL_RESTYLE_FLAGS.  Probably not...)

::: layout/style/nsComputedDOMStyle.cpp:150
(Diff revisions 5 - 6)
>      // Then if there is a restyle root, we check if the root is an ancestor of
>      // this content. If it is not, then we don't need to restyle immediately.
>      // Note this is different from Gecko: we only check if any ancestor needs
>      // to restyle _itself_, not descendants, since dirty descendants can be
>      // another subtree.

(It occurs to me know that we could have done something simliar for Servo, where we have a bit on the parent that indicates whether any child has a LaterSiblings hint.  But I think now that we have this snapshot expansion code written, and that it will give us more accurate results, it's better to keep it this way.)

::: layout/style/nsComputedDOMStyle.cpp:879
(Diff revisions 5 - 6)
> -  // Disconnected elements should not rely on cached style context.
> -  if (!mContent->IsInComposedDoc()) {
> -    return FlushTarget::Normal;
> -  }
> +  // If mContent is not in the same document, we could do some checks to know if
> +  // there are some pending restyles can be ignored across documents, but it
> +  // can be complicated and should be edge case, so we just don't bother to do
> +  // the optimization here.

Small wording nit:

// If mContent is not in the same document, we could do some checks to know if
// there are some pending restyles that can be ignored across documents
// (since we will use the caller document's style), but it can be compliated
// and should be an edge case, so we just don't bother to do the optimization
// in this case.

::: layout/style/nsComputedDOMStyle.cpp:890
(Diff revisions 5 - 6)
>    // If parent document is there, also needs to check if there are something
> -  // changed that needs to flush this document (e.g. size change for iframe)
> +  // changed that needs to flush this document (e.g. size change for iframe).

Nit: "if there is some change that needs to flush this document".
Attachment #8889296 - Flags: review?(cam) → review+
I think these patches look good now, thanks!

Since this is a pretty big change to how getComputedStyle works, and given how we close to the Firefox 57 merge (the Nightly code freeze should start today), I think we should risk manage this by waiting until after the merge to land it, and target it for Firefox 58.
Agreed. We will need a milestone to stabilize.
Thanks for your help!
Comment on attachment 8889296 [details]
Bug 1363805 - Part 3: Do lazy flushing if possible.

https://reviewboard.mozilla.org/r/160348/#review185888

Just drive-by-commenting, feel free to ignore.

::: layout/base/ServoRestyleManager.cpp:1199
(Diff revision 7)
>  }
>  
>  void
>  ServoRestyleManager::ProcessAllPendingAttributeAndStateInvalidations()
>  {
> -  AutoTimelineMarker marker(mPresContext->GetDocShell(),
> +  if (mSnapshots.IsEmpty()) {

Why the early return? It's going to be cheap if it's empty anyway.

::: layout/style/nsComputedDOMStyle.cpp:119
(Diff revision 7)
> +    // hint.
> +    if (node->HasFlag(ELEMENT_ALL_RESTYLE_FLAGS |
> +                      ELEMENT_HAS_CHILD_WITH_LATER_SIBLINGS_HINT)) {
> +      return true;
> +    }
> +    node = node->GetParent();

I _think_ this needs to use the flattened tree to be completely correct.

::: layout/style/nsComputedDOMStyle.cpp:885
(Diff revision 7)
> +  // there are some pending restyles can be ignored across documents (since we
> +  // will use the caller document's style), but it can be complicated and should
> +  // be an edge case, so we just don't bother to do the optimization in this
> +  // case.
> +  nsIDocument* contentDocument = mContent->GetOwnerDocument();
> +  if (contentDocument != aDocument) {

Also, why `GetOwnerDocument` instead of `OwnerDoc()`? You have an `nsIContent` so they're always going to be the same.

Also, this could read nicer (I think) just doing `if (aDocument != mContent->OwnerDoc())`, and using `aDocument` everywhere else.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #78)
> ::: layout/style/nsComputedDOMStyle.cpp:119
> (Diff revision 7)
> > +    // hint.
> > +    if (node->HasFlag(ELEMENT_ALL_RESTYLE_FLAGS |
> > +                      ELEMENT_HAS_CHILD_WITH_LATER_SIBLINGS_HINT)) {
> > +      return true;
> > +    }
> > +    node = node->GetParent();
> 
> I _think_ this needs to use the flattened tree to be completely correct.

Yes, that indeed should be the flattened tree.  (We want to follow the tree that property inheritance follows.)
Comment on attachment 8907455 [details]
Bug 1363805 - Part 2: Add a flag to note that a child has LaterSiblings hint.

https://reviewboard.mozilla.org/r/179140/#review185890

::: dom/base/Element.h:1817
(Diff revision 3)
>  
>  inline void nsINode::UnsetRestyleFlagsIfGecko()
>  {
>    if (IsElement() && !AsElement()->IsStyledByServo()) {
> -    UnsetFlags(ELEMENT_ALL_RESTYLE_FLAGS);
> +    UnsetFlags(ELEMENT_ALL_RESTYLE_FLAGS |
> +               ELEMENT_HAS_CHILD_WITH_LATER_SIBLINGS_HINT);

We also unset these flags in a couple more places, like `BindToTree` and such, have you checked we don't need to do it there?
Comment on attachment 8889296 [details]
Bug 1363805 - Part 3: Do lazy flushing if possible.

https://reviewboard.mozilla.org/r/160348/#review185888

> Why the early return? It's going to be cheap if it's empty anyway.

In a tight loop, perf shows that creating two iterators (one in ClearSnapshots()) will increase 1~2% execution time.

> Also, why `GetOwnerDocument` instead of `OwnerDoc()`? You have an `nsIContent` so they're always going to be the same.
> 
> Also, this could read nicer (I think) just doing `if (aDocument != mContent->OwnerDoc())`, and using `aDocument` everywhere else.

Right, I think OwnerDoc() will be a bit faster than GetOwnerDocument().
Just found a weird bug bug 1401857, which force me to do some strange thing to reset the style sheet state for tests.
Comment on attachment 8907455 [details]
Bug 1363805 - Part 2: Add a flag to note that a child has LaterSiblings hint.

https://reviewboard.mozilla.org/r/179140/#review185890

> We also unset these flags in a couple more places, like `BindToTree` and such, have you checked we don't need to do it there?

This flag will eventually cleared by PostRestyleEvent or frame construction, and it does not participate restyling, so I think it's ok for now.
Disabled the test for gecko due to bug 1401857.
(In reply to Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] from comment #88)
> This flag will eventually cleared by PostRestyleEvent or frame construction,
> and it does not participate restyling, so I think it's ok for now.

But for example we could set this flag, then remove the element from the document before we process restyles.  Then if we re-insert the element into the document later, it will have the bit set when it shouldn't.  It's probably not a big deal, because it just means that a getComputedStyle might be slower, but it also seems easy enough to clear in UnbindFromTree.
Pushed by wpan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ecdb6bd3ebc
Part 1: Add a flag to nsIDocument::FlushPendingNotifications. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2ea24d8301f0
Part 2: Add a flag to note that a child has LaterSiblings hint. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3b0852f8a696
Part 3: Do lazy flushing if possible. r=heycam
https://hg.mozilla.org/integration/autoland/rev/60457d490110
Part 4: Fix tests that need style flushing. r=heycam
https://hg.mozilla.org/integration/autoland/rev/ee619ad512f4
Part 5: Add testcase for restyle generation. r=heycam
(In reply to Cameron McCormack (:heycam) from comment #90)
> But for example we could set this flag, then remove the element from the
> document before we process restyles.  Then if we re-insert the element into
> the document later, it will have the bit set when it shouldn't.  It's
> probably not a big deal, because it just means that a getComputedStyle might
> be slower, but it also seems easy enough to clear in UnbindFromTree.

Oops, I think I clicked too soon. :(

For this case, wouldn't it need to restyle anyway?
The element will have to get styled, but since it won't happen through processing a restyle (instead the frame constructor will generate a style context for it), I don't think the bit will get cleared.
Hmm ... I thought the flag will clear by UnsetRestyleFlagsIfGecko in nsCSSFrameConstructor::ProcessChildren, or you mean we need to clear the flag for the container of removing element? (i.e. this->mParent->UnsetFlags)
Then it's fine. :-)
See Also: → 1404140
Performance Impact: --- → P2
Whiteboard: [qf:p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: