1546098 - nsRefreshDriver never stops for top-level documents in content processes, burning CPU

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kenny Levinsen :kennylevinsen]

Attachment: 0001-WIP-Stop-nsRefreshDriver.patch

https://bugzilla.mozilla.org/show_bug.cgi?id=1521786 changed the timer stop logic of nsRefreshDriver::Tick.

Before, any driver without observers would stop their timer on the next tick.

After 1521786 was merged, however, the logic changed to stopping observers in the following scenarios:

• Immediately, if we are one of the following are true:
  • Throttled
  • Not a content process
  • Not a top-level document
  • Under test
  • In ASAP mode
• In 4 seconds, if we have a incomplete ready-state and have not yet had a contentful paint.

This leaves top-level document drivers, and drivers on content-processes forever running unless they are explicitly frozen or throttled. This results in a very significant amount of wasted CPU, as the IPC vsync pump is left running.

That a running but empty nsRefreshDriver consumes so much CPU is somewhat of a separate issue.

I have attached a simple fix. It changes the condition so that the 4-second delay is used for incomplete, non-contentful drivers if we are not throttled/under test/in ASAP mode. In all other cases, the timer is stopped immediately.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Yeah, the condition introduced in bug 1521786 doesn't look quite right to me either. Easier fix is probably just an } else { StopTimer(); } at the end, probably...

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Thanks a lot for the report, Kenny!

Comment 3

[Kenny Levinsen :kennylevinsen]

} else { StopTimer(); } would work, but that leaves the construct as:

if (a) {
   StopTimer();
} else if (b) {
   DelayedStopTimer();
} else {
   StopTimer();
}

That looks a bit silly to me, which is why I instead did if (!a && b) { DelayedStopTimer(); } else { StopTimer(); }, with the exceptions for content processes and top-level documents removed from condition a.

Comment 4

[Kenny Levinsen :kennylevinsen]

Attachment: Bug 1546098 - Ensure that nsRefreshDriver is always stopped when not needed.

nsRefreshDrivers belonging to fully-loaded documents in content processes
now correctly stop their timer when no observers are present.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Yes, totally agreed. It's a much more justifiable patch to uplift to beta though. Anyhow, probably not a huge deal either way.

The patch looks sane to me. I pushed it to the try server here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f4d1e21ce4e9b7d9579d5740fe0d792322cc8a0

But probably Olli or Farre should review it too. We're out of review flags in Bugzilla (patches are supposed to go via Phabricator these days), so needinfo it is :)

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Ah, and as I say it you figure out how to use Phabricator, nice :)

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Here's another try push with the suggested change in Phabricator: https://treeherder.mozilla.org/#/jobs?repo=try&revision=326604f48e3d43a1db6ba2aaee686cab4942fc94

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b7f613b9372
Ensure that nsRefreshDriver is always stopped when not needed. r=emilio

Comment 9

[Oana Pop-Rus]

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

Comment 10

[Pascal Chevrel (relman team) -> :pascalc]

Emilio, since this is a 67 regression, could you request an uplift to beta? Thanks

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9059820 [details]
Bug 1546098 - Ensure that nsRefreshDriver is always stopped when not needed.

Beta/Release Uplift Approval Request

• User impact if declined: More CPU usage when idling
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple patch that goes back to the behavior from before bug 1521786 in more cases.
• String changes made/needed: none

Comment 12

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9059820 [details]
Bug 1546098 - Ensure that nsRefreshDriver is always stopped when not needed.

Low risk patch minimizing cpu usage when idling which is a recent 67 regression, uplift approved for 67 beta 15, thanks.

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/454de582d1d6