Closed Bug 1435214 Opened 3 years ago Closed 3 years ago

Make stylesheet insertions that have @keyframes and @font-face rules optimizable

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Blocks: 1420423
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().
Attachment #8947769 - Flags: review?(hikezoe) → 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?)
Attachment #8947768 - Flags: review?(xidorn+moz) → 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.
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
Attachment #8947768 - Flags: review- → review?(xidorn+moz)
(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.
Flags: needinfo?(hikezoe)
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 :)
(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
(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.
Flags: needinfo?(hikezoe)
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).
Attachment #8947768 - Flags: review?(xidorn+moz) → 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.
Attachment #8947769 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4fef754d2761
Optimize @keyframes rule insertions. r=xidorn,hiro
https://hg.mozilla.org/mozilla-central/rev/4fef754d2761
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.