When a collection is triggered due to scheduling try to avoid interrupting another zone's collection (which will loose some progress) by increasing the scheduling thresholds in this case.
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Hi Jon, I don't know what impact this has, if any, do you think it should be committed now or if we should wait until we can benchmark it and see if it makes a difference? Here are the try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c3947a7f0b2eb2be6cc3ff687d6537fecfee9b1
Comment on attachment 8873299 [details] [diff] [review] Attempt to avoid interrupting currently running GCs Review of attachment 8873299 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/GCRuntime.h @@ +138,4 @@ > > /* Fraction of threshold.gcBytes() which triggers an incremental GC. */ > UnprotectedData<double> zoneAllocThresholdFactor_; > + /* The same except when doing so would interrupt an already running GC. */ nit: whitespace at end of line. ::: js/src/jsgc.cpp @@ +3011,5 @@ > + size_t igcThresholdBytes; > + double zoneAllocThresholdFactor; > + > + wouldInterruptCollection = isBackgroundSweeping() || > + ( isIncrementalGCInProgress() && zone->isCollecting() ); This seems to be the same version of the patch you sent yesterday. We don't need the isBackgroundSweeping() check because isIncrementalGCInProgress() will return true in this case. We need to check !zone->isCollecting() because that's the case that can cause the GC reset. @@ +3014,5 @@ > + wouldInterruptCollection = isBackgroundSweeping() || > + ( isIncrementalGCInProgress() && zone->isCollecting() ); > + zoneAllocThresholdFactor = ( wouldInterruptCollection ? > + tunables.zoneAllocThresholdFactorAvoidInterrupt() : > + tunables.zoneAllocThresholdFactor() ); nit: remove parentheses around this expression
Revised patch after review.
Change the threshold from 0.975 to 0.95 after talking with jonco. 0.95 is less likely to create extra non-incremental collections.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/7798f8aff281 Attempt to avoid interrupting currently running GCs. r=jonco
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Just fyi, there was a regression on AWFY Octane splay latency, probably because of this patch: https://treeherder.mozilla.org/perf.html#/alerts?id=7016 https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0ed2ebbb0b73f2b241318a17e9774da28277975c&tochange=a8bfe68a333f2e3afea9c161f7cd00185f8da83b
Paul, can you investigate the Octane regression in comment 7? Thanks!
No problem Jan, I'm working on this now.
The regression here is that a GC slice goes overbudget, this occurs in a GC that was delayed (by this theshold) after (but I'm not sure if it interrupts) a GC that had a delayed finalisation slice see bug 1376557. So I've filed that new bug to attempt to fix this regression there. I don't know why this slice goes overbudget, but hopefully by fixing the delayed finalization slice we fix this regression.
Today I learnt that the regression introduced here was already in beta (I thought it was only in central) and will go into release on Wednesday (I thought it would go into Beta). So it's now too late to backout this change for Firefox 55. However we could back it out for 56, or I can keep working on my fix in bug 1376557 and ask for it to be applied to either beta (fixing it in 56) or central (in 57). What should we do? I'm very sorry for the confusion, I must have missed the merge date back in June.
Did this also regress things in the browser, or is it shell only? It would be good to get this fixed in 57 at least, one way or another.
Let's back this out in central right away and request an uplift for beta. When bug 1376557 is fixed we can land the whole fix together. Sorry, I should have kept an eye on this too.
There's a nice way to back this out. It's a one line change to one of the hard-coded GCScheduleTUnables. I'll write and test it in the shell now.
You need to log in before you can comment on or make changes to this bug.