Closed Bug 1297769 Opened 4 years ago Closed 3 years ago

Rewrite GCRuntime::refillFreeListOffMainThread to not use helper thread condition variables to wait for the end of GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(2 files, 1 obsolete file)

Following bug 1288579, GCRuntime::refillFreeListOffMainThread should be changed to use a better way of waiting for the GC to finish.
Even better, we could work out which data structures this accesses that are also accessed by the GC and protect them.
That should be the zone's freeLists[kind], its arenaLists[kind], and possibly the runtime-wide chunk list. I don't think we have to worry about the free list, since it gets cleared at the start of GC (I always forget how allocation between slices works though). Maybe we should have a per-zone arena list lock, and a chunk list lock separate from the general GC lock.
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)
Right, but in this case we don't have to worry about locking the first two because the thread has exclusive use of the zone.  And the chunk list is already protected by use of the GC lock.  So maybe we don't need to wait for the GC to finish here at all?
Attached patch bug1297769-remove-gc-wait (obsolete) — Splinter Review
Rather than waiting for the GC to finish, assert that the current zone is not being collected and allocate.

A first try run was as green as can be expected.  Here's a larger one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7b84d010497051fe49a7da39c81d53b71e7ad52
Assignee: nobody → jcoppeard
A larger try run had a few timeouts but not more than normal: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7b84d010497051fe49a7da39c81d53b71e7ad52

What do you think?
Attachment #8800190 - Flags: review?(sphink)
Attachment #8799394 - Attachment is obsolete: true
Attachment #8800190 - Attachment is patch: true
Comment on attachment 8800190 [details] [diff] [review]
bug1297769-remove-gc-wait v2

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

Ok, I'll confess that I've never fully understood our thread management, and it's about time that I should.

I wish there were more comments. For example, what are the rules around accessing heapState_? It's not atomic, it is only sometimes protected with AutoLockHelperThreadState, and at least in assertions it seems to be read at all kinds of random times. AutoTraceSession sets it, but sometimes without locking, and the decision of whether to lock depends on an unlocked nonatomic access of Runtime.numExclusiveThreads. Which appears to be ok, since AutoTraceSession seems to be used on the runtime's main thread only. AutoEnterCycleCollection also sets it, but unconditionally uses AutoLockHelperThreadState.

None of this seems wrong to me necessarily, just that it's nonobvious. It would be a lot easier to figure out what's going on if heapState_ were commented with its thread access rules.

backgroundFinalizeState[n] is read, but it's a sequentially consistent atomic, so that seems fine.

arenaLists[n] is read, and it's nonatomic, but it is protected by the GC lock if needed according to BFS[n] above, so apparently the background finalize thread is the only other non-GC thing that could be mucking with it? A comment above arenaLists about their thread access policy would be nice.

::: js/src/gc/Allocator.cpp
@@ +300,5 @@
>  /* static */ TenuredCell*
>  GCRuntime::refillFreeListOffMainThread(ExclusiveContext* cx, AllocKind thingKind)
>  {
> +    // A GC may be happening on the main thread, but zones used by exclusive
> +    // contexts are never collected.

And if a non-GC heap traversal is happening, I guess it doesn't matter because we're only writing the free list here, and a traversal won't read that.

What about AdoptArenas? Say you have two exclusive contexts, one of them parsing and the other allocating, and the parsing thread finishes and the main thread calls AdoptArenas for it, while the allocating thread lands in refillFreeListOffMainThread. I guess it doesn't really have anything to do with this patch, but just for my information, how do they keep from racing on arenaLists[n]?
Attachment #8800190 - Flags: review?(sphink) → review+
Oh, sorry, I had a followup question. After this patch, is AutoLockHelperThreadState still needed in AutoTraceSession and AutoEnterCycleCollection?
(In reply to Steve Fink [:sfink] [:s:] from comment #6)
> For example, what are the rules around
> accessing heapState_? It's not atomic, it is only sometimes protected with
> AutoLockHelperThreadState

As I undertstand it, it should only be accessed under the helper thread state lock (but we can skip this if there are no exclusive threads present).

> and at least in assertions it seems to be read at all kinds of random times.

Yes, this is bad and I raised bug 1307498 for it.

> It would be a lot easier to figure out what's going on if heapState_ were
> commented with its thread access rules.

I'll figure out if the current locking situation is still required (re coment 7) and if so comment, otherwise I'll post a patch to remove it.

> arenaLists[n] is read, and it's nonatomic, but it is protected by the GC
> lock if needed according to BFS[n] above, so apparently the background
> finalize thread is the only other non-GC thing that could be mucking with
> it? A comment above arenaLists about their thread access policy would be
> nice.

Yes this is non-obvious and is an optimisation so that we don't have to lock unless necessary.  I don't know whether this is still required these days now we have generational GC.  Maybe we should remove it and see if it makes any difference.

> ::: js/src/gc/Allocator.cpp
> @@ +300,5 @@
> >  /* static */ TenuredCell*
> >  GCRuntime::refillFreeListOffMainThread(ExclusiveContext* cx, AllocKind thingKind)
> >  {
> > +    // A GC may be happening on the main thread, but zones used by exclusive
> > +    // contexts are never collected.
> 
> And if a non-GC heap traversal is happening, I guess it doesn't matter
> because we're only writing the free list here, and a traversal won't read
> that.

I hadn't really considered that, but ideally these exclusive zones would not be visible via the API.  I can't see why we'd ever want to expose them, except for debugging purposes (and I mean e.g. js::DumpHeap, not the debugger).

> What about AdoptArenas? Say you have two exclusive contexts, one of them
> parsing and the other allocating, and the parsing thread finishes and the
> main thread calls AdoptArenas for it, while the allocating thread lands in
> refillFreeListOffMainThread. I guess it doesn't really have anything to do
> with this patch, but just for my information, how do they keep from racing
> on arenaLists[n]?

The two exclusive contexts will have different zones so they will not interfere with each other.
(In reply to Steve Fink [:sfink] [:s:] from comment #7)
> Oh, sorry, I had a followup question. After this patch, is
> AutoLockHelperThreadState still needed in AutoTraceSession and
> AutoEnterCycleCollection?

No, all access to heap state will happen on the main thread so no locking is required.  I'll land bug 1313098 first though to ensure that.
(In reply to Jon Coppeard (:jonco) from comment #9)
> I'll land bug 1313098 first though

Oh wait, that totally doesn't work because we need to remove this loop that waits for the heap state to be idle.  I'll land them both together.
As per comments above, updated patch that removes this waiting and also removes locking while updating the heap state now it's no longer needed.
Attachment #8804794 - Flags: review?(sphink)
Comment on attachment 8804794 [details] [diff] [review]
bug1297769-remove-gc-wait v3

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

\o/ It's nice to clear that up.

::: js/src/jsgc.cpp
@@ +5563,5 @@
>  {
>      MOZ_ASSERT(prevState == JS::HeapState::Idle);
>      MOZ_ASSERT(heapState != JS::HeapState::Idle);
>      MOZ_ASSERT_IF(heapState == JS::HeapState::MajorCollecting, rt->gc.nursery.isEmpty());
> +    rt->setHeapState(heapState);

This does make me wonder why AutoTraceSession takes a JSRuntime* instead of a JSContext* if it has to be main thread. But looking at it, I didn't see a good way to change it.
Attachment #8804794 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2497845a72a1
Don't wait for GC to finish when allocating off-main-thread r=sfink
https://hg.mozilla.org/mozilla-central/rev/2497845a72a1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.