Test and Fixed for SharedPtr leak #46
Conversation
threading/smartptrs.nim
Outdated
| if p.container != nil: | ||
| # this `fetchSub` returns current val then subs | ||
| # so count == 1 means we're the last | ||
| if p.container.counter.fetchSub(1, Release) == 1: |
There was a problem hiding this comment.
Returning the current value and then decrementing sounds like an unnatural CPU instruction. Are you sure this generates good code?
There was a problem hiding this comment.
Most of the atomics I've seen return the value after modification.
I haven't looked at the generated code but figure the C intrinsics are correct. Looks like ARM64 does some sort of register locking and the operations inside are flexible: https://stackoverflow.com/a/35218319 Maybe there's performance implications? I sorta thought they had specific atomic instructions, but sounds like x86_64 does something similar. Though this guy seems a bit different: https://devblogs.microsoft.com/oldnewthing/20220811-00/?p=106963
The std/atomics version does the decrement and returns the result. It took me a bit to figure out the difference. Both work and pass tests.
There was a problem hiding this comment.
Looks like Rust does it similarly in their Arc: https://github.com/rust-lang/rust/blob/a09c1f85f1bc5be9a85e1395e7482caff91260cc/library/alloc/src/sync.rs#L2372
Their fetch_sub also does "Subtracts from the current value, returning the previous value": https://github.com/rust-lang/rust/blob/a09c1f85f1bc5be9a85e1395e7482caff91260cc/library/core/src/sync/atomic.rs#L2550C1-L2551C1 .
There was a problem hiding this comment.
Ah, I think I get it a bit. The release mode makes the atomic register-load strictly ordered, so returning that value (the original) makes sense whereas the atomic register-write at the end isn't as strict.
I think doing sub_fetch would need to have acquire mode. The names seem sorta backwards to me.
|
@Araq any word on this? It'd be nice to get the standard repo stable since I am also getting memory allocator corruption when using the master branch. I suspect use after frees. I don't have the time to prove it, but this branch works like a charm. There's probably a sequence where one thread gets 0 thinking its ready to free, but before doing so another takes it's first copy that'd lead to a double free or use-after-free. |
|
Rename it back to |
Ah, that was my favorite part. Ok done. |
Change back to zero based count to restore good behavior with `default()`
update comment
See #45.