ObjectPool: synchronize mutating methods with a Mutex on threaded targets#2058
Open
AxGord wants to merge 2 commits into
Open
ObjectPool: synchronize mutating methods with a Mutex on threaded targets#2058AxGord wants to merge 2 commits into
AxGord wants to merge 2 commits into
Conversation
Member
|
I wonder if it would make sense to add a separate |
ObjectPool mutates shared state (counters, fast slots, the inactive list) from get / release / add / remove / clear / size-setter. On threaded targets, concurrent use causes counter drift and duplicate handouts. ThreadSafeObjectPool extends ObjectPool and overrides every mutating public method to wrap the super call with a sys.thread.Mutex acquire / release pair. The non-mutating get_size accessor and the dynamic create / clean callbacks are intentionally untouched (callbacks are invoked from already-locked paths). On non-threaded targets (js, flash) the class is replaced with a 'typedef ThreadSafeObjectPool<T> = ObjectPool<T>'. Same compile-time API, zero runtime cost, no thread-safety code emitted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ObjectPoolTest covers the single-threaded API surface: get/release reuse, size cap, clean callback, clear, remove, size-setter pre-fill. ThreadSafeObjectPoolTest, guarded by '#if target.threaded', adds a concurrent regression: 8 worker threads x 1000 get/release iterations on a 4-slot pool, with a 5-second deadline so the test fails cleanly if pool state corrupts. Verifies no duplicate handouts and counters return to expected values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
85591ef to
c8266f1
Compare
Author
|
Done — pushed c8266f15 (force-push, the original two commits were replaced). Design now:
Class doc-comment notes two caveats: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lime.utils.ObjectPoolmutates shared state (__pool,__inactiveObject0/1,__inactiveObjectList,activeObjects,inactiveObjects) fromget/release/add/remove/clear/size-setter withoutsynchronization. When two threads drive the same pool concurrently, the
counters drift and the same instance can be handed out twice.
This PR adds a
sys.thread.Mutexguarded behind#if target.threadedsosingle-threaded targets (js, flash) keep the existing zero-overhead path,
and adds unit tests covering both the regular API surface and the
multi-thread regression.
The bug
Both
get()andrelease()decrement-and-incrementactiveObjects/inactiveObjectsand reassign__inactiveObject0/1without locking.Classic interleaving:
inactiveObjects > 0→ true, enters__getInactive().inactiveObjects > 0→ still true (A hasn't decremented yet),also enters
__getInactive().__inactiveObject0, the same instance is returned toboth callers; or
inactiveObjectsis double-decremented and ends up negative.Fix
sys.thread.Mutex __mutexfield under#if target.threaded.add,clear,get,release,remove,set_size) with two inline helpers__lock()/__unlock()that compile to no-ops on non-threaded targets.
__addInactive/__getInactive/__removeInactiveare only called from already-locked public paths — they don't lock again.
One small behaviour change
In
#if debugmoderelease()now also calls__unlock(); return;aftereach
Log.errorso a release with an invalid object can't continue intoactiveObjects--and__pool.remove(object). Without the early return,the function continued and corrupted pool state further past the detection.
Only observable in
debugbuilds.Tests
New
tests/unit/src/utils/ObjectPoolTest.hx(registered inTestMain.hx):clean callback, clear, remove, size-setter pre-fill.
testConcurrentGetReleaseDoesNotDriftCountersguarded by
#if target.threaded: 8 worker threads × 1000 get/releaseiterations on a 4-slot pool, with a 5-second deadline in the waiter
loop so the test fails cleanly if pool state corrupts (no CI hang).
Result on neko (un-mutexed run is from
develop, run via temporary checkout):Total: 9 tests, 23 assertations, ~11 ms on neko in both builds.
Risk
Non-threaded targets (js, flash) — zero runtime cost.
__lock()/__unlock()areinlineand their bodies are#if target.threaded-gated,so every call site (
get,release,add,clear,remove,set_size)compiles away completely. Verified by inspecting generated JS: no
__lock/__unlock/__mutexreferences at any call site. Twoempty function definitions remain in the output (~40 bytes of dead code,
never invoked) — happy to
#if-gate those away too if reviewers preferzero bytes.
Threaded targets — single-thread use pays an uncontended-lock cost.
Benchmarked on neko (5M get/release iterations, single thread, 64-slot
pool): unpatched ~330 ns per get/release pair, patched ~565 ns — about
+70% on neko, where
Mutexis heavy. On cpp/hl with nativeuncontended mutex (
std::mutex/ pthread futex) the cost is typicallyin the low-tens-of-ns per acquire/release, so the percent overhead is
much smaller. In real OpenFL hot paths (Event pool, Graphics
shaderBuffer pool, etc.) the call rate is ~tens-to-hundreds per frame,
so the absolute cost stays in the microsecond range per frame even on
neko.
Multi-thread use is the entire point of the PR. Without the mutex,
the test above demonstrates the pool ends up with
inactiveObjects=-1and hands out duplicates; the patched version is stable.
If reviewers want to keep single-thread users opt-out, I'm happy to
add a
-D lime_objectpool_no_mutexcompile flag in a follow-up.