Skip to content

Optimize sorting tree container lookup#2670

Open
KaroUniform wants to merge 3 commits into
TeamTwilight:latestfrom
KaroUniform:agent/optimize-sorting-tree-container-search
Open

Optimize sorting tree container lookup#2670
KaroUniform wants to merge 3 commits into
TeamTwilight:latestfrom
KaroUniform:agent/optimize-sorting-tree-container-search

Conversation

@KaroUniform

@KaroUniform KaroUniform commented Jul 16, 2026

Copy link
Copy Markdown

What changed

  • Enumerate block entities from already-loaded chunks instead of probing every block position in the configured cube.
  • Cache sided item capabilities with NeoForge's BlockCapabilityCache while the target block entity is loaded.
  • Partition capability caches by ServerLevel identity, use weak block-entity keys within each level, and clear the complete per-level table on server-side LevelEvent.Unload.
  • Update the sorting-particle call to the 26.1 packet signature.
  • Add coverage for chunk boundaries, unloaded chunks, removed block entities, successful transfers, transaction rollback, directional cache reuse, cross-level isolation, and selective level cleanup.

Why

The sorting tree runs once per second. At the default range of 16, the previous implementation checked 35,937 block positions per active core before it could begin sorting. The loaded-chunk lookup does not load or generate chunks and scales with loaded chunks and block entities rather than the volume of the search cube.

The follow-up keeps NeoForge's recommended BlockCapabilityCache for repeated queries without restoring the old lifetime and cross-level keying problems. Caches are separated by level, ordinary block/chunk invalidation releases cached handlers, and level unload explicitly removes the remaining per-level object graph.

This is the forward port of #2669 to the current latest / 26.1 branch.

Validation

  • Java 25 strict compilation with the project's -Xlint flags and -Werror: passed for the changed production code and tests against the real Minecraft 26.1.1 / NeoForge 26.1.1.6-beta classpath.
  • Standalone lifecycle harness: passed cache reuse, directional separation, identical positions in different levels, selective clear, cache recreation, and position changes.
  • Real NeoForge dedicated-server integration test with chests at the same coordinates in the Overworld and Nether: passed reuse, directional separation, level isolation, and selective cleanup.
  • 500,000 cached capability lookups completed in 17 ms without increasing cache-table sizes.
  • Clean server shutdown observed per-level unload cleanup from 2 tables to 1 and then 0.
  • Independent correctness and red-team reviews found no blockers.
  • git diff --check: passed.

The full upstream latest build is not currently a usable signal: the base branch has unrelated unfinished 26.1 port compilation errors, and its Azure pipeline still selects JDK 21 while the Gradle toolchain requires Java 25. The isolated checks above avoid hiding that baseline condition while still compiling and exercising the changed code.

@@ -1,31 +0,0 @@
package twilightforest.util;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this deleted?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was only used by the Sorting Tree (and its dedicated test). This PR no longer needs it because, while iterating loaded block entities, we already have the BlockEntity and BlockState and can query the capability directly. Leaving it would therefore be dead code.

Its map also lived for the lifetime of the block singleton without eviction, was keyed only by position and direction (not by ServerLevel), and the previous cube iterator could supply mutable BlockPos instances. Retaining it would preserve the lifetime and cross-level keying issues this change is intended to remove. The test was removed along with the now-unused helper.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NeoForge devs strongly recommend to use BlockCapabilityCache whenever possible.

@Tamaized Tamaized Jul 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out and for linking the NeoForge guidance. I reworked this in 7c0c4d8 to keep using BlockCapabilityCache for each loaded block entity and side.

To avoid the lifetime and cross-level issues in the previous helper, the caches are now partitioned by ServerLevel, use weak block-entity keys within each level, and explicitly clear the complete per-level table on server-side LevelEvent.Unload.

I added tests for reuse, directional separation, identical positions in different levels, and selective cleanup. I also exercised the exact cache implementation on a NeoForge dedicated server: 500,000 repeated lookups kept the table sizes stable, and shutdown cleanup went from 2 level tables to 1 and then 0.

Could you please take another look and let me know whether this now matches the recommended pattern?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if a single event needs its own class, would possibly fit better into MiscEvents.

@KaroUniform KaroUniform Jul 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a reasonable suggestion. MiscEvents is a better fit for a single stateless handler. I moved the unload listener there and removed the dedicated event class in 73e506b. Thanks!

@Tamaized

Copy link
Copy Markdown
Member

Just an FYI, I don't want to merge a change like this until the initial 1:1 parity port is complete so this is going to be placed on the back burner but it is being considered regardless.

Though I personally feel some rework needs to be done to not have the field inside the block itself. I would love for it to be a block entity instead but that also has some drawbacks we're not willing to make, for example: allowing pistons to push it around. But that's out of scope for this PR and something we'd revisit later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants