Closed Bug 1548358 Opened 7 months ago Closed 7 months ago

Consider to finish forget skippable phase sooner if there is idle time to use

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Right now we may end up calling CCRunnerFired quite a few times before deciding to stop it or call CC - meaning there are quite some wake-ups even if the browser is otherwise rather idle.
I was thinking that maybe we could finish sooner, just try to find some slice which would do some work. Or the recursive call will just kill the CCRunner if there isn't anything to do.
Note, the earlier 'else if' still forces the most important forgetSkippable calls and such.

Calling the method recursively is just the easiest way to implement the approach.
The return value of the method is true, if some work was done.

(still away from my main machine, so moz-phab doesn't work. Misusing data-review)

Attachment #9061992 - Flags: data-review?(continuation)
Comment on attachment 9061992 [details] [diff] [review]
finish_forget_skippable_period_sooner.diff

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

It looks like the max stack depth here is numEarlyTimerFires.

This looks pretty confusing. What is it really doing? It seems like you end up just recursing until sCCRunnerFireCount is high enough that isLateTimerFire becomes true?
Attachment #9061992 - Flags: data-review?(continuation) → review-
Duplicate of this bug: 1548359

yes. That is the whole point. Ending up to late timer fires, and skipping some of the earlier possible forget skippables.

I thought the comment was quite clear.
Any ideas how you'd improve this?

I'm not worried about max recursion depth here.

Flags: needinfo?(continuation)

And to clarify still, isLateTimerFire is when interesting stuff happens. Either we decide to stop, or do still another run which starts CC. I'm not sure how to make the comment better, and calling the method recursively is rather clean way to achieve the behavior.

Blocks: 1547950

I still don't really understand what this is doing. Isn't this the same as just setting sCCRunnerFireCount to numEarlyTimerFires + 1? The only other conditions I see involve sPreviousSuspectedCount and sCleanupsSinceLastGC, and those aren't going to change when we recurse, or the passage of time, and I assume those aren't intended to fire either. I would think that if the CC isn't doing much of anything because there's nothing to do, we'd be better off delaying the next check, rather than doing more work immediately.

Flags: needinfo?(continuation)

Setting sCCRunnerFireCount to sCCRunnerFireCount + 1 and calling the stuff again, until we end up to the
late fires where we actually do something.
And once we're at the late firing case, we call ShouldTriggerCC(), which may lead to still another forget skippable, and then possibly stop the forget skippable cycle, or continue to CC.

The issue is that we have too many CCRunnerFired calls which don't do anything (return false). That causes tons of wake ups.
If we do have idle time, better to just try to run CC if possible, or stop.

And I did verify that we often do end up running
if (ShouldTriggerCC(nsCycleCollector_suspectedCount())) {
... this
}

since there can be enough suspected objects to trigger CC. After that there would another asynchronous
call to CCRunnerFired to possibly trigger CC.
But we wouldn't have so many CCRunnerFired calls which don't do anything.

Comment on attachment 9061992 [details] [diff] [review]
finish_forget_skippable_period_sooner.diff

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

I guess we can try this, though I still worry we'll end up doing more CC work. Hopefully telemetry will let us know if this regresses it.

Can you change it to set sCCRunnerFireCount to numEarlyTimerFires so it is clear that what this is really doing is skipping ahead past all of the early timer fires?
Attachment #9061992 - Flags: review- → review+

CYCLE_COLLECTOR_TIME_BETWEEN should regress if the patch behaves badly.

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c868e3f1c12
Consider to finish forget skippable phase sooner if there is idle time to use, r=mccr8
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

== Change summary for alert #20880 (as of Thu, 09 May 2019 11:49:18 GMT) ==

Improvements:

7% raptor-tp6-docs-firefox loadtime linux64-shippable opt 1,035.48 -> 965.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20880

You need to log in before you can comment on or make changes to this bug.