Emilio Cobos Álvarez (:emilio) Assignee
	Description • 7 years ago

.

	Hiroyuki Ikezoe (:hiro)
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8947769 [details]
Bug 1435214: Optimize @keyframes rule insertions.

https://reviewboard.mozilla.org/r/217486/#review223280

Looks pretty good!  One thing I am wondering is that we can clear the hash table when we really need to reduild whole style data?  I mean RebuildAllStyleData().

	Xidorn Quan [:xidorn] UTC+11
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8947768 [details]
style: Make insertion of @font-face rules not necessarily restyle the whole document.

https://reviewboard.mozilla.org/r/217484/#review223286

Thinking a bit more about it, this is actually not correct.

Firstly, if we do not restyle the document, how would we kick off the font downloading if there is any matching in the document? IIRC, font is loaded lazily when characters are actually requested.

In addition, when a font becomes listed in `@font-face` rule, we usually need to draw invisible text instead of going to fallback directly (the `font-display: block` behavior by default), and if we do not restyle, we would not be able to do so. That would also make the page rendering more racy (font rendering would further depend on when the stylesheet is loaded).

So I think we probably cannot take this optimization, and we should have test against it (maybe it already fails some test?)

	Xidorn Quan [:xidorn] UTC+11
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8947769 [details]
Bug 1435214: Optimize @keyframes rule insertions.

https://reviewboard.mozilla.org/r/217486/#review223288

It's getting too late here now... I'll probably review it tomorrow, but definitely not today... some comments first.

::: layout/style/nsAnimationManager.h:350
(Diff revision 1)
>      return false;
>    }
>  
> +  bool AnimationMayBeReferenced(nsAtom* aName) const
> +  {
> +    return mMaybeReferencedAnimations.GetEntry(aName);

Using `Contains` instead should make it clearer here (although internally `Contains` just calls into `GetEntry`).

::: layout/style/nsAnimationManager.h:359
(Diff revision 1)
> +  // This includes names for both animations that are running and @keyframes
> +  // which have not been found, and is used for style invalidation (so it's not
> +  // needed to keep it always up-to-date).

Maybe something like this is clearer:
> This includes all animaton names referenced regardless of whether a corresponding `@keyframes` rule is available.
>
> It may contain names which is no longer referenced, but it should always contain names which is currently referenced, so that it is usable for style invalidation.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 6 • 7 years ago

Comment on attachment 8947768 [details]
style: Make insertion of @font-face rules not necessarily restyle the whole document.

Per IRC discussion, I think this patch is right.

Font loads are triggered from layout, and[1] is what makes sure that we reflow the page when @font-face rules are added (and recascade).

There's no need to do a full doc restyle, and there'd be no need to recascade if there wasn't any Ex / Ch unit, but that's another matter.

[1]: https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/layout/base/nsPresContext.cpp#2357

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 7 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8947769 [details]
> Bug 1435214: Optimize @keyframes rule insertions.
> 
> https://reviewboard.mozilla.org/r/217486/#review223280
> 
> Looks pretty good!  One thing I am wondering is that we can clear the hash
> table when we really need to reduild whole style data?  I mean
> RebuildAllStyleData().

Does RebuildAllStyleData (with a non-zero restyle-hint) guarantee that we'll rebuild all the animations? If not I doubt I can do this.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 10 • 7 years ago
mozreview-review-reply

Comment on attachment 8947769 [details]
Bug 1435214: Optimize @keyframes rule insertions.

https://reviewboard.mozilla.org/r/217486/#review223288

> Using `Contains` instead should make it clearer here (although internally `Contains` just calls into `GetEntry`).

Done, agreed :)

> Maybe something like this is clearer:
> > This includes all animaton names referenced regardless of whether a corresponding `@keyframes` rule is available.
> >
> > It may contain names which is no longer referenced, but it should always contain names which is currently referenced, so that it is usable for style invalidation.

I like it! I added that comment :)

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 11 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8947769 [details]
> Bug 1435214: Optimize @keyframes rule insertions.
> 
> https://reviewboard.mozilla.org/r/217486/#review223280
> 
> Looks pretty good!  One thing I am wondering is that we can clear the hash
> table when we really need to reduild whole style data?  I mean
> RebuildAllStyleData().

We can probably do something like:

diff --git a/layout/base/ServoRestyleManager.cpp b/layout/base/ServoRestyleManager.cpp
index fab5f4d12d09..6d846781f9af 100644
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -1136,6 +1136,15 @@ ServoRestyleManager::DoProcessPendingRestyles(ServoTraversalFlags aFlags)
   }
 
   if (mRestyleForCSSRuleChanges) {
+    styleSet->UpdateStylist();
+    nsINode* restyleRoot = mDocument->GetServoRestyleRoot();
+    if (root == mDocument || root == mDocument->GetRootElement()) {
+      // We're going to restyle the whole document and all the animations will
+      // be rebuilt, so now we can clear the animation manager's record of which
+      // animations are referenced.
+      presContext->AnimationManager()->ClearMaybeReferencedAnimations();
+    }
+
     aFlags |= ServoTraversalFlags::ForCSSRuleChanges;
   }

But it's not clear to me it's worth

	Hiroyuki Ikezoe (:hiro)
	Comment 12 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > Comment on attachment 8947769 [details]
> > Bug 1435214: Optimize @keyframes rule insertions.
> > 
> > https://reviewboard.mozilla.org/r/217486/#review223280
> > 
> > Looks pretty good!  One thing I am wondering is that we can clear the hash
> > table when we really need to reduild whole style data?  I mean
> > RebuildAllStyleData().
> 
> Does RebuildAllStyleData (with a non-zero restyle-hint) guarantee that we'll
> rebuild all the animations? If not I doubt I can do this.

We try to rebuild all animations (i.e. UpdateAnimations is called) when we meet TraversalFlags::ForCSSRuleChanges during traversal, IIRC.

	Xidorn Quan [:xidorn] UTC+11
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8947768 [details]
style: Make insertion of @font-face rules not necessarily restyle the whole document.

https://reviewboard.mozilla.org/r/217484/#review223512

OK... So we don't need to restyle, because we don't store any actual reference to font faces in the computed style. We need a reflow to kick off the necessary download, which will be triggered by nsPresContext::UserFontSetUpdated (which will also trigger a restyle anyway).

	Xidorn Quan [:xidorn] UTC+11
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8947769 [details]
Bug 1435214: Optimize @keyframes rule insertions.

https://reviewboard.mozilla.org/r/217486/#review223514

Looks good, thanks.

::: layout/style/nsAnimationManager.h:363
(Diff revision 3)
> +  // corresponding `@keyframes` rule is available.
> +  //
> +  // It may contain names which are no longer referenced, but it should always
> +  // contain names which are currently referenced, so that it is usable for
> +  // style invalidation.
> +  nsTHashtable<nsRefPtrHashKey<nsAtom>> mMaybeReferencedAnimations;

Hmmm, so it looks like we actually never clear this set in the lifetime of the document... which is probably fine.

	Pulsebot
	Comment 15 • 7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4fef754d2761
Optimize @keyframes rule insertions. r=xidorn,hiro

	Narcis Beleuzu [:NarcisB]
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4fef754d2761