Closed Bug 1245261 Opened 7 years ago Closed 7 years ago

Use an Atomic to synchronize gcTriggerBytes

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Attachment #8714993 - Flags: review?(jcoppeard)
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.
(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.
Attachment #8714993 - Flags: review?(jcoppeard) → review+
(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
Here is the TSAN report. If this does not jibe with how you expect things to work then we should figure out why!
Flags: needinfo?(emanuel.hoogeveen)
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
Flags: needinfo?(emanuel.hoogeveen)
https://hg.mozilla.org/mozilla-central/rev/a4543906a71a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.