Skip to content

perf: read the live holder in the grave hopper check - #141

Merged
XxFran10xX merged 2 commits into
mainfrom
perf/grave-hopper-holder
Oct 28, 2026
Merged

XxFran10xX merged 2 commits into
mainfrom
perf/grave-hopper-holder

Conversation

@XxFran10xX

Copy link
Copy Markdown
Contributor

Why

Players report TPS dips on Main. In the 10-22 spark profile (hPFtqh9M5c), the grave check on hopper moves cost 7.1 % of all server-thread time. #136 fixed 5.2 % of that (GraveManager.getAt copied the chest via getState()). The other 1.9 % is still there: GraveInteractListener.inventoryBlock calls Inventory#getHolder(), which on Paper also builds a BlockState snapshot of the whole chest, just so we can read its block. It runs on both inventories of every InventoryMoveItemEvent.

Change

  • inventoryBlock asks for the live holder with getHolder(false). The block it returns is the same, so the grave check gives the same answer:
    • single chests and other block containers: same holder type, same block;
    • double chests: still a DoubleChest holder, so they fall through to getLocation() as before;
    • hopper minecarts: still the entity holder plus the location fallback.
  • GraveEventsTest: MockBukkit 4.95 does not implement getHolder(boolean) and threw UnimplementedOperationException, which the extension reports as a skip. The grave chest inventory is now a spy that returns its holder, and a new assertion checks that hopper moves never call getHolder().

Testing

  • GraveEventsTest: 32 run, 0 failures, 0 skipped. On Windows the full suite has its usual file-system failures (atomic moves, POSIX permissions); none of them touch this code, and Linux CI is the reference.
  • Dev: this class swapped into Dev's current RPC jar (the only bytecode difference was this method), loaded at Dev's restart and tested with the grave hopper bot (results below).
  • Reviewed by Sol (GPT-6.1) over two rounds; approved.

🤖 Generated with Claude Code

XxFran10xX and others added 2 commits October 28, 2026 10:08
Hopper moves run isGraveInventory on both inventories for every
transfer. Inventory#getHolder() builds a BlockState snapshot that copies
the whole chest just so we can find its block; in the 10-22 Main spark
profile that copy was ~1.9% of all server-thread time. getHolder(false)
returns the live block entity instead, with the same block.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…kkit

MockBukkit 4.95 does not implement Inventory#getHolder(boolean); it threw
UnimplementedOperationException, which the extension reports as a skip,
so graveBlocksResistBreaksExplosionsPistonsAndHoppers silently stopped
running. Spy the grave chest inventory and return its holder instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0a8e15fa-5815-4f98-810e-e8765c1aef70
Commits

Reviewing files that changed from the base of the PR and between 65966a1 and 5f4a287.

Files selected for processing: 2
  • src/main/java/net/tfminecraft/rpcharacters/grave/GraveInteractListener.java
  • src/test/java/net/tfminecraft/rpcharacters/grave/GraveEventsTest.java

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Hopper movement into grave chests is now blocked more reliably, including when the chest is identified through its live inventory holder rather than its location. Existing location-based checks remain in place, helping prevent items from being moved into a grave chest through either detection method.

The grave inventory lookup now calls getHolder(false). The hopper-movement test checks cancellation when the destination inventory has a live grave chest holder and verifies that the path does not call getHolder() on that inventory.

Changes: Grave hopper lookup

Layer / File(s) Summary
Live holder lookup and validation
src/main/java/net/tfminecraft/rpcharacters/grave/GraveInteractListener.java, src/test/java/net/tfminecraft/rpcharacters/grave/GraveEventsTest.java
inventoryBlock uses getHolder(false). The test checks that hopper movement into a grave chest is cancelled when the destination inventory has a live chest holder, without calling getHolder() on that inventory.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Refactor

Possibly related PRs

  • TF-Minecraft/RPCharacters#136: Changes the grave tag lookup during hopper checks. This PR changes the inventory holder lookup in the same grave hopper-check path.

Suggested reviewers: ryanbarlow97

Merge Risk: Minimal · up to 5f4a2

The live-holder lookup preserves grave detection for the stated inventory types while avoiding the snapshot lookup.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@XxFran10xX

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@github-actions github-actions Bot added the size:XS 0-9 effective changed lines (tests excluded in mixed PRs). label Oct 28, 2026
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@XxFran10xX

Copy link
Copy Markdown
Contributor Author

Dev test (10-28 09:4x UTC): this class was swapped into Dev's current RPC jar (javap showed inventoryBlock as the only difference) and loaded at a Dev restart. Grave hopper bot test (~/perf-20261028-test/grave.js): the hopper under a grave chest stayed empty, the hopper under an ordinary chest pulled, and the grave kept its items. PASS.

@XxFran10xX
XxFran10xX merged commit 8c078a1 into main Oct 28, 2026
3 checks passed
@XxFran10xX
XxFran10xX deleted the perf/grave-hopper-holder branch October 28, 2026 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 effective changed lines (tests excluded in mixed PRs).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant