perf: layout/content fingerprint split + bot display-name cache (PR-D) - #129
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Arbousier1
added
performance-ab
…spawn on re-arrangement Spark profile (spark.lucko.me/8yCLAsdfdV) showed applyRenderPrecompute as the largest mahjong hotspot (27%): every hand re-arrangement (deal/draw/discard) shifted all tile positions, which changed every tile fingerprint (point was part of the content fingerprint) and forced a full remove+respawn of 26+ regions per seat while reconcile (teleport) was almost never reached. - handPrivateTileFingerprint/handPublicTileFingerprint no longer mix tile positions into the content fingerprint; new privateHandLayoutFingerprint / publicHandLayoutFingerprint track the layout separately. - TableRegionDisplayCoordinator keeps per-region appliedLayoutFingerprints; a layout-only change now enters the render path and is satisfied by reconcile (entity teleport) instead of respawn, with respawn retained as a safe fallback. - cache rendered bot display names (i18n) per player+locale, invalidated on participant changes, cutting per-tick displayName rendering.
Arbousier1
force-pushed
the
pr-d-layout-reconcile
branch
from
August 5, 2026 03:56
f7e8875 to
20aeb8d
Compare
…mplete The first A/B run showed a 39.8% regression on region.apply-deferred because the per-seat layout fingerprints enumerated every tile point (320-capacity builder, ~150 FNV mixes each) on every apply, even on the short-circuit path. Tile coordinates are fully determined by (seat, hand size, tile index, selected indices) — all already present in the per-tile content fingerprints once hand size is added to the public tile fingerprint — so the layout fingerprint only needs to carry the hand structure: - handPublicTileFingerprint now includes hand size, making public hand content fingerprints layout-complete again; - privateHandLayoutFingerprint/publicHandLayoutFingerprint shrink to a few structural FNV fields (computed once per seat per apply, never enumerating coordinates); - both short-circuit paths drop the per-region layout lookup and go back to a pure content-fingerprint comparison; - the reconcile-vs-respawn decision on the update path keeps the layout gate: structure unchanged -> reconcile (teleport in place), structure changed (hand grew/shrunk) -> respawn. Net effect: the every-tick apply fixed cost returns to baseline while the real-game win (layout-only changes no longer poison every tile fingerprint) is preserved.
The A/B rerun (dedicated runner, 8 iterations) shrank the apply-deferred regression from -39.8% to -6.1% and flipped complete-fingerprints to +3.04% PASS_OPTIMIZED, but the remaining regression tracks the extra hand-size field added to handPublicTileFingerprint (one extra FNV mix per tile per apply). Tile index and hand size are packed into a single injective field (tileIndex * 64 + handSize, both < 64 in every legal game state), so the public tile fingerprint keeps exactly the pre-change field count and the every-tick short-circuit fixed cost returns to baseline while the layout-complete content semantics are preserved.
The A/B rerun still showed a -7.25% apply-deferred regression. The benchmark drives the planning coordinator with a zero budget, so every apply only pays the enqueue cost: fingerprint computation plus the per-seat layout fingerprint precomputation, which is never consumed on the short-circuit path. Since tile coordinates are fully determined by (seat, hand size, tile index, selected indices) — all covered by the content fingerprints — the layout structure fingerprint reduces to the hand size itself. Use the already-computed handSize value as the fingerprint (long) instead of calling the fingerprint service, removing the last per-apply computation added by the layout/content split. The reconcile-vs-respawn gate keeps comparing the applied hand size with the current one. The now-unused privateHandLayoutFingerprint/publicHandLayoutFingerprint methods are removed.
…fingerprint Shaves the last per-apply arithmetic (a multiply) off the short-circuit path; field count and injectivity are unchanged.
Region fingerprints are compared per region key (hand-private:WIND:index), so the type prefix field is redundant and costs one FNV mix per tile per apply. Removing it from the hand tile fingerprints restores the short-circuit fixed cost to below baseline on the apply benchmarks.
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
Spark profile (https://spark.lucko.me/8yCLAsdfdV) shows
applyRenderPrecomputeat 27% of the main thread. Root cause: tile content fingerprints mixed in raw tile positions, so every hand re-arrangement (deal/draw/discard → hand size changes → all points shift) changed every tile fingerprint and forced full remove+respawn of 26+ regions per seat, whilereconcile(teleport) was sampled near zero.TableRegionFingerprintService+TableRegionDisplayCoordinator):point; newprivateHandLayoutFingerprint/publicHandLayoutFingerprinttrack positions separately (per-seat, once per apply).appliedLayoutFingerprints; short-circuit now requires content fingerprint AND layout fingerprint to match. Layout-only changes enter the render path and are satisfied byreconcile(entity teleport / smooth movement) instead of region respawn; respawn stays as the safe fallback.MahjongTableSession): rendered i18n bot names cached per player+locale, invalidated on participant changes (removeBot/replaceBotWithPlayer/removePlayer/clearBotNamesForLifecycle).displayNamewas ~2000 samples of the profile (per-tick i18n rendering viaLocalizedMessages).Test plan
./gradlew compileJavagreen locally