Closed Bug 1213977 Opened 4 years ago Closed 3 years ago

Allow incremental GC when keepAtoms is set

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: fitzgen, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

I'm looking at a profile of cnn.com, and around the 19200ms mark, there is a non-incremental GC that takes almost 200ms and whose nonIncrementalReason is "keepAtoms set".

DXR'ing gives me https://dxr.mozilla.org/mozilla-central/rev/b68eab795f9de072bee12821b0f09422e5aa0da9/js/src/jsgc.cpp#6049 and https://dxr.mozilla.org/mozilla-central/rev/b68eab795f9de072bee12821b0f09422e5aa0da9/js/src/vm/Runtime.h#1231-1239 but it still isn't clear to me why we are disallowing incremental GC instead of treating atoms conservatively. It seems to me that it is having a fairly detrimental effect here.
Attached file profile.json.gz
To view the profile:

* Un-gzip it
* Open devtools (cmd+alt+I or ctrl+shift+I on osx and others respectively)
* Select the "performance" tab
* Click the "Import" button on the left
* Select the un-gzipped file
Component: JavaScript Engine → JavaScript: GC
The problem is that the value of keepAtoms can change between incremental slices, and this is a simple heuristic to avoid running into problems.  We can probably do better though.
The telemetry in bug 1314828 is showing that this is a big problem. We should definitely fix it.

Could we could avoid sweeping the atoms table and the atoms compartment if keepAtoms is set? My memory is that the atoms compartment is always swept in the last slice since there is potentially an edge from any compartment to it. We would still null out weak pointers to unmarked atoms from other compartments, but I don't think that would break anything that uses keepAtoms (just parsing I think).
(In reply to Bill McCloskey (:billm) from comment #3)
Yes, I think this would work but it could also prevent us from ever collecting the atoms compartment (until we hit one of our allocation triggers which would also do a non-incremental GC).

What we really want is to fix bug 1205132.
(In reply to Jon Coppeard (:jonco) from comment #4)
> Yes, I think this would work but it could also prevent us from ever
> collecting the atoms compartment (until we hit one of our allocation
> triggers which would also do a non-incremental GC).

Why? We would sweep the atoms as long as keepAtoms isn't set, which I expect wouldn't be very often.

> What we really want is to fix bug 1205132.

Well, that would be good too.
(In reply to [PTO to Dec5] Bill McCloskey (:billm) from comment #5)
> Why? We would sweep the atoms as long as keepAtoms isn't set, which I expect
> wouldn't be very often.

I was thinking that this would happen frequently due to parsing.  But it's not that bad - we don't allow off-main-thread parsing to happen during an incremental GC where we are collecting the atoms zone.  So it's only if incremental slices happen to be triggered during parsing that the GC will observe keepAtoms is set.  Also, if an atoms zone GC is triggered while we have keepAtoms set then we defer it (this was a problem when allocating a lot of atoms during parsing).  So, I think we are good on this front.
Attached patch remove-aka-checkSplinter Review
AutoKeepAtoms is used while we have unrooted atom pointers live on the stack.   JSRuntime::keepAtoms() returns true while one of these is live or if off-main-thread parsing is happening.

In this situation we don't have information about which atoms should be rooted so if GC starts in this state we don't collect the atoms zone.

Currently we also abort the incremental GC if we start a slice in this state.  However I can't see why we need to do this.  No off main thread parsing is allowed to start if we are currently doing a GC of the atoms zone.  Parsing on the main thread can allocate new atoms but these will be marked under the existing rules of incremental collection.

I tried taking this out and try is looking green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=277388eec7eb738f503fc1bc3a2c9ba4b8d6c4e9&group_state=expanded

Can you think of a reason this is not safe?
Attachment #8816152 - Flags: review?(sphink)
Comment on attachment 8816152 [details] [diff] [review]
remove-aka-check

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

This certainly sounds sound to me.
Attachment #8816152 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40c8129cffbf
Don't reset an ongoing incremental GC if AutoKeepAtoms is set r=sfink
https://hg.mozilla.org/mozilla-central/rev/40c8129cffbf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1322420
Assignee: nobody → jcoppeard
You need to log in before you can comment on or make changes to this bug.