Remove JSRuntime::needsBarrier

RESOLVED FIXED in Firefox 41



JavaScript Engine
5 years ago
3 years ago


(Reporter: terrence, Assigned: terrence)



Firefox Tracking Flags

(firefox41 fixed)



(3 attachments)



5 years ago
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.
Assignee: general → nobody

Comment 1

3 years ago
Barriers also need to be disabled during major collections. We should switch the runtime needs barriers checks for !isHeapBusy.
Assignee: nobody → terrence

Comment 2

3 years ago

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

3 years ago
Created attachment 8613809 [details] [diff] [review]

This simply removes the volatile from heapState. I did not find anything in the list here (\%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.
Attachment #8613809 - Flags: review?(jcoppeard)

Comment 4

3 years ago
Created attachment 8613810 [details] [diff] [review]

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.
Attachment #8613810 - Flags: review?(jcoppeard)

Comment 5

3 years ago
Created attachment 8613811 [details] [diff] [review]

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.
Attachment #8613811 - Flags: review?(jcoppeard)


3 years ago
Attachment #8613809 - Flags: review?(jcoppeard) → review+


3 years ago
Attachment #8613810 - Flags: review?(jcoppeard) → review+
Comment on attachment 8613811 [details] [diff] [review]

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().
Attachment #8613811 - Flags: review?(jcoppeard) → review+

Comment 7

3 years ago
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.
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.