diff --git a/lib/system/arc.nim b/lib/system/arc.nim index d380aa621d..cd74e7f4fa 100644 --- a/lib/system/arc.nim +++ b/lib/system/arc.nim @@ -252,7 +252,36 @@ proc nimDecRefIsLast(p: pointer): bool {.compilerRtl, inl.} = writeStackTrace() cfprintf(cstderr, "[DecRef] %p %ld\n", p, cell.count) - when (defined(gcAtomicArc) or defined(gcYrc)) and hasThreadSupport: + when defined(gcAtomicArc) and hasThreadSupport and + not defined(nimNoAtomicArcFastPath): + # Uniquely-referenced fast path: skip the RMW entirely. + # + # A counted reference can only be derived from the location being + # destroyed (which happens-before this destructor, or the program races + # on that location) or from another counted reference (whose + # contribution is already in `rc`, forcing the RMW below). So observing + # a zero count proves no other thread holds a reference to this cell and + # therefore none can be inside this destructor: there is nothing to + # adjudicate and no RMW is needed. This is only sound because + # `--mm:atomicArc` has no collector -- ORC/YRC mutate `rc` from a + # participant that holds no counted reference at all. + # + # The load must be ACQUIRE: the count may have reached zero because + # another thread's release-decrement got there first, and we have to see + # its writes before destroying the object. + # + # The slow path stays self-testing (it frees on the value the RMW + # returned, never on a separate load), which is what keeps this out of + # the nim-lang/threading#45 bug class. + if (atomicLoadN(addr cell.rc, ATOMIC_ACQUIRE) and not rcMask) == 0: + result = true + when traceCollector: + cprintf("[ABOUT TO DESTROY] %p\n", cell) + elif atomicDec(cell.rc, rcIncrement) == -rcIncrement: + result = true + when traceCollector: + cprintf("[ABOUT TO DESTROY] %p\n", cell) + elif (defined(gcAtomicArc) or defined(gcYrc)) and hasThreadSupport: # `atomicDec` returns the new value if atomicDec(cell.rc, rcIncrement) == -rcIncrement: result = true diff --git a/tests/arc/tconcurrentdecref.nim b/tests/arc/tconcurrentdecref.nim new file mode 100644 index 0000000000..2222fe51b7 --- /dev/null +++ b/tests/arc/tconcurrentdecref.nim @@ -0,0 +1,71 @@ +discard """ + matrix: "--mm:atomicArc --threads:on" + output: "ok" +""" + +# Every thread here holds its OWN counted reference to the same cell and drops +# it concurrently with the others. Exactly one free per object must happen: a +# leak (nobody frees) and a double free (two threads free) are both caught. +# +# This is the shape that went wrong in nim-lang/threading#45, where the +# destructor decided who frees from a separate load and discarded the result +# of the read-modify-write, so the role could be dropped by every participant +# at once. `nimDecRefIsLast` must always decide on the value its own RMW +# returned. The uniquely-referenced fast path added on top of it may only +# skip the RMW when the load proves no other thread holds a reference. + +import std/atomics + +type + Payload = object + id: int + Obj = ref Payload + +var freeCount: Atomic[int] + +proc `=destroy`(p: Payload) = + discard freeCount.fetchAdd(1, moRelease) + +const + NumObjects = 2000 + NumThreads = 6 + Rounds = 3 + +type + Arg = object + refs: seq[Obj] + +var + go: Atomic[bool] + threads: array[NumThreads, Thread[ptr Arg]] + args: array[NumThreads, Arg] + +proc worker(a: ptr Arg) {.thread.} = + while not go.load(moAcquire): cpuRelax() + a.refs.setLen(0) # drop them all, as fast as possible + +proc main = + var expected = 0 + for round in 1..Rounds: + var mine = newSeq[Obj](NumObjects) + for i in 0 ..< NumObjects: + mine[i] = Obj(id: i) + for t in 0 ..< NumThreads: + args[t].refs = newSeq[Obj](NumObjects) + for i in 0 ..< NumObjects: + args[t].refs[i] = mine[i] # counted copy + go.store(false, moRelease) + for t in 0 ..< NumThreads: + createThread(threads[t], worker, addr args[t]) + go.store(true, moRelease) # everybody drops at once... + mine.setLen(0) # ...including this thread + joinThreads(threads) + expected += NumObjects + let got = freeCount.load(moAcquire) + if got != expected: + echo "round ", round, ": got ", got, " frees, expected ", expected, + (if got < expected: " (leak)" else: " (double free)") + quit 1 + echo "ok" + +main()