gh-105766: Add Locking Around Custom Allocators - #105619
gh-105766: Add Locking Around Custom Allocators#105619ericsnowcurrently wants to merge 8 commits into
Conversation
Sorry, something went wrong.
|
@vstinner, any objections? I tried to do this in a way that would not penalize the main interpreter or interpreters that share the GIL. Likewise, allocators that only wrap the current allocator should not be affected. |
Sorry, something went wrong.
|
The rules was (is?) that you need to hold the GIL to call PyObject_Malloc, etc. Presumably, the presence of per-interpreter GILs, means that allocators need to use per-interpreter state rather than process-wide state, but I don't see how this enforces that. |
Sorry, something went wrong.
|
The allocators are process-global, so a per-interpreter GIL wouldn't guard against races between interpreters. Requiring that they be per-interpreter would mean we'd have to change the allocator API. |
Sorry, something went wrong.
|
If the allocators are process-wide, there would need to be a lock on every call to ob_malloc, which would be disastrous for performance. So that doesn't sound right |
Sorry, something went wrong.
|
Yeah, we lock around every allocation when a custom, non-wrapper allocator is used in a subinterpreter that has its own GIL. I don't see what else we can do, aside from doing nothing (allowing races). |
Sorry, something went wrong.
|
Maybe we just ask custom allocators to make sure they are thread-safe? |
Sorry, something went wrong.
gpshead
left a comment
There was a problem hiding this comment.
overall yes I think something like this PR is needed for the current state of affairs. reusing the main runtime's GIL as the allocator lock is also what I'd assumed would be a natural first implementation.
Sorry, something went wrong.
| return status; | ||
| } | ||
| HEAD_LOCK(runtime); | ||
| runtime->allocators.num_gils++; |
There was a problem hiding this comment.
Check for overflow. if the value is already INT_MAX pre-increment, we need to bail, even if that means SystemError.
Sorry, something went wrong.
|
|
||
| _PyRuntimeState *runtime = interp->runtime; | ||
| HEAD_LOCK(runtime); | ||
| runtime->allocators.num_gils--; |
There was a problem hiding this comment.
Add an assert(runtime->allocators.num_gils > 0); before this.
Sorry, something went wrong.
| debug_alloc_api_t obj; | ||
| } debug; | ||
| PyObjectArenaAllocator obj_arena; | ||
| int num_gils; |
There was a problem hiding this comment.
I suggest unsigned int
Sorry, something went wrong.
| _PyMem_MallocLocked(void *ctx, size_t size) | ||
| { | ||
| PyMemAllocatorEx *wrapped = (PyMemAllocatorEx *)ctx; | ||
| if (_PyRuntime.allocators.num_gils > 1) { |
There was a problem hiding this comment.
You lock updates (writes) to this value. Is it safe to use without atomic access or a lock? (requiring an atomic read here would presumably be a performance hit?).
Rather than always checking... could the logic that does num_gils++ with the (main runtime) lock held also do:
... num_gils++;
if (num_gils == 2 && !has_locking_wrapper(...)) {
maybe_add_locking_wrapper(...);
}such that the wrapped pointer switch happens upon first creation of an additional gil while the main runtime gil (the only gil prior to the current code running) is still held.
I'm not sure it is worth doing the opposite during finalization. Once wrapped due to per subinterpreter GILs, just stay wrapped. At least the wrappers won't have this conditional anymore.
I suspect this thought is either overoptimization... or actually necessary to avoid locking/atomic access to num_gils.
Sorry, something went wrong.
There was a problem hiding this comment.
That's a great idea. I'll try it out.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
|
note the discussion Eric started: https://discuss.python.org/t/what-to-do-about-custom-allocators-and-the-gil/30327 |
Sorry, something went wrong.
|
I'm closing this. Instead, we'll specify that allocators must be thread-safe (at least when isolated subinterpreters are in play). |
Sorry, something went wrong.
The "mem" and "object" allocators are documented as dependent on the GIL. However, after per-interpreter GIL landed we weren't enforcing that. This fixes that by using wrapping all allocations/frees with a dedicated runtime-global lock, but only if necessary. Note that we actually use the main interpreter's GIL as that lock, but only for interpreter's that have their own GIL. Doing so closely matches the original behavior.