diff options
author | Ben Gamari <ben@smart-cactus.org> | 2022-11-14 13:31:06 -0500 |
---|---|---|
committer | Marge Bot <ben+marge-bot@smart-cactus.org> | 2022-12-23 19:09:30 -0500 |
commit | 543cae0084a72ca767a443d857f9e65a5a79f71d (patch) | |
tree | f9c08709e686a4a3db6d3fed87694cad59856296 | |
parent | 387d4fcc29441098d41340c12e56d9693b5e4c94 (diff) | |
download | haskell-543cae0084a72ca767a443d857f9e65a5a79f71d.tar.gz |
nonmoving: Refactor update remembered set initialization
This avoids a lock inversion between the storage manager mutex and
the stable pointer table mutex by not dropping the SM_MUTEX in
nonmovingCollect. This requires quite a bit of rejiggering but it
does seem like a better strategy.
-rw-r--r-- | rts/sm/GC.c | 9 | ||||
-rw-r--r-- | rts/sm/NonMoving.c | 14 | ||||
-rw-r--r-- | rts/sm/NonMovingMark.c | 70 | ||||
-rw-r--r-- | rts/sm/NonMovingMark.h | 5 | ||||
-rw-r--r-- | rts/sm/Storage.c | 2 |
5 files changed, 66 insertions, 34 deletions
diff --git a/rts/sm/GC.c b/rts/sm/GC.c index c91d795756..5bfee440a3 100644 --- a/rts/sm/GC.c +++ b/rts/sm/GC.c @@ -842,11 +842,9 @@ GarbageCollect (uint32_t collect_gen, // Flush the update remembered sets. See Note [Eager update remembered set // flushing] in NonMovingMark.c if (RtsFlags.GcFlags.useNonmoving) { - RELEASE_SM_LOCK; for (n = 0; n < getNumCapabilities(); n++) { - nonmovingAddUpdRemSetBlocks(&getCapability(n)->upd_rem_set.queue); + nonmovingAddUpdRemSetBlocks(&getCapability(n)->upd_rem_set); } - ACQUIRE_SM_LOCK; } // Mark and sweep the oldest generation. @@ -867,8 +865,6 @@ GarbageCollect (uint32_t collect_gen, // old_weak_ptr_list should be empty. ASSERT(oldest_gen->old_weak_ptr_list == NULL); - // we may need to take the lock to allocate mark queue blocks - RELEASE_SM_LOCK; // dead_weak_ptr_list contains weak pointers with dead keys. Those need to // be kept alive because we'll use them in finalizeSchedulers(). Similarly // resurrected_threads are also going to be used in resurrectedThreads() @@ -878,10 +874,9 @@ GarbageCollect (uint32_t collect_gen, #if !defined(THREADED_RTS) // In the non-threaded runtime this is the only time we push to the // upd_rem_set - nonmovingAddUpdRemSetBlocks(&gct->cap->upd_rem_set.queue); + nonmovingAddUpdRemSetBlocks(&gct->cap->upd_rem_set); #endif nonmovingCollect(&dead_weak_ptr_list, &resurrected_threads); - ACQUIRE_SM_LOCK; } // Update the max size of older generations after a major GC: diff --git a/rts/sm/NonMoving.c b/rts/sm/NonMoving.c index 24ab3cbba4..541dfe85c5 100644 --- a/rts/sm/NonMoving.c +++ b/rts/sm/NonMoving.c @@ -899,7 +899,7 @@ static void nonmovingPrepareMark(void) static void nonmovingMarkWeakPtrList(MarkQueue *mark_queue, StgWeak *dead_weak_ptr_list) { for (StgWeak *w = oldest_gen->weak_ptr_list; w; w = w->link) { - markQueuePushClosure_(mark_queue, (StgClosure*)w); + markQueuePushClosureGC(mark_queue, (StgClosure*)w); // Do not mark finalizers and values here, those fields will be marked // in `nonmovingMarkDeadWeaks` (for dead weaks) or // `nonmovingTidyWeaks` (for live weaks) @@ -918,8 +918,14 @@ static void nonmovingMarkWeakPtrList(MarkQueue *mark_queue, StgWeak *dead_weak_p // - So, to be able to traverse `dead_weak_ptr_list` and run finalizers we // need to mark it. for (StgWeak *w = dead_weak_ptr_list; w; w = w->link) { - markQueuePushClosure_(mark_queue, (StgClosure*)w); - nonmovingMarkDeadWeak(mark_queue, w); + markQueuePushClosureGC(mark_queue, (StgClosure*)w); + + // Mark the value and finalizer since they will be needed regardless of + // whether we find the weak is live. + if (w->cfinalizers != &stg_NO_FINALIZER_closure) { + markQueuePushClosureGC(mark_queue, w->value); + } + markQueuePushClosureGC(mark_queue, w->finalizer); } } @@ -960,7 +966,7 @@ void nonmovingCollect(StgWeak **dead_weaks, StgTSO **resurrected_threads) // Mark threads resurrected during moving heap scavenging for (StgTSO *tso = *resurrected_threads; tso != END_TSO_QUEUE; tso = tso->global_link) { - markQueuePushClosure_(mark_queue, (StgClosure*)tso); + markQueuePushClosureGC(mark_queue, (StgClosure*)tso); } trace(TRACE_nonmoving_gc, "Finished marking roots for nonmoving GC"); diff --git a/rts/sm/NonMovingMark.c b/rts/sm/NonMovingMark.c index d10cd018f3..a0a86cdc07 100644 --- a/rts/sm/NonMovingMark.c +++ b/rts/sm/NonMovingMark.c @@ -27,6 +27,8 @@ #include "sm/Storage.h" #include "CNF.h" +static void nonmovingResetUpdRemSetQueue (MarkQueue *rset); +static void nonmovingResetUpdRemSet (UpdRemSet *rset); static bool check_in_nonmoving_heap(StgClosure *p); static void mark_closure (MarkQueue *queue, const StgClosure *p, StgClosure **origin); static void trace_tso (MarkQueue *queue, StgTSO *tso); @@ -261,17 +263,9 @@ static uint32_t markQueueLength(MarkQueue *q); #endif static void init_mark_queue_(MarkQueue *queue); -/* Transfers the given capability's update-remembered set to the global - * remembered set. - * - * Really the argument type should be UpdRemSet* but this would be rather - * inconvenient without polymorphism. - */ -void nonmovingAddUpdRemSetBlocks(MarkQueue *rset) +static void nonmovingAddUpdRemSetBlocks_(MarkQueue *rset) { - if (markQueueIsEmpty(rset)) return; - - // find the tail of the queue + // find the tail of the remembered set mark queue bdescr *start = rset->blocks; bdescr *end = start; while (end->link != NULL) @@ -282,14 +276,45 @@ void nonmovingAddUpdRemSetBlocks(MarkQueue *rset) end->link = upd_rem_set_block_list; upd_rem_set_block_list = start; RELEASE_LOCK(&upd_rem_set_lock); +} + +/* + * Transfers the given capability's update-remembered set to the global + * remembered set. + * + * Really the argument type should be UpdRemSet* but this would be rather + * inconvenient without polymorphism. + */ +static void nonmovingAddUpdRemSetBlocks_lock(MarkQueue *rset) +{ + if (markQueueIsEmpty(rset)) return; - // Reset remembered set + nonmovingAddUpdRemSetBlocks_(rset); + // Reset the state of the remembered set. ACQUIRE_SM_LOCK; init_mark_queue_(rset); rset->is_upd_rem_set = true; RELEASE_SM_LOCK; } +/* + * Transfers the given capability's update-remembered set to the global + * remembered set. + * + * Really the argument type should be UpdRemSet* but this would be rather + * inconvenient without polymorphism. + * + * Caller must hold SM_LOCK. + */ +void nonmovingAddUpdRemSetBlocks(UpdRemSet *rset) +{ + if (markQueueIsEmpty(&rset->queue)) return; + + nonmovingAddUpdRemSetBlocks_(&rset->queue); + init_mark_queue_(&rset->queue); + rset->queue.is_upd_rem_set = true; +} + #if defined(THREADED_RTS) /* Called by capabilities to flush their update remembered sets when * synchronising with the non-moving collector as it transitions from mark to @@ -301,7 +326,7 @@ void nonmovingFlushCapUpdRemSetBlocks(Capability *cap) "Capability %d flushing update remembered set: %d", cap->no, markQueueLength(&cap->upd_rem_set.queue)); traceConcUpdRemSetFlush(cap); - nonmovingAddUpdRemSetBlocks(&cap->upd_rem_set.queue); + nonmovingAddUpdRemSetBlocks_lock(&cap->upd_rem_set.queue); atomic_inc(&upd_rem_set_flush_count, 1); signalCondition(&upd_rem_set_flushed_cond); // After this mutation will remain suspended until nonmovingFinishFlush @@ -399,7 +424,7 @@ void nonmovingFinishFlush(Task *task) { // See Note [Unintentional marking in resurrectThreads] for (uint32_t i = 0; i < getNumCapabilities(); i++) { - reset_upd_rem_set(&getCapability(i)->upd_rem_set); + nonmovingResetUpdRemSet(&getCapability(i)->upd_rem_set); } // Also reset upd_rem_set_block_list in case some of the UpdRemSets were // filled and we flushed them. @@ -424,7 +449,8 @@ push (MarkQueue *q, const MarkQueueEnt *ent) if (q->top->head == MARK_QUEUE_BLOCK_ENTRIES) { // Yes, this block is full. if (q->is_upd_rem_set) { - nonmovingAddUpdRemSetBlocks(q); + // Flush the block to the global update remembered set + nonmovingAddUpdRemSetBlocks_lock(q); } else { // allocate a fresh block. ACQUIRE_SM_LOCK; @@ -780,7 +806,7 @@ void markQueuePushClosure (MarkQueue *q, /* TODO: Do we really never want to specify the origin here? */ void markQueueAddRoot (MarkQueue* q, StgClosure** root) { - markQueuePushClosure(q, *root, NULL); + markQueuePushClosureGC(q, *root); } /* Push a closure to the mark queue without origin information */ @@ -908,18 +934,24 @@ void initMarkQueue (MarkQueue *queue) } /* Must hold sm_mutex. */ -void init_upd_rem_set (UpdRemSet *rset) +void nonmovingInitUpdRemSet (UpdRemSet *rset) { init_mark_queue_(&rset->queue); rset->queue.is_upd_rem_set = true; } -void reset_upd_rem_set (UpdRemSet *rset) +static void nonmovingResetUpdRemSetQueue (MarkQueue *rset) { // UpdRemSets always have one block for the mark queue. This assertion is to // update this code if we change that. - ASSERT(rset->queue.blocks->link == NULL); - rset->queue.top->head = 0; + ASSERT(rset->is_upd_rem_set); + ASSERT(rset->blocks->link == NULL); + rset->top->head = 0; +} + +void nonmovingResetUpdRemSet (UpdRemSet *rset) +{ + nonmovingResetUpdRemSetQueue(&rset->queue); } void freeMarkQueue (MarkQueue *queue) diff --git a/rts/sm/NonMovingMark.h b/rts/sm/NonMovingMark.h index 1cfcc860bf..4b5c61cb38 100644 --- a/rts/sm/NonMovingMark.h +++ b/rts/sm/NonMovingMark.h @@ -137,8 +137,7 @@ extern bdescr *upd_rem_set_block_list; void nonmovingMarkInitUpdRemSet(void); -void init_upd_rem_set(UpdRemSet *rset); -void reset_upd_rem_set(UpdRemSet *rset); +void nonmovingInitUpdRemSet(UpdRemSet *rset); void updateRemembSetPushClosure(Capability *cap, StgClosure *p); void updateRemembSetPushThunk(Capability *cap, StgThunk *p); void updateRemembSetPushTSO(Capability *cap, StgTSO *tso); @@ -165,7 +164,7 @@ void nonmovingResurrectThreads(struct MarkQueue_ *queue, StgTSO **resurrected_th bool nonmovingIsAlive(StgClosure *p); void nonmovingMarkDeadWeak(struct MarkQueue_ *queue, StgWeak *w); void nonmovingMarkLiveWeak(struct MarkQueue_ *queue, StgWeak *w); -void nonmovingAddUpdRemSetBlocks(struct MarkQueue_ *rset); +void nonmovingAddUpdRemSetBlocks(UpdRemSet *rset); void markQueuePush(MarkQueue *q, const MarkQueueEnt *ent); void markQueuePushClosureGC(MarkQueue *q, StgClosure *p); diff --git a/rts/sm/Storage.c b/rts/sm/Storage.c index 40353ea180..5ba971bbcf 100644 --- a/rts/sm/Storage.c +++ b/rts/sm/Storage.c @@ -325,7 +325,7 @@ void storageAddCapabilities (uint32_t from, uint32_t to) if (RtsFlags.GcFlags.useNonmoving) { nonmovingAddCapabilities(to); for (i = 0; i < to; ++i) { - init_upd_rem_set(&getCapability(i)->upd_rem_set); + nonmovingInitUpdRemSet(&getCapability(i)->upd_rem_set); } } |