Skip to content

perf: read grave tags without copying the chest on hopper moves - #136

Merged
XxFran10xX merged 2 commits into
mainfrom
perf/grave-hopper-no-snapshot
Oct 23, 2026
Merged

XxFran10xX merged 2 commits into
mainfrom
perf/grave-hopper-no-snapshot

Conversation

@XxFran10xX

Copy link
Copy Markdown
Contributor

Summary

Stops the grave check from copying a whole chest on every hopper transfer.

GraveInteractListener.onHopperMove calls GraveManager.isGrave for the source and the destination of every InventoryMoveItemEvent on the server. GraveManager.getAt read the grave tag through block.getState(). That builds a snapshot of the block entity, so each check deep-copied every item in the chest just to read one tag.

In a spark profile of Main (2026-10-22, 20:28–21:28 UTC, about 30 players), this path (CraftBlock.getState → CraftContainer.<init> → ContainerHelper.loadAllItems) cost about 3 s of server-thread time per minute, around 5% of every tick. The copies are also garbage that adds to young-GC work.

getAt now reads the tag with block.getState(false), the live block entity, without a snapshot. The state is only read for the tag on the server thread and never stored. Lookups return the same graves as before, including the byBlock location fallback.

Testing

  • GraveManagerTest: new taggedLookupReadsTheLiveBlockEntityWithoutCopyingTheChest checks that getAt finds a tagged grave and never calls getState(). The existing grave tests pass.
  • The full local run on Windows has the same 11 failures and 141 errors as main. They're Windows file-permission tests (AccessDenied, POSIX views); Linux CI is the reference.
  • Bot test on TFMCDev01 (~/grave-hopper-test/test.js + open.js, character bot TestChar1). I ran it on the old jar first (pass), then on this build:
    • The bot dies with items, which leaves a grave chest. A hopper the bot places under the grave chest pulls nothing, and the grave chest keeps its items.
    • A control hopper the bot places under an ordinary bot-placed chest pulls the items as before.
    • The owner right-clicks the grave and gets everything back ("You recovered your belongings.", 32 cobblestone + 16 oak logs). That goes through the new getAt tag path.
    • The Dev log had no RPCharacters errors.

Part of the 2026-10-22 ping/lag investigation, together with VehicleFramework and Games PRs.

🤖 Generated with Claude Code

GraveManager.getAt runs for both inventories of every InventoryMoveItemEvent.
It used block.getState(), which snapshots the block entity and deep-copies
the whole chest inventory just to read the grave id tag. A spark profile on
Main showed this at ~3 s of server-thread time per minute.

Read the live block entity with getState(false) instead. The lookup only
reads the persistent data container on the server thread and never keeps the
state, so the result is identical.

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

coderabbitai Bot commented Oct 23, 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: 49a5117c-8e6f-4ffa-822b-9510e8905f7a
📥 Commits

Reviewing files that changed from the base of the PR and between a7a7126 and 3699fad.

📒 Files selected for processing: 2
  • src/main/java/net/tfminecraft/rpcharacters/grave/GraveManager.java
  • src/test/java/net/tfminecraft/rpcharacters/grave/GraveManagerTest.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
    • Grave lookups now read the grave ID from the live block state rather than a copied state. This keeps checks such as identifying whether a block is a grave tied to the current block entity data, without changing the existing ID validation or fallback lookup behaviour.

GraveManager.getAt now reads the grave ID from the live tile state. A test checks that grave lookup and detection do not call the copying getState() overload.

Changes: Grave lookup

Layer / File(s) Summary
Read grave IDs from live tile state
src/main/java/net/tfminecraft/rpcharacters/grave/GraveManager.java, src/test/java/net/tfminecraft/rpcharacters/grave/GraveManagerTest.java
getAt uses getState(false) to read the grave ID. The test checks that lookup and grave detection use this overload and do not call getState().

Priority: ➖ Normal

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

Change: Refactor

Suggested reviewers: ryanbarlow97

Merge Risk: ⚪ Minimal · up to 3699f

No actionable merge-blocking risk remains from the reviewed change.

- [ ] 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.

@github-actions github-actions Bot added the size:XS 0-9 effective changed lines (tests excluded in mixed PRs). label Oct 23, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@XxFran10xX
XxFran10xX marked this pull request as ready for review October 23, 2026 00:16
@XxFran10xX

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 23, 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
XxFran10xX merged commit 45e6b7a into main Oct 23, 2026
3 of 4 checks passed
@XxFran10xX
XxFran10xX deleted the perf/grave-hopper-no-snapshot branch October 23, 2026 00:22
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