1213977 - Allow incremental GC when keepAtoms is set

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: 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

Comment 2

[Jon Coppeard (:jonco)]

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 4

[Jon Coppeard (:jonco)]

(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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 6

[Jon Coppeard (:jonco)]

(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.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: remove-aka-check

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?

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8816152 [details] [diff] [review]
remove-aka-check

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

This certainly sounds sound to me.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40c8129cffbf
Don't reset an ongoing incremental GC if AutoKeepAtoms is set r=sfink

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40c8129cffbf