Closed
Bug 1367455
Opened 7 years ago
Closed 7 years ago
Scheduled GCs should avoid interrupting running GCs
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: pbone, Assigned: pbone)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
6.00 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Whiteboard: [qf]
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p3]
Assignee | ||
Comment 1•7 years ago
|
||
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
Attachment #8873299 -
Flags: review?(jcoppeard)
Comment 2•7 years ago
|
||
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
Assignee | ||
Comment 3•7 years ago
|
||
Revised patch after review.
Attachment #8873299 -
Attachment is obsolete: true
Attachment #8873299 -
Flags: review?(jcoppeard)
Attachment #8873713 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 4•7 years ago
|
||
Change the threshold from 0.975 to 0.95 after talking with jonco. 0.95 is less likely to create extra non-incremental collections.
Attachment #8873713 -
Attachment is obsolete: true
Attachment #8873713 -
Flags: review?(jcoppeard)
Attachment #8873804 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Attachment #8873804 -
Flags: review+
Updated•7 years ago
|
Attachment #8873804 -
Flags: review+
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7798f8aff281 Attempt to avoid interrupting currently running GCs. r=jonco
Keywords: checkin-needed
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7798f8aff281
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 7•7 years ago
|
||
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
Comment 8•7 years ago
|
||
Paul, can you investigate the Octane regression in comment 7? Thanks!
Flags: needinfo?(pbone)
Assignee | ||
Comment 9•7 years ago
|
||
No problem Jan, I'm working on this now.
Assignee | ||
Comment 10•7 years ago
|
||
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.
Flags: needinfo?(pbone)
Assignee | ||
Comment 11•7 years ago
|
||
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.
Flags: needinfo?(jdemooij)
Flags: needinfo?(jcoppeard)
Comment 12•7 years ago
|
||
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.
Flags: needinfo?(jdemooij)
Comment 13•7 years ago
|
||
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.
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 14•7 years ago
|
||
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.
Updated•2 years ago
|
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in
before you can comment on or make changes to this bug.
Description
•