Skip to content

Reduce temporary allocations while scanning ships#561

Draft
frostdev-ops wants to merge 1 commit into
LemADEC:MC1.12from
frostdev-ops:perf/jump-scan-allocations
Draft

Reduce temporary allocations while scanning ships#561
frostdev-ops wants to merge 1 commit into
LemADEC:MC1.12from
frostdev-ops:perf/jump-scan-allocations

Conversation

@frostdev-ops

Copy link
Copy Markdown

Experimental motivation

JumpShip.save() allocates five arrays sized to the full bounding volume before it knows how many blocks will actually move.

Scope

  • Reuses one MutableBlockPos while scanning.
  • Reads the block once per block state.
  • Replaces the five bounding-volume arrays with five priority lists.
  • Preserves scan traversal, placement priority, mass/volume accounting, transformer checks, and final jumpBlocks ordering.

Allocation evidence and limitations

The old code reserved exactly 5 * boundingVolume reference slots. The new code avoids that fixed reservation: 5,000 slots at volume 1,000; 500,000 at volume 100,000; and 5,000,000 at volume 1,000,000. At the default tier-3 per-side limit, a 193-cubed bounding box would have reserved 35,945,285 reference slots. JVM byte cost depends on reference width and array headers.

  • Java 8 offline Gradle build: passed (test NO-SOURCE).
  • All pairwise merges and both aggregate build orders passed.
  • Small, medium, and maximum-size live scan timings have not been recorded yet, so this remains draft.

Manual testing pending

Compare mass, volume, transformer results, final block order, tile data, and successful movement against upstream.

@frostdev-ops
frostdev-ops force-pushed the perf/jump-scan-allocations branch from 4253c94 to 530283d Compare July 11, 2026 16:34
@LemADEC

LemADEC commented Jul 14, 2026

Copy link
Copy Markdown
Owner

There's 2 changes proposed here, first MutableBlockPos:
I've already tried using it here, but there's an edge case that ends up corrupting the world, and I should have left a comment there for this, my bad. The case is something like this: a block should have a tile entity but lost it (for whatever reason), when saving the ship, we call getTileEntity on it, upstream find the entity is missing, so it creates a new one using the mutable, forgetting to change it to a plain BlockPos, then save looks for the next block and the tile entity get corrupted. So, I prefer to play on the safe side and use a BlockPos, effectively spamming small objects in memory, but that's better than world corruption.
If we want to go the MutableBlockPos route, I suggest we properly cover that getTileEntity call, without breaking mods compatibility...

@LemADEC

LemADEC commented Jul 14, 2026

Copy link
Copy Markdown
Owner

Now, regarding the second change, replacing 5 pre-allocated JumpBlock native arrays with a single ArrayList:
The existing approach aims to avoid reallocation which causes CPU load and more work for the GC, it pre-allocates up to 137MB that'll be freed immediately right after. It's only 137MB because those are only pointers (4 bytes), they're not the full fledged JumpBlock (48+ bytes), they're just null pointers.
The proposed approach uses a non pre-allocated ArrayList which will reallocate at least 16 times on a medium sized ship 17x77x17 (22253 bounding volume, 4791 actual blocks). That's a good intention but actually makes it way worse. If you want to go that route, pre-allocate the array to a reasonable value (measured actual volume from a previous scan of the ship, or just a ratio like 20 % of the bounding volume).

To move forward, I suggest you measure the actual dimensions and blocks of the ships you're having issues with, that'll help understand what's going on here.
Also, splitting this one in 2 PR to separate the changes would help.

Replace the five bounding-volume JumpBlock arrays with lists pre-sized
from the last ship scan volume (or 20% of the bounding box when no scan
is available), avoiding both the large upfront allocation and repeated
list growth.
@frostdev-ops

Copy link
Copy Markdown
Author

Split as requested.

This PR is now just the pre-sizing: kept the five placeTime buckets, but as lists pre-sized from the last ship scan (shipCore.shipVolume) when available, else 20% of the bounding volume. No reallocation on the way up, and no 5x bounding-volume pointer arrays either - on your 17x77x17 example that's five 4791-capacity lists instead of five 22253-slot arrays.

The MutableBlockPos half moved to #564, with the escape covered: the mutable only serves the skip path (air / left-behind, most of the bounding box) and switches to toImmutable() before anything can reach getTileEntity or the JumpBlock constructor, plus a comment documenting the corruption case you described. If you'd rather not have a mutable anywhere near that loop at all, feel free to just close that one - the two changes are independent now.

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.

2 participants