Repository navigation
perf: read inventory holders without copying the block - #74
Merged
Merged
Conversation
The gear, resonance and artifact menus were slow on Main because every getHolder() call on a container inventory copies the whole block state. The gear click handler did this up to four times per click, and the resonance refresh did it for every online player. These now use getHolder(false), which returns the same holder without the copy, and the gear click handler reads it once. The code only checks the holder's type, so behaviour does not change. No call site needed the copying form. MockBukkit does not implement getHolder(false), so the affected tests use a server mock whose inventories answer it like getHolder(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Update production holder checks src/main/java/net/tfminecraft/magic/artifact/ArtifactCarePlaces.java, src/main/java/net/tfminecraft/magic/gear/gui/GearInventoryManager.java, src/main/java/net/tfminecraft/magic/listener/MagicSessionListener.java, src/main/java/net/tfminecraft/magic/manager/ArtifactCreateGuiManager.java, src/main/java/net/tfminecraft/magic/manager/ResonanceGuiManager.java |
These code paths now retrieve inventory holders with getHolder(false). Gear click handling reuses the retrieved holder for its checks and dispatch. |
Adapt test inventories src/test/java/net/tfminecraft/magic/HolderLookupServerMock.java, src/test/java/net/tfminecraft/magic/AttunementTest.java, src/test/java/net/tfminecraft/magic/ChargeEdgeCoverageTest.java, src/test/java/net/tfminecraft/magic/GearCoverageSupport.java, src/test/java/net/tfminecraft/magic/ListenerTest.java |
HolderLookupServerMock wraps inventories created through seven overloads. Test setup uses this mock, and the affected tests use or stub getHolder(false). |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Suggested reviewers: ryanbarlow97
Merge Risk: 
f136d
-
The optimization preserves the holder behavior used by production and test paths; no merge-blocking risk was found.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
github-actions
Bot
added
the
size:M
XxFran10xX
marked this pull request as ready for review
October 31, 2026 12:42
Contributor
Author
|
@coderabbitai review |
Action performed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
Why
Main profiles, 20 hourly spark dumps 10-30 10:00 → 10-31 05:00 (peak 37 players).
Inventory#getHolder()without an argument builds a full BlockState copy when the inventory belongs to a block (chest, barrel, double chest), and every plugin's click/open listener calls it again. Over the 20 h that cost ~200 s of main-thread time, mostly as short stalls on clicks in heavy containers; one click at 04:01 took 2.0 s. In Magic:GearInventoryManager.onClick7.3 s,ResonanceGuiManager.refreshOpenInventories4.0 s,ArtifactCreateGuiManager.onInventoryClick1.6 s.Change
getHolder(false); the 4 inGearInventoryManager.onClickare now one read.getHolder(boolean), so a test-onlyHolderLookupServerMockanswers it.For plugin menus the holder is the plugin's own object, so the result is identical. For real block inventories the code only type-checks or reads the holder, so the live state gives the same answer. Nothing players see changes.
Testing
mainalso has.TestChar1:/faction list(row click works),/referraland/activitymenus opened and worked.🤖 Generated with Claude Code