perf(gb-bot): replace tileNumber/isHonor string parsing with lookup tables#106
Closed
Arbousier1 wants to merge 4 commits into
Closed
perf(gb-bot): replace tileNumber/isHonor string parsing with lookup tables#106Arbousier1 wants to merge 4 commits into
Arbousier1 wants to merge 4 commits into
Conversation
…ables GbRoundSupport.tileNumber() allocated a new String via name().substring(1,2) and parsed it with Integer.parseInt on every call. In the GB bot discard decision hot loop (GbBotDecisionService.discardPreference), tileNumber and isHonor are called once per candidate tile per hand tile, producing repeated short-lived String allocations that escape into parseInt and cannot be escape-analyzed away. Precompute two ordinal-indexed arrays once at class init: - TILE_NUMBERS: the 1-9 value for suited tiles, 0 for honors/flowers/unknown - TILE_HONORS: the EAST..RED_DRAGON honor range as a boolean mask tileNumber() and isHonor() now perform a single array load instead of string allocation + parsing. Behavior is unchanged: the same control flow (null/flower/honor guard) is preserved, and the lookup values are computed from the original expressions during static init. Target profile: gb-bot (must_pass: gb.duplicate.time, gb.mixed.time, gb.unique.time).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Arbousier1
added
performance-ab
…allocation bestDiscardChoice allocated a 45-element GbTingResponse[] on every call to memoize ting evaluations by tile ordinal. On the duplicate-hand hot path measured by GbBotDecisionBenchmark.duplicateHand the memo only ever writes a single slot, so the array allocation dominates the per-iteration allocation footprint without delivering reuse benefit. Replace the per-call allocation with a ThreadLocal buffer that is cleared via Arrays.fill at the start of each call. Semantics are unchanged: the array is indexed by MahjongTile.ordinal(), fully reset before use, and never escapes the method. Each worker thread gets its own buffer, so there is no contention or cross-thread visibility concern. This eliminates ~376 bytes of heap allocation per discard decision (object header + 45 pointer slots), which should push the duplicateHand improvement above the 3% AB gate threshold that the lookup-table-only commit could not clear on its own.
…er-call allocation" This reverts commit 0a3e879.
… hot path suggestedDiscardIndex only needs the discard index, but went through bestDiscardChoice which always allocates a DiscardChoice record (int index, long readyScore, int discardPreference) just to immediately extract .index() and discard the record. On the duplicateHand hot path measured by GbBotDecisionBenchmark.duplicateHand this is a per-iteration allocation that the caller never benefits from. Add a bestDiscardIndex variant that returns primitive int directly, avoiding the record allocation. The loop body is identical to bestDiscardChoice; only the return shape differs. bestDiscardChoice is retained unchanged for the reaction/kan callers that need the full (index, readyScore, discardPreference) triple. This is a pure allocation elimination — no behavior change, no ThreadLocal overhead, no shared mutable state. Pairs with the lookup-table optimization in the previous commit to push duplicateHand above the 3% AB gate threshold.
Collaborator
Author
|
Closing after 3 inconclusive AB attempts (lookup tables, ThreadLocal tingMemo reverted, bestDiscardIndex). The gb-bot hot path's allocation savings are within JIT optimization noise for this benchmark. Will revisit with a different approach. |
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
Replace per-call String allocation +
Integer.parseIntparsing inGbRoundSupport.tileNumber()and the ordinal-range check inisHonor()with two ordinal-indexed lookup tables computed once at class init.Motivation
GbRoundSupport.tileNumber()is called inside the GB bot discard-decision hot loop (GbBotDecisionService.discardPreference). Every call allocates a new String viatile.name().substring(1, 2)and parses it withInteger.parseInt. Because the String escapes intoparseInt, escape analysis cannot eliminate it.isHonor()performs a two-ordinal comparison on every call.discardPreferenceis invoked once per hand tile inbestDiscardChoice, and internally iterates every candidate tile callingisHonor+tileNumbermultiple times per candidate. For a 14-tile hand this produces dozens of short-lived String allocations per decision.Change
Precompute two
private static finalarrays indexed byMahjongTile.ordinal():TILE_NUMBERS[ord]— the 1-9 value for suited tiles (M1-M9, P1-P9, S1-S9, including red-fives), 0 for honors/flowers/unknown.TILE_HONORS[ord]—trueforEAST..RED_DRAGON,falseotherwise.tileNumber(tile)now returnsTILE_NUMBERS[tile.ordinal()](after the same null/flower/honor guard), andisHonor(tile)returnsTILE_HONORS[tile.ordinal()].Behavior
Unchanged. The lookup values are computed from the original expressions during static initialization. The same control-flow guard (
null || isFlower || isHonor) is preserved intileNumber. The only edge-case difference:tileNumber(MahjongTile.UNKNOWN)now returns 0 instead of throwingNumberFormatException— butUNKNOWNis never passed totileNumber(all callers guard withisFlower()/isHonor()or only pass suited tiles from hands/walls).Target profile
gb-bot— must_pass:gb.duplicate.time,gb.mixed.time,gb.unique.time.