Details

Reviewers

mstange
bas

Commits

rMOZILLACENTRAL6721a4116d17: Bug 1778575, try to run a pending vsync asap if the main thread is otherwise…

Bugzilla Bug ID

1778575

Summary

There are (at least) two different cases leading to lower fps.
Motionmark seems to hit FinishedWaitingForTransaction() case rather often, but the testcase in the bug
hits the other case more often.

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 470682
Build 566433: arc lint + arc unit

Event Timeline

smaug planned changes to this revision.Oct 13 2022, 11:40 AM

smaug created this revision.

Herald added a project: secure-revision. · View Herald TranscriptOct 13 2022, 11:40 AM

Harbormaster completed remote builds in B469235: Diff 634891.Oct 13 2022, 11:40 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 13 2022, 11:40 AM

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

https://treeherder.mozilla.org/jobs?repo=try&revision=b9a381322cd5805e2323ba56c0a50e80fe38638f

Some motionmark tests
https://treeherder.mozilla.org/jobs?repo=try&revision=21b5c43d9cb2bbcbbf815ac55b40c47977220960

smaug planned changes to this revision.Oct 13 2022, 3:23 PM

smaug updated this revision to Diff 634986.

smaug retitled this revision from WIP: Bug 1778575, try to run a pending vsync asap if the main thread is otherwise empty to WIP: Bug 1778575, try to run a pending vsync asap if the main thread is otherwise empty, or if we skipped paints because of pending transactions.

smaug edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B469299: Diff 634986.Oct 13 2022, 3:23 PM

motionmark
https://treeherder.mozilla.org/jobs?repo=try&revision=e58f1b5e8e6633be027f7b27dc069865092d0711

other tests
https://treeherder.mozilla.org/jobs?repo=try&revision=8bea5489dfa4f7c190e8f309252dbc1f9480bf91

Doesn't seem to affect motionmark numbers on CI.

smaug requested review of this revision.Oct 14 2022, 7:48 AM

smaug retitled this revision from WIP: Bug 1778575, try to run a pending vsync asap if the main thread is otherwise empty, or if we skipped paints because of pending transactions to Bug 1778575, try to run a pending vsync asap if the main thread is otherwise empty, or if we skipped paints because of pending transactions.

smaug added inline comments.

layout/base/nsRefreshDriver.cpp
697	The idea here is just to push a normal priority task to the queue, and when it runs, trigger a refreshdriver tick if there aren't other tasks. This seems to be hit quite often. This could be also implemented also in another level, closer to TaskController/Thread, so that in AfterProcessTask callback we'd check if there are other tasks to run. But decided to keep this RefreshDriver scheduling code in one place and the implemented approach seems to work quite well in many cases.
2902	This is effectively backing out bug 1371668, but because DidComposite is now using control priority (and JS shouldn't run at that point), we want to dispatch a new runnable which uses vsync priority. And because of that, it needs to do extra checks whether it still makes sense to trigger tick. CanDoCatchUpTick() happens to have the reasonable checks, so reusing it here. This is after another type of catch-up tick.

Herald added a subscriber: emilio. · View Herald TranscriptOct 14 2022, 7:48 AM

Bas reviewed bug 1371668, so might want to review this :)
Or mstange.

I couldn't see differences in motionmark numbers, but when profiling locally, the behavior was different, less idle time. And I manually verified the extra tick from nsRefreshDriver::FinishedWaitingForTransaction()
is hit rather often.

layout/base/nsRefreshDriver.cpp
2902	Also, the old code, before bug 1371668, didn't use information about the latest missing vsync, but passed wrong /now/ time to the Tick. This is trying to pass more reasonable value.

smaug updated this revision to Diff 635632.Oct 14 2022, 7:54 PM

smaug retitled this revision from Bug 1778575, try to run a pending vsync asap if the main thread is otherwise empty, or if we skipped paints because of pending transactions to Bug 1778575, try to run a pending vsync asap if the main thread is otherwise empty, or if we skipped paints because of pending transactions, r=mstange,bas.

smaug edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B469833: Diff 635632.Oct 14 2022, 7:54 PM

smaug updated this revision to Diff 635640.Oct 14 2022, 8:05 PM

smaug edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B469838: Diff 635640.Oct 14 2022, 8:05 PM

smaug updated this revision to Diff 636331.Oct 18 2022, 10:45 AM

smaug edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B470305: Diff 636331.Oct 18 2022, 10:45 AM

smaug added a child revision: D159583: Bug 1778575, add support for low priority tasks and use such to trigger pending RefreshDriverTimer, r=bas.Oct 18 2022, 10:45 AM

bas accepted this revision.Oct 18 2022, 2:32 PM

This revision is now accepted and ready to land.Oct 18 2022, 2:32 PM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!