Make Slab safe to use from multiple threads - #344
alexcrichton merged 2 commits into
Conversation
wasmtime objects free their `Slab` handles from finalizers, and the garbage
collector runs finalizers on whichever thread triggers a collection. So an
application that calls wasmtime from one thread, while other threads just
allocate Python objects (a boto upload, in our case), gets `deallocate`
running on those other threads at the same time as `allocate` on the
wasmtime thread.
The old `Slab` can't handle that. Its free list lives in the same array as
its values, and both `allocate` and `deallocate` update it across several
statements. If a thread switch lands in the middle, one handle goes to two
owners, and later the slab is poisoned for good:
TypeError: list indices must be integers or slices, not tuple
Get rid of the free list. Handles come from a counter and values live in a
dict, so there's no multi-statement state left to interleave. A lock guards
only the counter (`next()` on `itertools.count` isn't atomic on
free-threaded builds). The lock is never held while a value is dropped, so a
finalizer that frees another handle can't deadlock. Handles start at 1, so a
null `void *` env read back as `idx or 0` can never match a live handle.
The new test runs allocate/deallocate from several threads. Against the old
`Slab` it fails every time, with an aliased handle or the `TypeError` above.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks! I've always historically hoped there's a better solution here but I'm not sure there is one...
As a comment on the implementation though, could the implementation largely stay the same way it was but with a lock around allocate/deallocate? I don't think it should be necessary to switch to a dictionary and a counter iterator in theory.
|
Hi @alexcrichton, thnx for a quick feedback. Main reason I chose a different structure was so I wouldn't need to use lock/unlock around allocate/deallocate, so there wouldn't be any performance loss. If some perf loss is okey, I'm happy to change the impl. |
|
Oh I hadn't really considered perf loss here, I was mostly just thinking of minimizing the diff and naively to me arrays feel faster than dicts, but I've no idea if that's actually the case. I'd lean towards the array-based solution as I'd suspect in the end it'd be more efficient, but if you feel differently this strategy seems reasonable too |
|
I created a small benchmark script, with single and multiple threads, check out how it looks for your setup ns per get+deallocate+allocate (best of 5, 1,000,000 ops)
threads list+lock dict speedup
1 243 114 2.13x
4 249 116 2.15xbenchmark script code: |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
That's quite the difference!
2 participants
We started using wasmtime-py from multiple treads and noticed that we are getting memory corruptions.
As a fix, changed non thread safe usage of lists for tracking memory slabs, to dict + counter, which is thread safe.
We already have an internal monkey patch with this behavior, so we are hoping to either merge this functionality upstream, or to learn if our approach has some gaps.