Skip empty entity tick-state removals

[leroysquad]

Summary

• Players return before touching the tick-state dictionary. Other early-outs only remove an entry when ContainsKey says one exists. Tick intervals are unchanged. This is the existing Distance-Based Phase-Ticking and AI LoD #315 throttle, not a second one. Split entity movement from AI, collision, and pathfinding in the EAR throttle #262's distance cache is still open and assigned.
• This is a small cleanup with no measured gain. The 4.2% sample on the 1000-bot capture is the whole ShouldThrottleStratumEntityTick stack, not an absent-key TryRemove. A review measurement put that dictionary operation at about 0.1 ms of an 81.9 ms tick, about 0.13%, at 1000 players and 11,089 entities.

Type

• Bug fix
• Performance
• Refactor or cleanup
• New feature
• Docs or build

Checklist

• .\scripts\extract-patches.ps1 ran clean. A full re-extract also rewrote unrelated patches, so only this file is in the commit. git apply --check against .baseline succeeds.
• dotnet build VintageStory.slnx -c Release -p:EmbedPatchedFiles=true is green.
• Every vanilla edit has a // Stratum marker.
• No vanilla source committed.
• Tested on a real server start, not just compilation. scripts/smoke-test.ps1 reached RunGame with no fatal errors.

Notes

The table below is the workload those captures were taken on. It is not a before/after for this cleanup, and there is no pass bar on TPS.

Workload	Players	Chunks	Entities	1-minute TPS	Mean tick	p95 tick	Lowest 1s TPS
Regular world, lithosprobe-20260930-121338	1000	138256	11089	12.2	81.9 ms	119.7 ms	9
Small world, lithosprobe-20260930-125523	2000	512	2463	10.4	95.2 ms	226.0 ms	3

Related issues

Refs #315

Made with Cursor

[Zaldaryon]

Bootstrap applied every patch. Both Release builds passed with zero errors (171 warnings unembedded, 4 embedded). scripts/scenarios.ps1 passed 49/49; scripts/smoke-test.ps1 reached WorldReady without fatal errors. GitHub reports only the successful Discord notification check, with no build or test checks.\n\nPlease add the post-change profile results before re-review. The PR has the baseline numbers but explicitly says the after run is still missing. This is hot-path work, and the description sets a concrete pass bar: above 12.2 one-minute TPS on the same regular-world chunk and entity counts, above 20 TPS through 1800 bots on the small world, and a lower ShouldThrottleStratumEntityTick lock share. The functional checks above cannot establish those results.\n\nindev advanced to ece5d425 when #343 merged. Please rebase this branch onto current indev before requesting review again.

[leroysquad]

Two items from the review are still open.

Post-change profile numbers are not attached. This pass did not start a load test.

The branch is not rebased onto ece5d42. That would rewrite the published commit. GitHub reports the pull request mergeable against current indev.

[Pixnop]

/rebase

[github-actions]

Rebased perf/entity-tick-state-locks onto indev, replaying 1 commit(s) and dropping any merge commits.

72ad5c6ef33603c4042ed0828d4bade4b5ac9e82 -> 567eaa7624e983f4d21949bb27982da6c5972fa3

Your local branch is now behind the remote. Run git pull --rebase before pushing again.

[Pixnop]

The change is correct. I checked what could go wrong: all three places that insert a tick state exclude players, an entry that exists is still removed, each key is only touched by the thread that ticks that entity, and the despawn removals and the 300-tick reconcile are untouched. Tick intervals do not change. The patch applies, the build is green, 49/49.

What does not hold is the number. On .NET 10 a TryRemove on an absent key does take the stripe lock, you are right about that. But it costs about 10.5 ns, against about 2 ns for ContainsKey (measured here, single thread, 8000 entries). With 1000 players and 11089 entities each paying it once per tick, that is 0.1 ms out of an 81.9 ms tick: 0.13 percent, not 4.2. The 4.2 percent in your profile is the whole ShouldThrottleStratumEntityTick stack, and the likely cost inside it is the per-entity scan over players in the tier computation. That would be the PR worth writing.

One side effect: with default config the fourth call site always has an entry, because the excluded-domains block a few lines earlier inserts one for every non-player entity that gets that far. There the guard adds a lookup, and the insert-then-remove churn stays.

On Zaldaryon's two requests, which I agree with:

• Numbers. Either post the after-profile, or reword the description as a small cleanup with no measurable gain. I am fine merging it as the second.
• Rebase. Done, I ran /rebase. Rebasing a PR branch is the flow here (CONTRIBUTING: "Rebase your branch when indev moves"), and the command exists so nobody has to do it by hand. Pull with --rebase before pushing again.

One mechanical fix. The hunk you edited grew by 17 lines, but the four hunk headers after it kept their old new-file offsets: +1573, +1616, +1753 and +1818 should read +1590, +1633, +1770 and +1837. git apply tolerates it, so nothing breaks today, but the next person to regenerate that patch gets unrelated churn.

[Zaldaryon]

Requesting changes. The cleanup may be correct, but the PR's performance claim and pass bar are not supported by the cited profile.

The 4.2% sample is the full ShouldThrottleStratumEntityTick stack, not absent-key TryRemove alone. The reviewed estimate for the dictionary operation is about 0.1 ms of an 81.9 ms tick at 1000 players and 11,089 entities, roughly 0.13%. Another call site already has an entry and adds a ContainsKey lookup without removing the insert/remove churn.

The PR body has no after-profile for this head and explicitly leaves the after values blank. Either post a reproducible before/after run at the stated workloads and pass bars, or reword this as a small cleanup with no measured gain and remove the unsupported performance checkbox and pass bar. GitHub reports no checks. The previous local build, smoke test, and 49 scenarios were on 72ad5c6, not this head.

[Zaldaryon]

Since my review, #347 merged and advanced indev from ece5d42 to c5c3aee. Please rebase this branch onto current indev when you update the performance evidence, then request re-review.

[leroysquad]

The description is reworded as a cleanup with no measured gain. The 4.2% sample is the whole ShouldThrottleStratumEntityTick stack. The reviewed estimate for the absent-key TryRemove is about 0.1 ms of the 81.9 ms tick, about 0.13%, at 1000 players and 11,089 entities. The performance checkbox and the TPS pass bar are off the description.

The four hunk new-file offsets after the edited region are +1590, +1633, +1770, and +1837 in 5e90950. git apply --check against the baseline file succeeds.

No after-profile. The bars above 12.2 one-minute TPS on the regular world and above 20 TPS through 1800 bots on the small world were not rerun. indev is at c5c3aee after #347 merged. This branch was not rebased.

[leroysquad]

Rebased onto current indev (c5c3aee). New head: 1b8b2ce.

[Zaldaryon]

Re-reviewed exact head 1b8b2cea50f73cc23f011e872fcc5591a2a9b417. The PR now describes this as a small cleanup, removes the unsupported performance attribution and pass bar, and matches the evidence. No blocking finding remains.

Validation on this head used the repository-pinned ILSpy 10.1.0.8386 on both Windows and Linux. Both bootstraps applied all patches. Linux make build, smoke (WorldReady), and scenarios passed 49/49. Windows unembedded and embedded Release builds passed with 0 errors; smoke reached WorldReady, and scenarios passed 49/49. git diff --check origin/indev...refs/review/344/head passed. GitHub reports no CI checks; the base is current.