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
ObjectPool's get/release/remove/add/clear/set_size methods mutate shared state (__pool map, __inactiveObject0/1, __inactiveObjectList, activeObjects, inactiveObjects) without synchronization. When OpenFL or user code drives the same pool from multiple hxcpp threads (software renderer + GL renderer, or any custom thread pool), the counters drift and the inactive-object slots can hand the same instance to two callers. This change adds a sys.thread.Mutex (guarded behind #if target.threaded so js / flash builds keep the existing zero-overhead path) and wraps the public mutating methods with __lock()/__unlock() inline helpers. The internal __addInactive / __getInactive / __removeInactive helpers are not locked separately — they are only ever called from already-locked public paths. In #if debug release() now also unlocks and returns after Log.error, matching the intent of the existing guard (don't decrement counters for an invalid object). Without the early return the function continued into activeObjects-- and __pool.remove on an invalid argument, corrupting pool state further.
Covers the API surface of lime.utils.ObjectPool with single-threaded functional tests (get/release reuse, size cap, clean callback, clear, remove, size setter pre-fill) and one multi-thread stress test guarded by #if target.threaded. The threaded test runs 8 worker threads x 1000 get/release iterations on a 4-slot pool, with a 5-second deadline in the main waiter loop so that pool-state corruption surfaces as a hard test failure instead of hanging the CI run. On the un-mutexed (pre-patch) ObjectPool the counters drift past the pool size (typical observation: active=5 inactive=-1 after all workers complete); on the mutex-guarded version the counters return to active=0 inactive=POOL_SIZE every run. Registered in TestMain.hx alongside the existing utils.* test cases.
Member
|
I wonder if it would make sense to add a separate |
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.