Fix/thread safety bullet particle - #652
Closed
noisethanks wants to merge 2165 commits into
Closed
noisethanks wants to merge 2165 commits into
noisethanks wants to merge 2165 commits into
Conversation
# Conflicts: # src/xrGame/console_commands.cpp
# Conflicts: # src/xrGame/Actor.cpp
0.5*ssaDiscard will draw half of objects, 0.25*ssaDiscard will draw 1/4 of objects less than that is not drawn
…ncrease density of closer objects, > 1 will reduce * Apply gradient cutoff to DetailManager
…ike lasersights, 0.5 exp to keep statics dense
* use IRenderVisualFlags::eIgnoreOptimization to toggle ssa discard per pVisual * Hud geometry and script attachments (hud and cam) have this flag
…guards - Add r__tex_evict_interval cvar (default 600, min 60, max 3600) controls frames between eviction passes, previously hardcoded - Wrap all TexEvict Msg() calls and counter variables in #ifdef DEBUG so release builds have zero logging overhead - Four cvars now expose the complete eviction tuning surface: r__tex_evict_enabled, r__tex_evict_age_frames, r__tex_evict_batch_size, r__tex_evict_interval
…stalled-textures feat(renderer): mid-session texture eviction system (DX11)
alife: guard against _SPAWN_ID(-1) in spawn graph traversal
…monized/xray-monolith into all-in-one-vs2022-wpo-mt
# Conflicts: # src/xrGame/sight_action.cpp
…ture_load_tasks PPL group.
(cherry picked from commit ad92b915c9a4a4c2124d054056be2ddc6834e5d8)
…xture-reload-thread-safety Address a race condition with reloading evicted textures by using texture_load_tasks PPL group.
# Conflicts: # src/xrGame/Level_Bullet_Manager.h
…monized/xray-monolith into all-in-one-vs2022-wpo-mt
dynamic PDA tab support for MT
# Conflicts: # src/xrGame/ui/UITabButton.h
# Conflicts: # src/xrGame/Actor.cpp # src/xrGame/ui/UI3tButton.cpp # src/xrGame/ui/UIFrameLineWnd.h # src/xrGame/ui/UIMotionIcon.h
CBulletManager::Clear() (Level_Bullet_Manager.cpp) took no lock, while UpdateWorkload/Render/AddBullet in the same class all correctly guard m_Bullets with m_Lock. UpdateWorkload runs on GameThread, Render on the main thread - genuine concurrent access, gated on mtBullets (on by default). Added the same m_Lock guard to Clear() for consistency. Also removed a stale DEBUG-only VERIFY(m_thread_id == GetCurrentThreadId()) in AddBullet - it asserts main-thread-only, but AddBullet legitimately runs off-main today (ShootingObject, WeaponKnife, Explosive, Lua's level.add_bullet). The matching assertion in UpdateWorkload was already removed; this restores consistency. CParticleGroup: three sites touch Device.seqParallelBeforRender with no synchronization - SItem::OnFrame (worker thread) and SItem::~SItem (either thread) both push/erase entries, while CRenderDevice::on_idle (main thread) drains the vector every frame. The existing onframe_lock doesn't cover this - it's per-CParticleGroup, so it can't exclude different groups from each other on this shared Device-level vector. Added a new CRenderDevice member, seqParallelBeforRenderCS, guarding all three sites - same pattern already used elsewhere for this exact producer/drainer shape (CEventAPI::CS, CEffect_Rain::rainCS). Tested across multiple sessions including firefights with active particle effects - no crashes, hangs, or behavioral regressions.
The main thread drains Device.seqParallelBeforRender in CRenderDevice::on_idle and invokes every registered callback. Two callbacks are registered across the codebase. PS::CParticleGroup::SItem::DelayDeleteChilds comes from the particle system, and CObjectList::ProcessDestroyQueue is registered whenever mt_Scheduler is set, which is the default. ProcessDestroyQueue destroys queued objects synchronously. That reaches IGame_ObjectPool::destroy and xr_delete, then ~IRenderable and model_Delete. g_bRendering is false during the drain, so CModelPool takes the immediate branch and deletes the visual outright. When that visual is a particle group its destructor clears items, every SItem destructor runs, and each one erased its own entry from Device.seqParallelBeforRender while the drain loop was still iterating over that vector. SItem::Clear does not empty _children_destroy, so the early return at the top of ~SItem does not prevent this. ~SItem now clears its delegate in place rather than erasing it, which leaves the vector size untouched for the duration of the drain. The drain skips cleared entries and clear()s the whole vector when it finishes, so a callback belonging to a destroyed SItem is still never invoked and cancellation keeps working. The drain loop now indexes by position and copies each delegate before invoking it, so an entry appended by a callback is handled safely as well. Also guarded the two CObjectList accesses to this vector with seqParallelBeforRenderCS. Both run on the main thread and neither currently overlaps the drain, so this is consistency with the invariant the previous commit established rather than a fix for a live race.
Owner
|
Redo this PR to target MT branch, currently this will merge all MT work (2k commits) into main which is not what i want for now |
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
6 participants
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.
Fix unsynchronized concurrent access: bullet manager and particle system
Found while investigating thread safety in the engine's multithreaded
render/game-logic split. Both fixes are self-contained. Neither depends
on or relates to any TBB work.
AddBullet in the same class all correctly guard m_Bullets with m_Lock.
UpdateWorkload runs on GameThread. Render runs on the main thread. This
is genuine concurrent access, on by default via mtBullets. Clear() only
runs on level load or disconnect, so it's not on the hot path, but it
was the one unguarded member of an otherwise consistent trio. Added the
same lock.
Also removed a stale DEBUG-only thread assertion in AddBullet. It
claimed AddBullet is main-thread-only. It isn't: several weapon/explosive
paths and a Lua binding call it off-main today. The matching assertion
in UpdateWorkload was already removed. This restores consistency.
SItem::OnFrame and SItem::~SItem write to it from a worker thread, and
CRenderDevice::on_idle drains it on the main thread every frame. The
existing onframe_lock doesn't cover this, since it's a per-particle-group
member and can't protect a shared, device-level vector. Added a new
lock on CRenderDevice, following the same pattern already used elsewhere
in the codebase for this exact producer/drainer shape.
Found but not fixed: SItem's destructor has a separate, pre-existing,
same-thread hazard if a deferred callback ever destroys an SItem
mid-drain. Not currently reachable and unrelated to the race above.
Flagging it rather than expanding scope here.
Tested across multiple sessions, including firefights with active
particle effects. No crashes, hangs, or behavior changes observed. This
confirms the new locking doesn't break anything. It doesn't prove the
original race ever caused a specific crash. Both were unsynchronized
shared-state access regardless of whether either was ever observed to
fail.
Edit: Upon closer inspection, it seems that the SItem iterator hazard is reachable. Second commit addresses it.