[WIP] Make Arc decrefs and increfs atomic #88
Conversation
|
The dec and incs must be ordered correctly; the load can be relaxed as it will not be able to pass any dec/incs due to their stricter ordering. The subsequent atomicdec being ordered prevents the load moving forward. Since the load is used as the condition for entering the block, should be aiight. Else I'll change it to acqrel |
I guess you haven't noticed this: nimskull/lib/system/atomics.nim Lines 228 to 233 in 989adc6 |
|
F*** 🤣 I assumed seqcst is the default! Hahaha I made an ass out of myself. Thank you bb ❤️ I'll fix it up |
There was a problem hiding this comment.
If you're going to touch every line in atomics why not improve it -- do we need to be exporting a distinct cint for AtomMemModel or can we make the API idiomatic, with an enum? These variables and constants are code smell; you can hide them in here with a converter if you're lazy. Also not a fan of the barrier template. Yikes.
| # proc atomicInc*(memLoc: var int, x: int = 1; order: AtomMemModel = ATOMIC_RELAXED): int {.inline, | ||
| # discardable, benign.} | ||
| # ## Atomic increment of `memLoc`. Returns the value after the operation. | ||
|
|
||
| # proc atomicDec*(memLoc: var int, x: int = 1; order: AtomMemModel = ATOMIC_RELAXED): int {.inline, | ||
| # discardable, benign.} | ||
| # ## Atomic decrement of `memLoc`. Returns the value after the operation. |
There was a problem hiding this comment.
Prefer when false: and document why these are removed.
| @@ -116,15 +116,30 @@ proc nimNewObjUninit(size, alignment: int): pointer {.compilerRtl.} = | |||
| cprintf("[Allocated] %p result: %p\n", result -! sizeof(RefHeader), result) | |||
|
|
|||
| proc nimDecWeakRef(p: pointer) {.compilerRtl, inl.} = | |||
| dec head(p).rc, rcIncrement | |||
| # We want to use atomic operations when threading is on | |||
| template decOpr(x,y: untyped): untyped = | ||
| when hasThreadSupport: | ||
| atomicDec(x, y, ATOMIC_ACQ_REL) | ||
| else: | ||
| dec(x, y) |
There was a problem hiding this comment.
Move it out and reuse it below. Add a comment or more descriptive name. Why not use typed?
| template loadOpr(x: untyped): untyped = | ||
| when hasThreadSupport: | ||
| atomicLoadN(x.addr, ATOMIC_ACQUIRE) | ||
| else: | ||
| x | ||
| template decOpr(x, y: untyped): untyped = | ||
| when hasThreadSupport: | ||
| atomicDec(x, y, ATOMIC_RELEASE) | ||
| else: | ||
| dec(x, y) | ||
|
|
There was a problem hiding this comment.
Comment or rename to be more descriptive.
|
Also, I would favor a single |
Bloody CLRF. |
As per the discussion on shared refs and arc, this fix will prevent any undefined behavior occuring.
The idea is to:
Have all atomic operations on ref count changes when threads:on
Make atomic operations opt-out using a Owned[T] type
Include a one file test that demonstrates the behavior before/after