Repository navigation
perf: read inventory holders without copying the block - #48
Merged
Merged
Conversation
The pet and staff menu listeners run on every inventory click, drag and close. Each plain getHolder() call copied the whole block state when the inventory belonged to a chest or barrel. On Main StaffMenus.onClick and PetListener.onClick cost 3.3 s over 20 hours. The checks now use getHolder(false), which hands back the same holder without the copy, and the reload check reads the holder once instead of twice. Pet and staff menus have their own holders, so the result is the same object as before. No call site changes a block state, so none were left as they were. MockBukkit throws on getHolder(false), so a JUnit session listener patches its inventories to return their plain holder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
| Layer / File(s) | Summary |
|---|---|
Runtime inventory holder checks src/main/java/net/tfminecraft/companionpets/PetsPlugin.java, src/main/java/net/tfminecraft/companionpets/gui/PetMenus.java, src/main/java/net/tfminecraft/companionpets/listen/PetListener.java, src/main/java/net/tfminecraft/companionpets/ownership/CharacterLifecycle.java, src/main/java/net/tfminecraft/companionpets/staff/StaffMenus.java |
Menu and inventory checks now call getHolder(false). Existing type checks and subsequent handling remain unchanged. |
Test holder lookup compatibility pom.xml, src/test/java/net/tfminecraft/companionpets/testutil/HolderLookupPatch.java, src/test/resources/META-INF/services/org.junit.platform.launcher.LauncherSessionListener |
The test dependencies include junit-platform-launcher. Its session listener installs a Byte Buddy patch that replaces InventoryMock.getHolder(boolean) with a call to inventory.getHolder(). |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Suggested reviewers: nowkoo, ryanbarlow97
Merge Risk: 
dac6b
-
The holder lookup change has no identified merge-blocking issue.
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:S
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}
Byte Buddy reports a failed retransformation only to its listeners and returns normally, so the patch counted as installed even when InventoryMock kept its throwing getHolder(boolean). It now marks itself installed only after InventoryMock was transformed, and stops the test session with a clear error otherwise (CodeRabbit). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Rate Limit Exceeded
|
onTransformation fires before the JVM applies the new bytes, so a rejection in retransformClasses reaches only onError. The patch now fails the test session in that case too (Opus review). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 CompanionPets:StaffMenus.onClick1.7 s andPetListener.onClick1.6 s.Change
getHolder(false); the reload loop reads the holder once.HolderLookupPatchvia a JUnitLauncherSessionListener(adds test-scopejunit-platform-launcher).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
mainrun on Windows, class for class; Linux CI is the check.TestChar1:/faction list(row click works),/referraland/activitymenus opened and worked.🤖 Generated with Claude Code