Scheduled GCs should avoid interrupting running GCs

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pbone, Assigned: pbone)

Tracking

(Depends on: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p3]
(Assignee)

Comment 1

2 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 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

2 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

2 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

2 years ago
Keywords: checkin-needed
Attachment #8873804 - Flags: review+
Attachment #8873804 - Flags: review+

Comment 5

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7798f8aff281
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Paul, can you investigate the Octane regression in comment 7? Thanks!
Flags: needinfo?(pbone)
(Assignee)

Comment 9

2 years ago
No problem Jan, I'm working on this now.
(Assignee)

Updated

2 years ago
See Also: → bug 1376557
(Assignee)

Comment 10

2 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)

Updated

2 years ago
Depends on: 1376557
(Assignee)

Comment 11

2 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)
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)
(Assignee)

Comment 14

2 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.
(Assignee)

Updated

2 years ago
Depends on: 1386219
You need to log in before you can comment on or make changes to this bug.