diff options
author | Ben Gamari <ben@smart-cactus.org> | 2022-11-30 15:43:05 -0500 |
---|---|---|
committer | Marge Bot <ben+marge-bot@smart-cactus.org> | 2023-03-08 15:02:30 -0500 |
commit | 1b06967176559d6b2b530dd16e127fa4479ae47f (patch) | |
tree | 3c844098df62dac63bc8461a834296e7e9624b46 | |
parent | e4c3249f00a406a406b6f1190ca8be628b643042 (diff) | |
download | haskell-1b06967176559d6b2b530dd16e127fa4479ae47f.tar.gz |
nonmoving: Add missing write barriers in selector optimisation
This fixes the selector optimisation, adding a few write barriers which
are necessary for soundness. See the inline comments for details.
Fixes #22930.
-rw-r--r-- | rts/sm/Evac.c | 43 | ||||
-rw-r--r-- | rts/sm/NonMovingMark.c | 25 |
2 files changed, 62 insertions, 6 deletions
diff --git a/rts/sm/Evac.c b/rts/sm/Evac.c index 578ce153b9..5a523a7d81 100644 --- a/rts/sm/Evac.c +++ b/rts/sm/Evac.c @@ -1232,7 +1232,7 @@ unchain_thunk_selectors(StgSelector *p, StgClosure *val) -------------------------------------------------------------------------- */ static void -eval_thunk_selector (StgClosure **q, StgSelector *p, bool evac) +eval_thunk_selector (StgClosure **const q, StgSelector *p, bool evac) // NB. for legacy reasons, p & q are swapped around :( { uint32_t field; @@ -1252,14 +1252,24 @@ selector_chain: bd = Bdescr((StgPtr)p); if (HEAP_ALLOCED_GC(p)) { + const uint16_t flags = RELAXED_LOAD(&bd->flags); + + // We should never see large objects here + ASSERT(!(flags & BF_LARGE)); + // If the THUNK_SELECTOR is in to-space or in a generation that we // are not collecting, then bale out early. We won't be able to // save any space in any case, and updating with an indirection is // trickier in a non-collected gen: we would have to update the // mutable list. - uint16_t flags = RELAXED_LOAD(&bd->flags); if (flags & (BF_EVACUATED | BF_NONMOVING)) { unchain_thunk_selectors(prev_thunk_selector, (StgClosure *)p); + if (flags & BF_NONMOVING) { + // The selector is reachable therefore we must push to the + // update remembered set in a preparatory collection. + // See Note [Non-moving GC: Marking evacuated objects]. + markQueuePushClosureGC(&gct->cap->upd_rem_set.queue, (StgClosure*) p); + } *q = (StgClosure *)p; // shortcut, behave as for: if (evac) evacuate(q); if (evac && bd->gen_no < gct->evac_gen_no) { @@ -1314,6 +1324,12 @@ selector_chain: // - undo the chain we've built to point to p. SET_INFO((StgClosure *)p, (const StgInfoTable *)info_ptr); RELEASE_STORE(q, (StgClosure *) p); + if (Bdescr((StgPtr)p)->flags & BF_NONMOVING) { + // See Note [Non-moving GC: Marking evacuated objects]. + // TODO: This really shouldn't be necessary since whoever won + // the race should have pushed + markQueuePushClosureGC(&gct->cap->upd_rem_set.queue, (StgClosure*) p); + } if (evac) evacuate(q); unchain_thunk_selectors(prev_thunk_selector, (StgClosure *)p); return; @@ -1404,6 +1420,11 @@ selector_loop: case THUNK_SELECTOR: // Use payload to make a list of thunk selectors, to be // used in unchain_thunk_selectors + // + // FIXME: This seems racy; should we lock this selector to + // ensure that another thread doesn't clobber this node + // of the chain. This would result in some previous + // selectors not being updated when we unchain. RELAXED_STORE(&((StgClosure*)p)->payload[0], (StgClosure *)prev_thunk_selector); prev_thunk_selector = p; p = (StgSelector*)val; @@ -1427,7 +1448,15 @@ selector_loop: // evacuate() cannot recurse through // eval_thunk_selector(), because we know val is not // a THUNK_SELECTOR. - if (evac) evacuate(q); + if (evac) { + evacuate(q); + } else if (isNonmovingClosure(*q)) { + // Even if `evac` is false we must ensure that the closure is + // pushed since it is reachable. + // See Note [Non-moving GC: Marking evacuated objects]. + markQueuePushClosureGC(&gct->cap->upd_rem_set.queue, (StgClosure*) *q); + } + return; } @@ -1472,6 +1501,10 @@ selector_loop: // recurse indefinitely, so we impose a depth bound. // See Note [Selector optimisation depth limit]. if (gct->thunk_selector_depth >= MAX_THUNK_SELECTOR_DEPTH) { + if (isNonmovingClosure((StgClosure *) p)) { + // See Note [Non-moving GC: Marking evacuated objects]. + markQueuePushClosureGC(&gct->cap->upd_rem_set.queue, (StgClosure*) p); + } goto bale_out; } @@ -1518,5 +1551,9 @@ bale_out: if (evac) { copy(q,(const StgInfoTable *)info_ptr,(StgClosure *)p,THUNK_SELECTOR_sizeW(),bd->dest_no); } + if (isNonmovingClosure(*q)) { + // See Note [Non-moving GC: Marking evacuated objects]. + markQueuePushClosureGC(&gct->cap->upd_rem_set.queue, *q); + } unchain_thunk_selectors(prev_thunk_selector, *q); } diff --git a/rts/sm/NonMovingMark.c b/rts/sm/NonMovingMark.c index 56cb73b5e4..1b1e1c7b79 100644 --- a/rts/sm/NonMovingMark.c +++ b/rts/sm/NonMovingMark.c @@ -648,6 +648,16 @@ void updateRemembSetPushThunkEager(Capability *cap, } break; } + case THUNK_SELECTOR: + { + StgSelector *sel = (StgSelector *) thunk; + if (check_in_nonmoving_heap(sel->selectee)) { + // Don't bother to push origin; it makes the barrier needlessly + // expensive with little benefit. + push_closure(queue, sel->selectee, NULL); + } + break; + } case AP: { StgAP *ap = (StgAP *) thunk; @@ -657,9 +667,11 @@ void updateRemembSetPushThunkEager(Capability *cap, trace_PAP_payload(queue, ap->fun, ap->payload, ap->n_args); break; } - case THUNK_SELECTOR: + // We may end up here if a thunk update races with another update. + // In this case there is nothing to do as the other thread will have + // already pushed the updated thunk's free variables to the update + // remembered set. case BLACKHOLE: - // TODO: This is right, right? break; // The selector optimization performed by the nonmoving mark may have // overwritten a thunk which we are updating with an indirection. @@ -1588,8 +1600,15 @@ mark_closure (MarkQueue *queue, const StgClosure *p0, StgClosure **origin) } case THUNK_SELECTOR: - nonmoving_eval_thunk_selector(queue, (StgSelector*)p, origin); + { + StgSelector *sel = (StgSelector *) p; + // We may be able to evaluate this selector which may render the + // selectee unreachable. However, we must mark the selectee regardless + // to satisfy the snapshot invariant. + PUSH_FIELD(sel, selectee); + nonmoving_eval_thunk_selector(queue, sel, origin); break; + } case AP_STACK: { StgAP_STACK *ap = (StgAP_STACK *)p; |