also looking at this
[codex] Fix parallel mark claiming
#1
1 file
+19
-6
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -437,7 +437,7 @@ pub fn Heap(comptime Binding: type) type { | ||
| 437 | 437 | /// and a mutator's `writeBarrier` never both push the same cell; the |
| 438 | 438 | /// single-threaded path is a plain check. |
| 439 | 439 | fn claimMark(self: *Self, h: *Header) bool { |
| 440 | if (self.concurrent.load(.acquire)) { | |
| 440 | if (self.parallel or self.concurrent.load(.acquire)) { | |
| 441 | 441 | if (@cmpxchgStrong(bool, &h.marked, false, true, .acq_rel, .monotonic) != null) return false; |
| 442 | 442 | _ = @atomicRmw(usize, &self.marked_count, .Add, 1, .monotonic); |
| 443 | 443 | return true; |
| @@ -549,10 +549,20 @@ pub fn Heap(comptime Binding: type) type { | ||
| 549 | 549 | /// stopped, so: |
| 550 | 550 | /// - The whiten pass + state reset run under `alloc_lock`, the same |
| 551 | 551 | /// leaf lock `create` takes, so the O(n) `all`-list walk and the |
| 552 | /// `marked` clears can't race a peer's prepend / born-grey mark-bit | |
| 553 | /// set. `alloc_lock` is a leaf (never held across a safepoint or a | |
| 554 | /// per-structure lock), so guarding the walk with it cannot deadlock | |
| 555 | /// a mutator that is parked or spinning for an object lock. | |
| 552 | /// born-grey mark-bit set can't race a peer's prepend. `alloc_lock` | |
| 553 | /// is a leaf (never held across a safepoint or a per-structure | |
| 554 | /// lock), so guarding the walk with it cannot deadlock a mutator | |
| 555 | /// that is parked or spinning for an object lock. The whiten *stores* | |
| 556 | /// to `marked` are atomic because `alloc_lock` does NOT serialize the | |
| 557 | /// barrier path: a peer still finishing a store from the *previous* | |
| 558 | /// cycle (it read `marking` true before the prior finish cleared it) | |
| 559 | /// reaches `claimMark`, whose CAS atomically touches `marked` without | |
| 560 | /// `alloc_lock`. That CAS is always a benign no-op here — its target | |
| 561 | /// was reachable-and-marked last cycle, so the strong CAS fails and | |
| 562 | /// `writeBarrier` returns before mutating any list (a successful CAS | |
| 563 | /// would mean a swept-garbage target, which the terminal root | |
| 564 | /// handshake already rules out). Atomic-vs-atomic is race-free; the | |
| 565 | /// new cycle's happens-before is the `marking`=true release below. | |
| 556 | 566 | /// - `marking`+`concurrent` are published *while still holding* |
| 557 | 567 | /// `alloc_lock`, so no cell can be prepended between "whitened" and |
| 558 | 568 | /// "barrier armed": a `create` that wins the lock after us already |
| @@ -571,7 +581,10 @@ pub fn Heap(comptime Binding: type) type { | ||
| 571 | 581 | std.debug.assert(self.parallel); |
| 572 | 582 | self.lockAlloc(); |
| 573 | 583 | var it = self.all; |
| 574 | while (it) |h| : (it = h.next) h.marked = false; | |
| 584 | // Atomic store: a lagging peer's `claimMark` CAS (barrier path, no | |
| 585 | // `alloc_lock`) can touch the same `marked` byte concurrently. See | |
| 586 | // the whiten note in the doc comment above. | |
| 587 | while (it) |h| : (it = h.next) @atomicStore(bool, &h.marked, false, .monotonic); | |
| 575 | 588 | self.marked_count = 0; |
| 576 | 589 | self.mark_stack.clearRetainingCapacity(); |
| 577 | 590 | self.weak_slots.clearRetainingCapacity(); |