1457678 - Video makes CPU go at 100% and starts the fans (due to canvas code messing up with the rule tree)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[petcuandrei]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180427102245

Steps to reproduce:

I visited this page https://www.youtube.com/watch?v=nvUuC7GgqeY


Actual results:

One of the Firefox processes just went wild. HTOP on GNU/Linux reports the content process having above 100% CPU usage (I think this means it uses a core at 100% and some other cored).


Expected results:

The CPU should not go this high with one tab, no addons and a pretty fresh profile.

This is quite recent. I use Nightly as my main browser and this never happened before.

More info here https://perf-html.io/from-addon/calltree/?thread=0&threadOrder=0-2-3-4-1&v=3

Comment 1

[petcuandrei]

I posted the wrong link. I created a new profile and here is the data https://perfht.ml/2vWbJZC

This time the CPU did not go at 100% and was more around 80% and 70%

Comment 2

[petcuandrei]

Same issue on this video: https://www.youtube.com/watch?v=7Pq-S557XQU with this data https://perfht.ml/2vS2IRg

Comment 3

[Daniel_A[:daschilean] [Please contact - [e-mail]]]

I verified this issue on Linux x86_64 with FF Nightly 61.0a1 (2018-05-01) and I cannot reproduce it.
The CPU did not go at 80% or 70% and was around 25%.

Could you please test this issue based on comment 2?

If you still have the issue please create a new profile, you have the steps here:https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager. Thanks

Comment 4

[petcuandrei]

I always test with fresh profiles. I created a new profile. I saw it on this video again https://www.youtube.com/watch?v=nvUuC7GgqeY and here is the new data from the new profile https://perfht.ml/2rbQMFp

This is what htop reports https://screenshots.firefox.com/4f1h8Y5f6bObHwaW/null

Maybe this is related also https://bugzilla.mozilla.org/show_bug.cgi?id=1458526 source https://www.reddit.com/r/firefox/comments/8gglib/very_high_cpu_usage_while_watching_videos_details/dybjzfn/?context=3

Maybe unrelated but my Firefox crashes while recording the second profile https://crash-stats.mozilla.com/report/index/648c7485-f513-43c8-9ad7-21a370180428

Comment 5

[petcuandrei]

Attachment: Screenshot from 2018-05-02 23-00-15.png

Comment 6

[Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos)]

Content Process 3 is spending a bunch of time inside requestAnimationFrame callbacks trying to draw into canvas, it looks like... I see Stylo attempting to resolve some font-related stuff:

https://perfht.ml/2JRNEFJ

Tentatively moving this to CSS computation... Hey emilio, anything actionable here?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Yeah, definitely, this is the icky font canvas code. I even left a comment there for reference a while ago:

  https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/servo/components/style/stylist.rs#1560

Looks like we should fix that.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

That was surprisingly painless.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1457678: Make the font canvas code not mess with the rule tree.

Inserting a lot of rules in the root level is super-inefficient. This fixes it
by not doing it.

It gives the root rule node to the style, but that's fine, since it's useless.
All this code-path is already pretty messy.

This is on top of bug 1458192, but it's trivial to rebase it on top of other
patch instead.

Review commit: https://reviewboard.mozilla.org/r/241518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241518/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8972952 [details]
Bug 1457678: Make the font canvas code not mess with the rule tree.

https://reviewboard.mozilla.org/r/241518/#review247418

It took some time that I recall I did write this code originally. :)

Thanks for the fix!

::: servo/components/style/stylist.rs:1554
(Diff revision 1)
> -        // maybe...
> -        let rule_node = self.rule_tree.insert_ordered_rules(iter::once((
> +            block.declaration_importance_iter().map(|(declaration, importance)| {
> +                debug_assert!(!importance.important());

(I thought we can use normal_declaration_iter() here (and drop the assertion), but this way (with the assertion) might be a better way for the sanity check.)

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Looking at the code, I'm a bit surprised that no compiler complains about shadowing the variable "declarations" in GetFontStyleForServo[1]...

[1] https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/dom/canvas/CanvasRenderingContext2D.cpp#2710-2716

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8972952 [details]
Bug 1457678: Make the font canvas code not mess with the rule tree.

https://reviewboard.mozilla.org/r/241518/#review247426

::: servo/components/style/stylist.rs:1571
(Diff revision 1)
> -        // FIXME(emilio): the pseudo bit looks quite dubious!
> -        properties::cascade::<E>(
> +        // We don't bother inserting these declarations in the rule tree, since
> +        // it'd be quite useless and slow.
> +        properties::apply_declarations::<E, _, _>(
>              &self.device,
>              /* pseudo = */ None,
> -            &rule_node,
> +            self.rule_tree.root(),

I would really prefer you instead change `apply_declarations` to accept an `Option<&StrongRuleNode>` (preferably in a separate patch before this) and pass `None` for it here.

Given that the only place in `apply_declarations` uses this is passing it into `StyleBuilder::new` which accepts `Option<StrongRuleNode>`, I think this change should be trivial.

My concern is that people may forgot that the rule node can be unrelated to the declarations, and try to do something messy.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #12)
> Comment on attachment 8972952 [details]
> Bug 1457678: Make the font canvas code not mess with the rule tree.
> 
> https://reviewboard.mozilla.org/r/241518/#review247426
> 
> ::: servo/components/style/stylist.rs:1571
> (Diff revision 1)
> > -        // FIXME(emilio): the pseudo bit looks quite dubious!
> > -        properties::cascade::<E>(
> > +        // We don't bother inserting these declarations in the rule tree, since
> > +        // it'd be quite useless and slow.
> > +        properties::apply_declarations::<E, _, _>(
> >              &self.device,
> >              /* pseudo = */ None,
> > -            &rule_node,
> > +            self.rule_tree.root(),
> 
> I would really prefer you instead change `apply_declarations` to accept an
> `Option<&StrongRuleNode>` (preferably in a separate patch before this) and
> pass `None` for it here.
> 
> Given that the only place in `apply_declarations` uses this is passing it
> into `StyleBuilder::new` which accepts `Option<StrongRuleNode>`, I think
> this change should be trivial.
> 
> My concern is that people may forgot that the rule node can be unrelated to
> the declarations, and try to do something messy.

I'd argue the opposite. Generally we don't want people messing around with the rule tree this way... And we depend on the correct rules for styles for elements and such. This is the exception rather than the rule.

Happy to change it if you think it's worth though.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32192f022b84
Make the font canvas code not mess with the rule tree. r=xidorn,hiro

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #13)
> I'd argue the opposite. Generally we don't want people messing around with
> the rule tree this way... And we depend on the correct rules for styles for
> elements and such. This is the exception rather than the rule.
> 
> Happy to change it if you think it's worth though.

I don't get your point.

We don't want people to mess around the tree, but I don't see there is anything stop people from trying to do that. There isn't any comment or document warning people that don't touch the rule node passed in.

If we don't want them to do that wrong, we probably should have a comment explaining that.

Also, I don't see why passing in a wrong rule node may make any sense even if we don't want people to mess it up. Do you mean that people would eventually figure out they are messing things up via tests because they would get a wrong rule node in this case? I believe it would be much easier for them to find that out when they try to figure out why rule node is optional at the very beginning.

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/32192f022b84

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> (In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05)
> from comment #13)
> > I'd argue the opposite. Generally we don't want people messing around with
> > the rule tree this way... And we depend on the correct rules for styles for
> > elements and such. This is the exception rather than the rule.
> > 
> > Happy to change it if you think it's worth though.
> 
> I don't get your point.
> 
> We don't want people to mess around the tree, but I don't see there is
> anything stop people from trying to do that. There isn't any comment or
> document warning people that don't touch the rule node passed in.
> 
> If we don't want them to do that wrong, we probably should have a comment
> explaining that.
> 
> Also, I don't see why passing in a wrong rule node may make any sense even
> if we don't want people to mess it up. Do you mean that people would
> eventually figure out they are messing things up via tests because they
> would get a wrong rule node in this case? I believe it would be much easier
> for them to find that out when they try to figure out why rule node is
> optional at the very beginning.

My point is that the only style where the rule node is wrong is this canvas font style, which isn't handed over anywhere, and where using the rules is not really a thing.

We should probably just document the ResolveForDeclarations stuff, instead of saying that other consumers can't rely on it (because they can, do, and should, probably).

Would you be fine with that?

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Attachment: followup - Don't pass wrong rule node into apply_declarations.

I still don't see why it is a bad idea to pass in a None rather than a wrong rule node given this change is so trivial and help ensuring stuff is semantically more correct.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973394 [details] [diff] [review]
followup - Don't pass wrong rule node into apply_declarations.

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

So the servo caller relies on the wrong rule node, and I don't see why having None vs. the root rule node is semantically more correct in any way.

The root rule node is "no rules", so it means effectively the same. We only support None in the ComputedValues because of text / anon box styles, where it was kind of annoying to get the stylist just for that.

I guess we should fix that really, since all the rule nodes have a pointer to the root... Anyway this is fine if you really prefer it, but I don't see why this is an improvement necessarily.

::: servo/components/style/animation.rs
@@ +478,5 @@
>              // as existing browsers don't appear to animate visited styles.
>              let computed = properties::apply_declarations::<E, _, _>(
>                  context.stylist.device(),
>                  /* pseudo = */ None,
> +                Some(previous_style.rules()),

This is also still kinda wrong, then, since you're adding declarations here. But we do rely on this keeping the old rules...

Comment 20

[petcuandrei]

Latest build still has a high CPU issue. Content 3 had some big spikes but also the main thread https://perfht.ml/2rpkO7E same video https://www.youtube.com/watch?v=nvUuC7GgqeY

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Could you open another bug for that?

That profile looks completely different, with 30%+ time spent in painting in your GL driver. Let's keep this bug for the font canvas issue.

Comment 22

[petcuandrei]

I reported the second bug https://bugzilla.mozilla.org/show_bug.cgi?id=1459500