1245261 - Use an Atomic to synchronize gcTriggerBytes

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: atomic_gcTriggerBytes-v0.diff

I think Relaxed is fine for this: since we're exclusively raising the ceiling OMT, the only affect of these not propagating to the main thread is that we trigger a GC sooner.

Comment 1

[Jeff Walden [:Waldo]]

Are there really no cases where we modify gcTriggerBytes in one thread, then we observe that modification in another thread and assume that a previous change in the one thread is visible in the another thread?  Your justification here is quite vague about whether gcTriggerBytes observers ever assume any sort of syncing occurs when a gcTriggerBytes write has been observed, and the various gcTriggerBytes hits make me extraordinarily un-confident about the claim that there are no syncing requirements here.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1

gcTriggerBytes is observed on the main thread to check whether a GC needs to be performed for a zone.  It may also be concurrently updated by background sweeping on a helper thread.  No other changes made by the background thread are assumed to be propagated.  If a GC is triggered we wait on the GC lock which synchronises the threads.  Therefore I think this is ok.

Terrence, did this come up due to the TSAN tests you're running?  If so it would be interesting to see the output.

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Jon Coppeard (:jonco) from comment #2)
> It may also be concurrently updated by background
> sweeping on a helper thread.

I don't think this happens anymore. We seem to call FinalizeArenas with KEEP_ARENAS while background sweeping [1], which recycles them instead and doesn't update gcTriggerBytes_.

[1] https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/js/src/jsgc.cpp#3009

Comment 4

[Terrence Cole [:terrence]]

Attachment: tsan_gcTriggerBytes.txt

Here is the TSAN report. If this does not jibe with how you expect things to work then we should figure out why!

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

Oh, duh, it just happens one level higher up now (still on the helper thread) - there's even a comment about it in FinalizeTypedArenas(). In GCRuntime::sweepBackgroundThings(), after the main loop, we take the lock and release all the arenas in ReleaseArenaList().

It's perhaps worth noting that the backgroundFinalizeState will be BFS_DONE at this point, so we won't take the lock while partially full Arenas are available - but these empty arenas that we're freeing here aren't available to for use anyway, and if we need to allocate a new Arena we'll take the lock first.

Orthogonal to this bug, but I don't really understand the logic at [1] - but then, I don't know much about how the ZoneHeapThreshold works.

Anyway, the worst that Relaxed memory ordering should be able to do here is make us do a Zone (Zonal?) GC sooner than we strictly needed to. We should take the lock at that point, updating our view of gcTriggerBytes_ (which is only ever modified while the GC lock is held), so there's no danger of them diverging forever. ReleaseAcquire ordering would synchronize between writes (and reads) to gcTriggerBytes_, which seems unnecessary here.

[1] https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/js/src/jsgc.cpp#1925

Comment 6

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4543906a71a66827ead733c2974a4013df6fd40
Bug 1245261 - Use an atomic to safely access gcTriggerBytes; r=jonco

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4543906a71a