887030 - Remove JSRuntime::needsBarrier

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

We use JSRuntime::needsBarrier to disable pre-barriers during minor GC's. We have to do this because the templatization of HashKeyRef makes it impossible to cast out the barriers. It would be more performant, although more code, to specialize each of the HashKeyRef users for its own type so that they cannot fire pre barriers. This would allow us to add an assertion to the per-barriers that we are not fired during a minor GC and eliminate one branch in the opt pre-barrier.

Comment 1

[Terrence Cole [:terrence]]

Barriers also need to be disabled during major collections. We should switch the runtime needs barriers checks for !isHeapBusy.

Comment 2

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aef5701bf4a

The series passes all jsapi, jit, and jstests locally; and as importantly, fails all pre-barrier verifier tests when the heap-state condition is reversed.

Comment 3

[Terrence Cole [:terrence]]

Attachment: 1_remove_volatile_from_heapState-v0.diff

This simply removes the volatile from heapState. I did not find anything in the list here (https://bugzilla.mozilla.org/buglist.cgi?quicksearch=TSAN\%3A&list_id=12272064) that applies directly to heapState, so I am cautiously optimistic that we don't actually need any additional guards on it. Also, the next patch in the series has the side-effect of requiring off-thread readers to at least use runtimeFromAnyThread, so I think that also largely mitigates any risk from this change.

Comment 4

[Terrence Cole [:terrence]]

Attachment: 2_move_heapState_to_shadow-v0.diff

This patch moves heapState from GCRuntime to JS::shadow::Runtime. Conceptually, it would be better to have the heapState in the GC's heap state management block, but we're currently only set up for inlining runtime fields. Not a huge deal; certainly not any worse than what we're already doing with needsIncrementalBarrier.

Comment 5

[Terrence Cole [:terrence]]

Attachment: 3_remove_rtNeedsIncrementalBarrier-v0.diff

And remove JSRuntime::needsIncrementalBarrier. This turns out to be a huge win across the board: conceptually, we now very clearly only fire barriers only when HeapState == Idle; heapState is set exclusively by AutoTraceSession and read via JSRuntime; and we save ~50 lines of very confusing code.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8613811 [details] [diff] [review]
3_remove_rtNeedsIncrementalBarrier-v0.diff

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

::: js/src/jspubtd.h
@@ +163,5 @@
>        , gcStoreBufferPtr_(nullptr)
>      {}
>  
>      bool needsIncrementalBarrier() const {
> +        return heapState_ == JS::HeapState::Idle;

Do we need barriers to fire if we are tracing too?  e.g. this will return false in callbacks run from JS_IterateCompartments().

Comment 7

[Terrence Cole [:terrence]]

It seems the sole user of our iteration routines currently is about:memory, which I expect wants us to perturb the heap minimally. I've added a note to JS_IterateCompartments to this effect. In any case, try seems to be quite green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aef5701bf4an

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc178fdf1427
https://hg.mozilla.org/integration/mozilla-inbound/rev/557399d18235
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8695c1f052c

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc178fdf1427
https://hg.mozilla.org/mozilla-central/rev/557399d18235
https://hg.mozilla.org/mozilla-central/rev/d8695c1f052c