Closed Bug 1367455 Opened 7 years ago Closed 7 years ago

Scheduled GCs should avoid interrupting running GCs

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Performance Impact low
Tracking Status
firefox55 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p3]
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 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.
Attachment #8873299 - Attachment is obsolete: true
Attachment #8873299 - Flags: review?(jcoppeard)
Attachment #8873713 - Flags: review?(jcoppeard)
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+
Keywords: checkin-needed
Attachment #8873804 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/7798f8aff281
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Paul, can you investigate the Octane regression in comment 7? Thanks!
Flags: needinfo?(pbone)
No problem Jan, I'm working on this now.
See Also: → 1376557
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)
Depends on: 1376557
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)
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)
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)
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.
Depends on: 1386219
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in before you can comment on or make changes to this bug.