perf(serverareas): finish ServerWater chain sweep — SaveArea + ServerUnloadArea#272
Merged
Merged
Conversation
…rverUnloadArea PR #270 introduced Area\FirstWater + ServerWater\NextWater linked-list infrastructure and converted the per-tick underwater-damage check in GameServer.bb to walk it. SaveArea and ServerUnloadArea were left deferred for a follow-up — both still iterated the global `For W.ServerWater = Each ServerWater` collection and filtered by `If W\Area = A`, scanning every water in the world to find the subset owned by one Area. This change converts the three remaining sites: * ServerUnloadArea (1 walk): replaces a global after-cursor walk that filtered by Area with a direct chain walk over A\FirstWater. The body still Deletes the current node (legacy module — no GC), so the next link is captured into WNext before Delete. Adds an explicit A\FirstWater = Null after the loop for safety, even though A is Deleted immediately afterward. * SaveArea (2 walks): replaces a count walk + write walk pair, each scanning the whole global collection. The new code walks A\FirstWater twice in succession (count, then write). Chain head-insert order in ServerLoadArea is reverse of file order; ServerLoadArea reads N records and does not depend on order, so the file contents on disk may legitimately differ — but the round-trip behavior is identical. Performance: O(global_waters) → O(this_area_waters) per call. SaveArea is called from the area-save path (admin / shutdown), so the per-call win is modest, but ServerUnloadArea runs whenever an Area is unloaded and the old form scaled with the total water count across all loaded Areas. Also updates the now-stale doc comment on the head-insert in ServerLoadArea — the "SaveArea path still walks Each ServerWater" line became false. The global `For Each ServerWater` collection still exists and is still the source of truth for memory; this PR just stops two callsites from using it as a discovery iterator. Verification: - Full compile clean (Server, Client, GUE, Project Manager, all Tools). - All 19 tests pass. - Grep confirms zero `For W.ServerWater = Each ServerWater` walks remain in ServerAreas.bb. A focused unit test would require substantial inline stubbing of the Area type's dependencies (AreaInstance, server globals); skipping is documented in the PR body. Strongest available evidence: static (compile validates Field references; grep proves removal) plus identical-pattern reference to GameServer.bb:736-758 which shipped in PR #270. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f66b1f6fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+393
to
+394
| W = A\FirstWater | ||
| While W <> Null |
There was a problem hiding this comment.
Restricting ServerSaveArea to A\FirstWater drops valid waters that exist in the global ServerWater set but were never linked into that chain. In the editor path (src/GUE.bb:2915-2917), new waters are created with SW\Area = CurrentArea but no A\FirstWater/SW\NextWater link is established, so ZoneSave() (src/GUE.bb:9148) now writes a water count that excludes those records and they disappear on reload. Before this change, the Each ServerWater + If W\Area = A sweep preserved them.
Useful? React with 👍 / 👎.
This was referenced May 26, 2026
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
1 participant
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
PR #270 introduced the per-Area
ServerWaterlinked-list (Area\FirstWater+ServerWater\NextWater) and converted the per-tick underwater-damage check inGameServer.bbto walk it, leaving the SaveArea / ServerUnloadArea sites as a deferred follow-up. This PR finishes that sweep.Three sibling sites in
src/Modules/ServerAreas.bbstill iterated the globalFor W.ServerWater = Each ServerWatercollection and filtered byIf W\Area = A. After this change, no per-frame or per-save code path scans the global water collection to find an Area's subset.Non-technical
The server keeps a list of every body of water in the world. Until PR #270 every per-frame underwater check scanned that whole list and discarded the records that belonged to a different area. PR #270 fixed the per-frame check; this PR finishes the job for the area-save and area-unload paths, so neither of those has to scan every water in the world either.
Technical changes
src/Modules/ServerAreas.bbonly — one file, three sites:ServerUnloadArea(1 walk) — replaces a global after-cursor walk filtering byAreawith a direct chain walk overA\FirstWater. Body stillDeletes the current node (legacy module — no GC), soWNext = W\NextWateris captured beforeDelete W. SetsA\FirstWater = Nullafter the loop for safety even thoughAisDeleted immediately afterward.SaveAreacount walk — chain walk replaces the global walk + filter. O(global_waters) → O(this_area_waters).SaveAreawrite walk — chain walk replaces the global walk + filter. Same complexity win. Output ordering on disk may differ (head-insert is LIFO relative to file order), butServerLoadAreareads N records and is order-insensitive, so the round-trip behavior is identical.Also updates a now-stale doc comment in
ServerLoadArea— the line "SaveArea path still walks Each ServerWater" became false.Acceptance criteria
For W.ServerWater = Each ServerWaterwalks remain inServerAreas.bb(grep-verified).Each ServerWatercollection still exists (allocation + Delete sites use it).Test plan
compile.batclean (full, including Tools)test.bat— 19/19 passEach ServerWaterwalks in ServerAreas.bbFocused unit test was infeasible without substantial inline-stubbing of the
Areatype's dependencies (AreaInstance, server globals). Strongest available evidence: static (compile validates Field references; grep proves removal) plus mechanically-identical pattern reference toGameServer.bb:736-758shipped in PR #270.Trade-offs / deferred follow-ups
.datfiles between pre-PR and post-PR saves. Acceptable.For Each ServerWatercollection is still iterated at allocation (New ServerWaterregisters;Deleteunregisters). Removing the global collection entirely would be a follow-up: it would require routing everyServerWaterlifecycle event throughA\FirstWater, which currently has no value (no per-water lookups exist outside the chain-walking sites covered here and in perf(serverwater): O(actors*global_waters) → O(actors*per_area_waters) in per-tick underwater check #270).DeltaTimeseed (1-liner), remaining 15 stub docs/modules/*.md files.🤖 Generated with Claude Code