performance: defer cache rebuilds and reduce allocations in MapInstance#2810
Closed
performance: defer cache rebuilds and reduce allocations in MapInstance#2810
Conversation
Replace per-mutation cache rebuilds with dirty-flag deferred flushing across entities, projectiles, and traps in MapInstance, and cache surrounding-instance lookups to eliminate per-tick allocations. - Add _entityCacheDirty / _projectileCacheDirty / _trapCacheDirty flags; caches are now rebuilt once per tick in FlushDirtyCaches() called at the top of Update(), not on every Add/Remove call - Cache surrounding MapInstance lists (_cachedSurroundingInstances / _cachedSurroundingInstancesWithSelf) with an invalidation flag; replace all MapController.GetSurroundingMapInstances() call sites with GetCachedSurroundingInstances() to avoid allocating a new List<> on every tick - Add _entityListLock to decouple entity-list mutations from mMapProcessLock, reducing lock contention during the update cycle - Replace mIsProcessing = GetPlayers(true).Any() with a direct check over cached surrounding instances to avoid double allocation - Use ArrayPool<Guid>.Shared in DespawnProjectiles() to avoid a per-call List<Guid> and ToArray() - Snapshot NpcSpawnInstances/mEntities before acquiring EntityLocks in ResetNpcSpawns() and DespawnNpcs() to eliminate nested-lock deadlock risk - Replace GetEntities() call sites in hot paths (IsTileBlocked, UpdateResources, ClearEntityTargetsOf) with direct mCachedEntities array iteration - Use GlobalEventInstances.Values directly in UpdateGlobalEvents() instead of ToList(); short-circuit active search with break - Lazy-init toRemove list in SpawnItem(): no allocation in the common zero-match case - Replace ContainsKey+indexer double-lookup with TryGetValue in GetGlobalEventInstance() and FindEvent() - Invert null-guard conditions to early-return (SendMapEntitiesTo, RemoveItem, SpawnItems, UpdateResources) for reduced nesting - Replace if/else attribute type checks with switch expressions in SpawnMapAttributes() and CacheMapBlocks() - Collapse trivial single-expression methods to expression bodies (GetLock, GetController, ShouldBeCleaned, ShouldBeActive, GetCachedEntities, GetCachedBlocks, AddBatched*) - Remove GetControllerLock(): unused after refactor - Trim verbose XML doc comments to concise inline remarks; remove redundant using Intersect.Utilities Signed-off-by: Arufonsu <17498701+Arufonsu@users.noreply.github.com>
Arufonsu
added
chore
pandinocoder
requested changes
Feb 22, 2026
Member
pandinocoder
left a comment
pandinocoder
left a comment
There was a problem hiding this comment.
I'm not sure what's going on with this PR but I'm not done reviewing it. I suspect my existing comments apply to more than the stuff I read though.
Comment on lines
+169
to
+170
| private List<MapInstance> _cachedSurroundingInstances = new List<MapInstance>(); | ||
| private List<MapInstance> _cachedSurroundingInstancesWithSelf = new List<MapInstance>(); |
Member
There was a problem hiding this comment.
Prefer = []; syntax for non-Array collections
| // Cached surrounding-instance lists rebuilt only when dirty. | ||
| private List<MapInstance> _cachedSurroundingInstances = new List<MapInstance>(); | ||
| private List<MapInstance> _cachedSurroundingInstancesWithSelf = new List<MapInstance>(); | ||
| private bool _surroundingCacheDirty = true; |
Member
There was a problem hiding this comment.
Why is this not volatile when the others are?
|
|
||
| public bool IsDisposed { get; protected set; } | ||
|
|
||
| /// <summary> |
Member
There was a problem hiding this comment.
Why did you gut a doc comment...?
| /// Initializes the processing instance for processing - called in the constructor. Essentially refreshes the instance | ||
| /// so that it can give everything it has to offer to the player by the time they arrive in it | ||
| /// </summary> | ||
| // Invalidate the surrounding-instance cache when grid/instance topology changes. |
| // Invalidate the surrounding-instance cache when grid/instance topology changes. | ||
| public void InvalidateSurroundingCache() => _surroundingCacheDirty = true; | ||
|
|
||
| // Returns a cached list of surrounding MapInstances (only rebuilt when dirty). |
| { | ||
| dir = Randomization.NextDirection(); | ||
| } | ||
| dir = spawn.Direction != NpcSpawnDirection.Random ? (Direction)(spawn.Direction - 1) : Randomization.NextDirection(); |
Member
There was a problem hiding this comment.
This is less readable than the if statements and gives no performance benefit
| MapInstanceId = processLayer | ||
| }; | ||
|
|
||
| var npc = new Npc(npcBase, despawnable) { MapId = mMapController.Id, X = tileX, Y = tileY, Dir = dir, MapInstanceId = MapInstanceId }; |
Member
There was a problem hiding this comment.
This shouldn't be squished into one line.
|
|
||
| /// <summary> | ||
| /// Forcibly reset all Npcs originating from this map. | ||
| /// Snapshot spawn list before acquiring EntityLocks to avoid nested-lock deadlock. |
Member
There was a problem hiding this comment.
Why did you replace a proper doc comment with a detail comment that also doesn't appear to be relevant to the method operation and probably should be inside the method instead.
| /// </summary> | ||
| public void ResetNpcSpawns() | ||
| { | ||
| //Kill all npcs spawned from this map |
Member
There was a problem hiding this comment.
Should move the comment down to where it's relevant
|
|
||
| /// <summary> | ||
| /// Despawns all NPCs, and removes them from our dictionary of Spawn Instances. | ||
| /// Snapshot before EntityLocks to avoid nested-lock deadlock. |
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
2 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.
Replace per-mutation cache rebuilds with dirty-flag deferred flushing across entities, projectiles, and traps in MapInstance, and cache surrounding-instance lookups to eliminate per-tick allocations.