Repository navigation
feat(world): cut the per-tick cost of a populated zone - #566
Merged
Merged
Conversation
Five issues on the zone tick hot path, measured before and after on one machine. asobi#558 - sync_spatial_grid derived its removals with `maps:keys(Old) -- maps:keys(New)`. `--` is O(N*M), so with a spatial grid configured a zone holding N entities paid O(N^2) key comparisons every tick to almost always produce `[]`. Profiled on an inert 2,000-entity zone, `erlang:'--'/2` was 13.4% of the whole tick at ~500us a call. A fold with `is_map_key/2` is O(N). Tick wall time at 2,000 entities: 1,533us -> 910us. asobi#559 - every occupied zone cast `touch_zone` at the zone manager on every tick, and the manager rebuilt a map per cast. At tick_rate 50 that is 20 casts per second per occupied zone through the one gen_server that also answers `ensure_zone/2` on the join and NPC-crossing path, where the crossing budget is 1s and a timeout costs the crossing. The stamps move into a public ETS table zones write directly, and the reap sweep selects expired coords out of it. The cast stays for a zone started outside a manager, and the write is guarded: the table dies with the manager, which the instance supervisor stops before the zones. asobi#560 - the ticker asked the manager for the active set on every tick and got `ets:tab2list/1` back, plus a gen_server call when the manager was addressed by pid. The ticker now owns the set: the manager pushes a zone in when it opens, and the ticker monitors it, so a reaped or crashed zone drops out on its own DOWN. hot/cold become maps, which also retires the per-tick `lists:uniq/1`. Together with #559 the zone manager is off the tick path entirely. asobi#561 - `cold_tick_divisor = 0` stops ticking an idle zone altogether rather than ticking it 1-in-10. Nothing has to be caught up on warm-up: a zone is only demoted when its timer wheel and respawn queue are empty, and both are driven by the wall clock the resumed tick passes them. A zone with subscribers now declines `reap` and re-stamps itself - already true in effect via the tick, but a zone that stops ticking stops stamping. Default stays 10. ADR 0021. asobi#557 - `zone_tick/2` may return a fourth value saying what changed: `#{changed => #{Id => Entity}, removed => [Id]}`, merged onto the map it returned. A Lua script declaring it stops this bridge decoding the whole entities table, and the entities nobody named stay the same TERMS, so compute_deltas and the grid sync settle each with a pointer comparison instead of walking its fields. Measured with an inert script moving three entities under `owned`: 2,000 entities 10.5ms -> 5.9ms per tick, and the reductions the zone process itself burns 200,659 -> 1,208. The encode in the other direction is unchanged and still O(N); ADR 0022 says why that is a separate decision. Closes #557 Closes #558 Closes #559 Closes #560 Closes #561
Architecture guardian, security reviewer and code reviewer, in that order.
Two script-reachable ways to take out a zone, four silent-revert paths, and a
perf claim that was wrong in the common case.
Script-reachable, both introduced by the previous commit:
- `decode_dirty/2` guarded `is_tuple(Ref)`, which is not a Luerl-table check.
Luerl represents every non-scalar as a record - #tref, #funref, #erl_func,
#erl_mfa, #usdref - so `return entities, zone_state, false, function() end`
raised out of decode_to_map/2, and asobi_zone calls zone_tick/2 with no
try/catch: the zone died with everyone in it. Now checks the record tag, and
wraps the decode anyway because a #tref can be recursive.
- `narrow_changed/2` passed the whole decoded `changed` map to the bounded
formatter, which renders its argument with ~0p BEFORE truncating - measured
at ~10s on the zone process for a 32MB script binary, outside bounded_eval's
wall clock and invisible to max_heap_size. Three of those per limiter window
is a zone that never ticks again, with no crash to attribute it to. It now
logs a count plus the first offending key clipped to 64 bytes, so the bound
is on the way in, and bound_debug_term/1 uses ~P so the depth cut happens
during the render rather than after it.
Silent-revert paths, all four now reported with `bad_zone_dirty`. Each one
produced the same symptom - every entity in the zone stops moving, forever,
with no error - and three are one line from an idiom the guide itself shows:
- a typo'd key (`#{chnaged => ...}`) read as an empty declaration;
- `changed` built as a Lua array was normalised to #{} in the bridge, BEFORE
core could complain;
- a table declaring neither half became a well-formed empty declaration,
because decode_to_map/2 coerces an array-shaped decode to #{}. It now falls
back to the full decode, which is the only reading that cannot freeze a zone;
- narrow_ids/1 dropped bad ids silently while narrow_changed/2 logged.
Robustness:
- narrow_coords/1 filters and deletes a junk stamp key instead of
function_clausing the manager, which is transient under a ONE_FOR_ALL
supervisor - the crash restarts the zone supervisor, the ticker and the world
server. The code it replaced swept an unknown key harmlessly, so failing loud
was a regression. touch_zone/2 and release_zone/2 guard at the API edge.
- cold_tick_divisor takes the documented default on a bad value, loudly, rather
than folding into the never-tick regime. Landing in ADR 0021's semantics by
typo is not something a world should be able to do.
- stamp_active/2 answers `stamp_failed` rather than `ok`, and touch_manager/1
falls back to the cast, so a wrong handle degrades to the pre-#559 path
instead of to nothing.
Reverted from the previous commit: sync_spatial_grid/3's entity-identity match.
On a miss it is a map `=:=` that can only fail - O(entity) where the old code
was O(1) - and the miss path is every game that does not declare plus every Lua
game on any tick carrying input. compute_deltas/2 is where sharing pays and it
already tested identity.
Docs corrected against what the code does:
- ADR 0022's "inherent to any dirty contract" was overstated and its own cited
prior art contradicts it: AzerothCore's dirty flags govern replication, so a
missed flag costs an update; here a missed declaration REVERTS the mutation.
The claim is narrowed and the consequence stated - the reporting has to be
better than the prior art's, not equal.
- ADR 0016 gains an "Amended by later work" section: decision 3 is superseded
by #560, decision 1 is amended by ADR 0021 decision 4, and its file:line
citations are marked historical.
- ADR 0021 records the race the reap guard actually closes (release_zone
backdates, and a sweep landing before the next re-stamp stopped a watched
zone) and the zone-timer-wheel gap that would make divisor 0 broadly safe.
- The guide says its numbers assume a tick with no player input, because
handle_input rebuilds the entity map and undoes the sharing.
- Both Lua examples guard `e.hp`; the unguarded compare raises on any entity
without the field, which is most of them.
- Measurements re-taken on the final code, median of three: 9,292 -> 4,569us
and 200,466 -> 1,209 reductions at 2,000 entities under `owned`.
Tests: 19 added. The hostile-fourth-value cases were verified by reverting each
fix and confirming the specific case fails - the first version of them passed
against the unfixed code, because the mode rode on `game_state` (passed to
Luerl unencoded, so it never reached the script) and because `print` is
stripped from the sandbox. Also covered: sync_spatial_grid's movement branch,
set_zones releasing dropped monitors, the stamp teardown path, and
cold_tick_divisor = 0 surviving Lua config parsing - that last one is a single
token that could be reverted with every test still green.
… run `rebar3 ct` twice in a row failed the second time: `ops_board_listing` counted 6 entries where init_per_suite seeds 3, and `get_top_empty` found scores on a board it expects empty. The board ids were built with `erlang:unique_integer([positive])`, which is unique within ONE runtime instance. Every `rebar3 ct` is a new instance, so the second run hands out the same low integers, lands on the same board id, and reads the first run's rows - the local database persists between runs while CI gets a fresh one, so this only ever breaks local developers, and it looks like a regression in whatever work happened to be in flight. `asobi_test_helpers:unique_id/1` exists for exactly this and its documentation says so, citing asobi#357 where the same thing was found before. The suite simply was not using it. Swept the other `erlang:unique_integer/1` uses in test/. The rest are temporary filenames and ETS keys, where per-run uniqueness is all that is needed. Two write database rows - asobi_tournament_api_SUITE's board id and asobi_economy_SUITE's currency - and both were checked over repeat runs rather than assumed: neither fails, because nothing counts entries on the tournament board and the currency is scoped to a freshly registered player. Left alone rather than changed on suspicion. Verified with two consecutive full `rebar3 ct` runs, 390 passing each time.
🟡 Code Coverage — 76.8%9364 of 12185 lines covered. 🟡 ELP Lint — 192 warnings212 diagnostics found. See job logs for details. ✅ ELP eqWAlizeNo diagnostics. |
CI's eqWAlize job runs with EQW_SCOPE=diff against origin/main: it checks the modules a PR touched and requires zero errors in them, pre-existing ones included. My local gate was `eqwalize-all` compared against a baseline total, which showed no NEW errors and hid the twelve already sitting in four modules this branch happens to touch - one of them reached only by a comment edit. All twelve, by the idiom in docs/eqwalizer-idioms.md rather than a suppression: - asobi_telemetry: `?LOG_DEBUG` is the last expression of a function spec'd `-> ok`, and the macros return term(). Ends with `ok`. - asobi_zone_cold_tests: `sys:replace_state/2` hands its fun a term(), so the update needs a narrowing clause rather than a map update on an unknown shape. - asobi_lua_world_tests: `zone_tick/2` returns the zone state as term() - the callback contract is game-defined - so a test reading fields out of it narrows first. - asobi_zone_tests: the four sites that deliberately hand `player_input/4` a spec-violating seq, proving the zone neither acks nor function_clauses on one. The value genuinely crossed a wire, so `dynamic()` is the honest type; it is confined to one helper, enters `player_input/4` and never leaves it. eqwalize-all total: 197, from 212 on origin/main.
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.
Five issues on the zone tick hot path, measured before and after on one machine.
asobi#558 -
--on entity keys, every ticksync_spatial_grid/3derived removals withmaps:keys(Old) -- maps:keys(New).--is O(N*M), so with a spatial grid configured a zone holding N entities paid O(N^2) key comparisons every tick to almost always produce[]. Profiled on an inert 2,000-entity zone,erlang:'--'/2was 13.4% of the whole tick at ~498us a call. A fold withis_map_key/2is O(N). Tick wall time at 2,000 entities: 1,533us -> 910us.asobi#559 - the zone manager was on the per-tick path
Every occupied zone cast
touch_zoneat the manager on every tick, and the manager rebuilt a map per cast. Attick_rate = 50that is 20 casts/sec/zone through the one gen_server that also answersensure_zone/2on the join and NPC-crossing path, where the crossing budget is 1s and a timeout costs the crossing.Stamps move to a public ETS table zones write directly; the reap sweep selects expired coords out of it. The cast stays for a zone started outside a manager. The write is guarded: the table dies with the manager, and the instance supervisor stops the manager before the zones, so an unguarded
ets:insertwould turn every world teardown into a burst ofbadargcrashes.asobi#560 - the ticker copied the whole zone table every tick
It asked the manager for the active set on every tick and got
ets:tab2list/1back, plus agen_servercall when the manager was addressed by pid. The ticker now owns the set: the manager pushes a zone in when it opens, and the ticker monitors it, so a reaped or crashed zone drops out on its ownDOWN- which is the self-healing the old partition-against-the-list gave for free.hot/coldbecome maps, retiring the per-ticklists:uniq/1.With #559, the zone manager is off the tick path entirely.
asobi#561 -
cold_tick_divisor = 0An idle zone is not ticked at all. Nothing has to be caught up on warm-up, and that is a property of
zone_idle/1rather than an omission: a zone is only demoted when its timer wheel and respawn queue are both empty, and both are driven by the wall clock the resumed tick passes them.A zone with subscribers now declines
reapand re-stamps. Mostly a statement of emergent behaviour, but it also closes a real race at every divisor:release_zone/2backdates the stamp the moment a zone empties, and a sweep landing before the next re-stamp stopped a watched zone.Default stays 10. ADR 0021.
asobi#557 -
zone_tick/2may declare what changed{Entities, ZoneState, Busy, #{changed => #{Id => Entity}, removed => [Id]}}, merged onto the map the callback returned. A Lua script declaring it stops the bridge decoding the whole entities table, and the entities nobody named stay the same terms, socompute_deltas/2settles each with a pointer comparison instead of walking its fields.Median of three runs, inert script moving three entities,
lua_vm_mode = owned:The reduction column is the one that decides whether a zone keeps up with its tick budget, and it is the stable one.
The encode in the other direction is unchanged and still O(N). Removing it needs the VM to hold the entities table across ticks and asobi to push only its own mutations, which means tracking dirtiness across eight entity-writing sites where a missed mark is an entity that silently stops replicating. ADR 0022 records that as a separate decision.
Review gate
Architecture guardian -> security reviewer -> code reviewer, then a second commit working every finding. Highlights:
decode_dirty/2guardedis_tuple/1, which is not a Luerl-table check - Luerl represents every non-scalar as a record, soreturn entities, zone_state, false, function() endraised on a callback invoked with no try/catch and killed the zone. Now checks the record tag, with a try/catch for recursive tables.bounded_evaland invisible tomax_heap_size. The bound moved to the way in.bad_zone_dirty. Each produced the same symptom - every entity in the zone frozen, forever, no error - and three were one line from an idiom the guide itself showed.narrow_coords/1filters and deletes rather thanfunction_clause-ing the manager underone_for_all.sync_spatial_grididentity match: on a miss it is a map=:=that can only fail, and the miss path is the default.Tests
+19. The hostile-fourth-value cases were verified by reverting each fix and confirming the specific case fails - the first version of them passed against the unfixed code, because the mode rode on
game_state(handed to Luerl unencoded, so it never reached the script) and becauseprintis stripped from the sandbox.The third commit fixes a CT failure that was not flake:
asobi_leaderboard_api_SUITEbuilt board ids witherlang:unique_integer/1, unique within one runtime instance, so a secondrebar3 ctreused the ids and inherited the first run's rows.asobi_test_helpers:unique_id/1exists for this and says so, citing asobi#357.Gate status
fmt --check,xref,dialyzer,elp lintclean on changed files;eqwalize-all209 vs 212 on main (no new);ex_docunchanged at 6; drift checks pass; eunit 2,398/0; CT 390/0 on two consecutive runs.Known, not fixed here
asobi_world_zone_integration_tests:world_input_crossing_touches_only_ring_deltapasses spuriously:find_player_pid/1readspg:get_members(nova_scope, {player, PlayerId}), that module never registers the group, and in the full suite it borrows a live process another module left behind. Run alone it correctly reports 0 subscribers and fails. Unrelated to this change and it alters what the test asserts, so it wants its own PR.Closes #557
Closes #558
Closes #559
Closes #560
Closes #561