Closed
Bug 1313098
Opened 6 years ago
Closed 6 years ago
Ensure thread safe access to runtime's heap state
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(3 files)
42.21 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
912 bytes,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
888 bytes,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
Currently this has accessors that may be called off the main thread. We should assert that this is only ever accessed in a safe way and fix the problems that show up.
Assignee | ||
Comment 1•6 years ago
|
||
Patch to ensure thread safe access to the runtime's heap state. This means only written to on the main thread and only read on the main thread or from a thread performing GC on behalf of the main thread (with the main thread running collector code at this point and not running the mutator). I broadened the scope of our per-thread state that previously told us whether a thread was sweeping to be whether the thread is running any GC code. I could have used more fine-grained state to differentiate this but I didn't think it was worth it. There were a couple of places where we did access the heap state from the wrong thread: - barrier code where we're looking at e.g. a permanent atom belonging to another runtime I merged ArenaCellIterUnderFinalize and ArenaCellIterUnderGC and added ArenaCellIter. These assert they're called correctly. I removed a heap state check from write barriers in a couple of places. This is not needed if we check the needsIncrementalBarrier flag on the zone, which will always be false if we're on the main thread and currently inside a GC. Next up I guess we need to check that we have access to the zone.
Attachment #8804792 -
Flags: review?(sphink)
Assignee | ||
Comment 2•6 years ago
|
||
Patch to stop triggering a barrier in finalisation. This us allows us to assert that we're not collecting in IsIncrementalBarrierNeededOnTenuredGCThing.
Attachment #8804793 -
Flags: review?(continuation)
Updated•6 years ago
|
Attachment #8804793 -
Flags: review?(continuation) → review+
Comment 3•6 years ago
|
||
Comment on attachment 8804792 [details] [diff] [review] bug1313098-assert-heap-state-accesses Review of attachment 8804792 [details] [diff] [review]: ----------------------------------------------------------------- Wow, nice! It's making more sense now. ::: js/public/HeapAPI.h @@ +380,5 @@ > MOZ_ASSERT(thing); > MOZ_ASSERT(!js::gc::IsInsideNursery(thing.asCell())); > + > + // TODO: I'd like to assert !isHeapBusy() here but this gets called > + // while we are tracing the heap, e.g. during memory reporting. Can you put a bug number here? ::: js/src/gc/Allocator.cpp @@ +293,5 @@ > MOZ_ASSERT(!cx->runtime()->isHeapBusy(), "allocating while under GC"); > > AutoMaybeStartBackgroundAllocation maybeStartBGAlloc; > > + return arenas->allocateFromArena(zone, thingKind, true, maybeStartBGAlloc); Hm. The bool parameter is kind of mysterious here. Please either (1) change it to a class enum (CheckThresholds, NoCheckThresholds), or (2) move it to the end of the parameter list and default it to true, then don't pass it here and the other place where it's true, but comment the case where you pass in false. @@ +429,5 @@ > Arena* arena = chunk->allocateArena(rt, zone, thingKind, lock); > zone->usage.addGCArena(); > > // Trigger an incremental slice if needed. > + if (checkThresholds) It is nice to not repeat the whole !rt->isHeapMinorCollecting() && !isHeapCompacting()
Attachment #8804792 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 4•6 years ago
|
||
These patches are causing assertion failures on try because JitcodeGlobalTable::lookupForSamplerInfallible gets called on a random thread and asserts based on heap state. I also had to add the per-thread sweeping state back in because I realised it wasn't subsumed by the collecting state: the background sweep task operates in parallel with the mutator.
Assignee | ||
Comment 6•6 years ago
|
||
We need to manually avoid that barrier in one more place.
Attachment #8807546 -
Flags: review?(continuation)
Comment 7•6 years ago
|
||
Comment on attachment 8807546 [details] [diff] [review] bug1313098-dont-expose-in-move-hook Review of attachment 8807546 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsJSNPRuntime.cpp @@ +1742,5 @@ > JS::AutoSuppressGCAnalysis nogc; > > auto entry = > static_cast<NPObjWrapperHashEntry*>(sNPObjWrappers->Search(npobj)); > MOZ_ASSERT(entry && entry->mJSObj); I guess a boolean check already doesn't trigger the barrier? @@ +1743,5 @@ > > auto entry = > static_cast<NPObjWrapperHashEntry*>(sNPObjWrappers->Search(npobj)); > MOZ_ASSERT(entry && entry->mJSObj); > + MOZ_ASSERT(entry->mJSObj.unbarrieredGetPtr() == old); Is there no overload for operator== to handle this without a barrier? I thought some cases were covered.
Attachment #8807546 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #7) > I guess a boolean check already doesn't trigger the barrier? We have operator bool() that doesn't trigger the read barrier. > Is there no overload for operator== to handle this without a barrier? I > thought some cases were covered. Sadly not. We're working on this in bug 1308236. Partly this is because |old| is |const JSObject*| here.
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f246bb6108c Assert we only access heap state on the main thread r=sfink r=mccr8
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0f246bb6108c
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•