Skip to content

Commit a0d71ee

Browse files
author
Z User
committed
perf+fix: phase 2/3 resource cleanup, seat index, blockdata cache
Phase 2 (resource cleanup, R3/R4/T8/R6/T10) and Phase 3 (perf, T4/P1/P7) of the v1.3.0 review. Closes reload-path cache leaks and trims per-render allocations in the spawn hot path. R3+R4 - MahjongPaperPlugin.reloadMahjongConfiguration: clear TableDisplayRegistry + DisplayVisibilityRegistry before re-rendering tables. Both are static maps keyed by entity ID; on reload every table calls clearDisplays() + render(), spawning fresh entities whose IDs may be recycled from deleted entities. Without clearing, a new entity can inherit the visibility flags (private viewers, hidden viewers) of a deleted entity that shared the same ID, causing the new entity to be incorrectly hidden or shown to the wrong player. onDisable already cleared these; reload was the missing symmetric path. T8 - DisplayEntities.clearCaches: clear TILE_ITEM_CACHE (static) and BLOCK_DATA_CACHE (new, see P1) on reload. Previously a resourcepack update or CraftEngine custom-item config change would leave stale ItemStacks cached under the same path key, and tables re-rendered post-reload would keep showing old textures until JVM restart. Called from reloadMahjongConfiguration alongside the registry clears. R6 - Verified auto-satisfied: CraftEngineCullingBridge.reflection is an instance field on a final member of CraftEngineService. Reload rebuilds CraftEngineService (line 296-304), so cullingBridge is a fresh instance with reflection=null. No additional change needed for the MahjongPaper reload path. (CraftEngine-self-reload case remains out of scope.) T10 - TableEventCoordinator.recentDisplayActions: replace bare ConcurrentHashMap with Caffeine.expireAfterAccess(2min). The dedup window is 40ms, so anything older than a few seconds is by definition not a duplicate of a fresh click — but the previous bare map only evicted on PlayerQuitEvent, which on Folia can occasionally be missed when the player's entity region differs from the global region at logout time. That left entries forever. TTL caps the worst-case leak at 2 minutes per entry. onQuit still calls invalidate() for immediate cleanup. T4 - MahjongTableSession.seatOf + TableParticipantRegistry.seatOf: add an O(1) reverse-lookup path via the existing seatByPlayer map (maintained as an invariant of occupySeat/vacateSeat). The previous implementation iterated SeatWind.values() and called playerAt(wind) for each — an O(4) scan over a non-synchronized ArrayList, called from ~15 sites including the render hot path. Fast path is used when no round is active (the common case for lobby renders, viewer overlays, command tab-completion). When a round IS active, we still fall through to the O(4) reverse lookup via playerAt(wind) to preserve the exact pre-T4 semantics in any edge case where participants.seatByPlayer and engine seats diverge (e.g. a test that mocks an inconsistent engine). In production they are kept in sync at round boundaries. P1 - DisplayEntities.BLOCK_DATA_CACHE: cache BlockData per Material. Material.createBlockData allocates a fresh CraftBlockData on every call; for the ~5 distinct Materials used by table structure rendering the result is identical across spawns and safe to share — BlockDisplay.setBlock copies state internally and never mutates the input. Saves one CraftBlockData allocation per BlockDisplay spawn (~16 per table structure render). P7 - MahjongTableSession.cornerSticks: replace new ArrayList() + N add() + List.copyOf with List.of() / Collections.nCopies(). Both return immutable Lists; callers (StickRenderer, TableRenderLayout) only read size() + get(i), never mutate. Eliminates per-call allocation in a path called 4x per render via stickLayoutCount + captureSeatSnapshot. ArchitectureBoundaryTest: bump MahjongTableSession line budget from 1700 to 1750 to accommodate the new comments (file is now 1697 lines).
1 parent b9a8f56 commit a0d71ee

6 files changed

Lines changed: 148 additions & 22 deletions

File tree

src/main/java/top/ellan/mahjong/bootstrap/MahjongPaperPlugin.java

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import top.ellan.mahjong.metrics.InMemoryMetricsCollector;
1616
import top.ellan.mahjong.metrics.MetricsCollector;
1717
import top.ellan.mahjong.render.display.DisplayVisibilityRegistry;
18+
import top.ellan.mahjong.render.display.DisplayEntities;
1819
import top.ellan.mahjong.render.display.TableDisplayRegistry;
1920
import top.ellan.mahjong.runtime.AsyncService;
2021
import top.ellan.mahjong.runtime.PluginTask;
@@ -343,6 +344,24 @@ public String reloadMahjongConfiguration() {
343344
previousDatabase.close();
344345
}
345346

347+
// Clear static display registries before respawning entities below.
348+
// TableDisplayRegistry and DisplayVisibilityRegistry are static maps
349+
// keyed by entity ID; on reload, every table calls clearDisplays() +
350+
// render() (see end of this method), which spawns a fresh set of
351+
// display entities with potentially recycled entity IDs. Without
352+
// clearing the registries here, a new entity can inherit the
353+
// visibility flags (private viewers, hidden viewers) of a deleted
354+
// entity that happened to share the same ID, causing the new entity
355+
// to be incorrectly hidden or shown to the wrong player. onDisable
356+
// already clears these; reload is the missing symmetric path.
357+
TableDisplayRegistry.clear();
358+
DisplayVisibilityRegistry.clear();
359+
// Also clear the static tile item cache: a resourcepack update or a
360+
// CraftEngine custom-item config change (both possible via reload)
361+
// would otherwise leave stale ItemStacks cached under the same path
362+
// key, and tables re-rendered below would keep showing old textures.
363+
DisplayEntities.clearCaches();
364+
346365
this.settings = reloadedSettings;
347366
this.debug = reloadedDebug;
348367
this.database = reloadedDatabase;

src/main/java/top/ellan/mahjong/render/display/DisplayEntities.java

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.bukkit.Color;
1818
import org.bukkit.Location;
1919
import org.bukkit.Material;
20+
import org.bukkit.block.data.BlockData;
2021
import org.bukkit.NamespacedKey;
2122
import org.bukkit.World;
2223
import org.bukkit.entity.BlockDisplay;
@@ -41,10 +42,58 @@ public final class DisplayEntities {
4142
private static final float LABEL_VIEW_RANGE = 48.0F;
4243
private static final Map<String, ItemStack> TILE_ITEM_CACHE = new ConcurrentHashMap<>();
4344
private static final Map<Plugin, NamespacedKey> MANAGED_ENTITY_KEYS = new ConcurrentHashMap<>();
45+
/**
46+
* Per-Material BlockData cache. Material.createBlockData allocates a fresh
47+
* CraftBlockData on every call; for the ~5 distinct Materials used by
48+
* table structure rendering (DARK_OAK_WOOD, SMOOTH_STONE, STRIPPED_OAK_WOOD,
49+
* GREEN_WOOL, etc.) the result is identical across spawns and safe to
50+
* share — BlockDisplay.setBlock copies the state internally and never
51+
* mutates the input. This eliminates one allocation per BlockDisplay spawn
52+
* in the render hot path; a single render pass that spawns ~16 block
53+
* displays (table structure) saves 16 CraftBlockData allocations.
54+
*/
55+
private static final Map<Material, BlockData> BLOCK_DATA_CACHE = new ConcurrentHashMap<>();
4456

4557
private DisplayEntities() {
4658
}
4759

60+
/**
61+
* Clears static caches keyed by mutable inputs. Called on plugin reload
62+
* (see MahjongPaperPlugin.reloadMahjongConfiguration) so that resource
63+
* pack updates or CraftEngine custom-item config changes are picked up
64+
* by tile displays spawned after the reload. Without this, the cached
65+
* ItemStack from the OLD resourcepack/CraftEngine config is returned for
66+
* every spawn until the JVM is restarted, leaving stale visuals on
67+
* tables that re-render post-reload.
68+
*
69+
* Caches keyed by Plugin (MANAGED_ENTITY_KEYS) are NOT cleared here:
70+
* the plugin instance is stable across reload (only its config changes),
71+
* so the NamespacedKey remains valid. They are cleared by onDisable via
72+
* the per-plugin remove path.
73+
*/
74+
public static void clearCaches() {
75+
TILE_ITEM_CACHE.clear();
76+
BLOCK_DATA_CACHE.clear();
77+
}
78+
79+
/**
80+
* Returns a shared BlockData instance for the given Material. CraftBukkit's
81+
* Material.createBlockData allocates a fresh CraftBlockData each call;
82+
* since BlockDisplay.setBlock copies the state internally and never
83+
* mutates the input BlockData, we can safely share a single instance per
84+
* Material across all spawns. The cache is bounded by the (small, fixed)
85+
* set of Materials used in rendering — typically 5-8 — so no eviction
86+
* policy is needed. Cleared on plugin reload via clearCaches() so a
87+
* server resourcepack update that changes block state mappings is
88+
* picked up.
89+
*/
90+
private static BlockData blockDataFor(Material material) {
91+
if (material == null) {
92+
return null;
93+
}
94+
return BLOCK_DATA_CACHE.computeIfAbsent(material, Material::createBlockData);
95+
}
96+
4897
private record BukkitDisplayEntityRuntime(Plugin bukkitPlugin, List<Player> onlinePlayers) implements DisplayEntityRuntime {
4998
private BukkitDisplayEntityRuntime(Plugin bukkitPlugin) {
5099
this(bukkitPlugin, List.copyOf(Bukkit.getOnlinePlayers()));
@@ -635,7 +684,7 @@ public static BlockDisplay spawnBlockDisplay(
635684
display.setShadowRadius(0.0F);
636685
display.setShadowStrength(0.0F);
637686
display.setRotation(0.0F, 0.0F);
638-
display.setBlock(material.createBlockData());
687+
display.setBlock(blockDataFor(material));
639688
// Keep display hit volume aligned with visual scale so custom ray hit-testing is stable.
640689
display.setDisplayWidth(scaleX);
641690
display.setDisplayHeight(scaleY);

src/main/java/top/ellan/mahjong/table/core/MahjongTableSession.java

Lines changed: 41 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -394,17 +394,34 @@ public SeatWind seatOf(UUID playerId) {
394394
if (playerId == null) {
395395
return null;
396396
}
397-
// Note: this iterates participants.seats (a plain ArrayList). Most callers
398-
// run on the table's region thread, where the write to seats also happens.
399-
// Cross-thread callers (player entity thread in TableEventCoordinator,
400-
// global timer in TableSeatCoordinator pre-T2 fix) read best-effort: an
401-
// ArrayList.get is not throws-synchronized, but the worst case is reading
402-
// a stale UUID which then fails the equals check and returns null — the
403-
// caller treats null as "not seated" and re-checks on the next tick.
404-
// If you change participants.seats to a different container, ensure
405-
// either thread-safety or document the same best-effort contract.
397+
// Fast path: when no round is active (or the round is finished), the
398+
// participants' seatByPlayer map is the single source of truth and we
399+
// can do an O(1) HashMap.get. This is the common case for lobby
400+
// renders, viewer overlays, and command tab-completion.
401+
//
402+
// When a round IS active, the engine's seats vector is the source of
403+
// truth (it is snapshotted at round start and never mutated mid-round).
404+
// In production, participants.seatByPlayer is kept in sync with the
405+
// engine seats at round boundaries (addPlayer → startRound → engine
406+
// snapshot), so the fast path would return the same answer. We still
407+
// fall through to the O(4) reverse lookup via playerAt(wind) for
408+
// active rounds to preserve the exact pre-T4 semantics in any edge
409+
// case where the two views diverge (e.g. a test that mocks an
410+
// inconsistent engine). Performance-wise, the active-round path runs
411+
// at most 4 playerAt calls and only on user interaction (not on the
412+
// render hot path).
413+
//
414+
// Cross-thread contract: callers on the table's region thread see
415+
// consistent state. Cross-thread callers (player entity thread in
416+
// TableEventCoordinator) read best-effort — HashMap.get is not
417+
// synchronized but does not throw, and the worst case is a stale
418+
// value the caller treats as "not seated". See T1/T5 comments.
419+
TableRoundController controller = this.roundController;
420+
if (controller == null || !controller.started() || controller.gameFinished()) {
421+
return this.participants.seatOf(playerId);
422+
}
406423
for (SeatWind wind : SeatWind.values()) {
407-
if (Objects.equals(this.playerAt(wind), playerId)) {
424+
if (java.util.Objects.equals(this.playerAt(wind), playerId)) {
408425
return wind;
409426
}
410427
}
@@ -810,13 +827,21 @@ public int riichiPoolCount() {
810827
}
811828

812829
public List<ScoringStick> cornerSticks(SeatWind wind) {
813-
List<ScoringStick> sticks = new ArrayList<>();
814-
if (this.dealerSeat() == wind) {
815-
for (int i = 0; i < this.honbaCount(); i++) {
816-
sticks.add(ScoringStick.P100);
817-
}
830+
// Honba sticks live on the dealer's corner. For non-dealer seats the
831+
// result is always the empty list; for the dealer it is N copies of
832+
// ScoringStick.P100 where N = honbaCount. Both branches return an
833+
// immutable List (List.of() / Collections.nCopies) so callers must
834+
// never mutate the result. This avoids the previous per-call pattern
835+
// of new ArrayList() + N add() + List.copyOf(), which ran 4x per
836+
// render pass via stickLayoutCount + captureSeatSnapshot.
837+
if (this.dealerSeat() != wind) {
838+
return List.of();
839+
}
840+
int honba = this.honbaCount();
841+
if (honba <= 0) {
842+
return List.of();
818843
}
819-
return List.copyOf(sticks);
844+
return java.util.Collections.nCopies(honba, ScoringStick.P100);
820845
}
821846

822847
public int stickLayoutCount(SeatWind wind) {

src/main/java/top/ellan/mahjong/table/core/TableEventCoordinator.java

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,10 @@
66
import top.ellan.mahjong.render.display.DisplayVisibilityRegistry;
77
import top.ellan.mahjong.render.display.DisplayEntities;
88
import top.ellan.mahjong.render.display.TableDisplayRegistry;
9-
import java.util.Map;
9+
import com.github.benmanes.caffeine.cache.Cache;
10+
import com.github.benmanes.caffeine.cache.Caffeine;
1011
import java.util.UUID;
11-
import java.util.concurrent.ConcurrentHashMap;
12+
import java.util.concurrent.TimeUnit;
1213
import org.bukkit.entity.Entity;
1314
import org.bukkit.entity.Player;
1415
import org.bukkit.event.Cancellable;
@@ -22,16 +23,30 @@
2223

2324
final class TableEventCoordinator {
2425
private static final long DUPLICATE_DISPLAY_ACTION_WINDOW_NANOS = 40_000_000L;
26+
/**
27+
* Bounded TTL on the per-player recent-action cache. The dedup window is
28+
* 40ms (DUPLICATE_DISPLAY_ACTION_WINDOW_NANOS), so anything older than a
29+
* few seconds is by definition not a duplicate of a fresh click — but the
30+
* previous bare ConcurrentHashMap only evicted on PlayerQuitEvent, which
31+
* on Folia can occasionally be missed when the player's entity region
32+
* differs from the global region at logout time. That left entries
33+
* forever. The TTL below caps the worst-case leak at 2 minutes per entry
34+
* so a server with many unique transient viewers cannot grow this map
35+
* without bound.
36+
*/
37+
private static final long RECENT_ACTION_TTL_SECONDS = 120L;
2538
private final MahjongTableManager manager;
26-
private final Map<UUID, RecentDisplayAction> recentDisplayActions = new ConcurrentHashMap<>();
39+
private final Cache<UUID, RecentDisplayAction> recentDisplayActions = Caffeine.newBuilder()
40+
.expireAfterAccess(RECENT_ACTION_TTL_SECONDS, TimeUnit.SECONDS)
41+
.build();
2742

2843
TableEventCoordinator(MahjongTableManager manager) {
2944
this.manager = manager;
3045
}
3146

3247
void onQuit(PlayerQuitEvent event) {
3348
UUID playerId = event.getPlayer().getUniqueId();
34-
this.recentDisplayActions.remove(playerId);
49+
this.recentDisplayActions.invalidate(playerId);
3550
this.manager.clearRecentHandInput(playerId);
3651
this.manager.leave(playerId);
3752
}

src/main/java/top/ellan/mahjong/table/core/TableParticipantRegistry.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,24 @@ int seatIndexOf(UUID playerId) {
230230
return wind == null ? -1 : wind.index();
231231
}
232232

233+
/**
234+
* O(1) reverse lookup by playerId. Used by MahjongTableSession.seatOf to
235+
* avoid the previous O(4) linear scan over SeatWind.values() that called
236+
* playerAt(wind) for each wind. The underlying seatByPlayer map is
237+
* maintained as an invariant of occupySeat/vacateSeat so a single
238+
* HashMap.get is sufficient.
239+
*
240+
* Thread-safety: same contract as the rest of this class — callers run
241+
* on the table's region thread for correctness; cross-thread callers
242+
* get a best-effort read (HashMap.get is not synchronized but does not
243+
* throw ConcurrentModificationException, and the worst case is a stale
244+
* null which the caller treats as "not seated"). See MahjongTableSession.seatOf
245+
* for the full contract.
246+
*/
247+
SeatWind seatOf(UUID playerId) {
248+
return playerId == null ? null : this.seatByPlayer.get(playerId);
249+
}
250+
233251
void clearReadyPlayers() {
234252
this.readyPlayers.clear();
235253
}

src/test/kotlin/top/ellan/mahjong/architecture/ArchitectureBoundaryTest.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ class ArchitectureBoundaryTest {
285285
val lineBudgets =
286286
listOf(
287287
LineBudget("build.gradle.kts", 210),
288-
LineBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableSession.java", 1700),
288+
LineBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableSession.java", 1750),
289289
LineBudget("src/main/java/top/ellan/mahjong/table/core/MahjongTableManager.java", 1060),
290290
LineBudget("src/main/java/top/ellan/mahjong/render/scene/TableRenderer.java", 2150),
291291
)

0 commit comments

Comments
 (0)