Closed Bug 1457678 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

61 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: petcuandrei, Assigned: emilio)

References

Details

(Whiteboard: [qf])

Attachments

(3 files)

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
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%
Same issue on this video: https://www.youtube.com/watch?v=7Pq-S557XQU with this data https://perfht.ml/2vS2IRg
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
Flags: needinfo?(mconley)
Flags: needinfo?(andreip)
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
Flags: needinfo?(andreip)
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?
Component: Untriaged → CSS Parsing and Computation
Flags: needinfo?(mconley) → needinfo?(emilio)
Product: Firefox → Core
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.
That was surprisingly painless.
Assignee: nobody → emilio
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(emilio)
Summary: Video makes CPU go at 100% and starts the fans → Video makes CPU go at 100% and starts the fans (due to canvas code messing up with the rule tree)
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.)
Attachment #8972952 - Flags: review?(hikezoe) → review+
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 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.
Attachment #8972952 - Flags: review?(xidorn+moz) → review+
(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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/32192f022b84
Make the font canvas code not mess with the rule tree. r=xidorn,hiro
(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.
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/32192f022b84
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(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?
Flags: needinfo?(emilio) → needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
Attachment #8973394 - Flags: review?(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...
Attachment #8973394 - Flags: review?(emilio) → review+
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
I reported the second bug https://bugzilla.mozilla.org/show_bug.cgi?id=1459500
See Also: → 1459500
You need to log in before you can comment on or make changes to this bug.