Skip to content

Add EntityCollectionUtil with thread-local reusable lists - #193

Open
toprakdevx wants to merge 10 commits into
GaleMC:mainfrom
toprakdevx:feat/entity-collection-util
Open

Add EntityCollectionUtil with thread-local reusable lists#193
toprakdevx wants to merge 10 commits into
GaleMC:mainfrom
toprakdevx:feat/entity-collection-util

Conversation

@toprakdevx

Copy link
Copy Markdown
Contributor

Provides thread-local reusable ArrayList buffers for Entity, VoxelShape, and AABB collections. Each get method clears and returns the thread-local list, eliminating per-call ArrayList allocations in hot entity query paths.

@Nrleryxx

Copy link
Copy Markdown
Member

The utility class looks correct, but it's currently unused and has no callers. Without actual usage sites, this is dead code. Additionally, ArrayList::new without an initial capacity will grow repeatedly for large entity lists, defeating part of the purpose. Please reopen with actual callers and appropriate capacity hints.

@Nrleryxx Nrleryxx closed this Jul 29, 2026
@Nrleryxx Nrleryxx reopened this Jul 29, 2026
@toprakdevx

Copy link
Copy Markdown
Contributor Author

Updated with capacity hints (64 entities, 32 shapes) and patch 0147 that uses the reusable list in Level.getEntities(Entity, AABB).

@MartijnMuijsers

Copy link
Copy Markdown
Member

Is this ever called off-thread?

@toprakdevx

Copy link
Copy Markdown
Contributor Author

It can be, so ThreadLocal makes it safe either way.

@Nrleryxx

Nrleryxx commented Jul 29, 2026

Copy link
Copy Markdown
Member

Can you convert the patch to file (minecraft source) patch format?

@MartijnMuijsers

Copy link
Copy Markdown
Member

It can be, so ThreadLocal makes it safe either way.

Can it currently? I'm not talking about future changes.

toprakdevx added a commit to toprakdevx/Gale that referenced this pull request Jul 29, 2026
@toprakdevx

Copy link
Copy Markdown
Contributor Author

Fixed the patch format. @MartijnMuijsers you're right, it's server-thread only currently. ThreadLocal adds minimal overhead and keeps it safe if that ever changes.

@MartijnMuijsers

Copy link
Copy Markdown
Member

@toprakdevx Did you check yourself that it is used on the server thread only?

@MartijnMuijsers

Copy link
Copy Markdown
Member

Can you convert the patch to file (minecraft source) patch format?

Also this by the way ^

@toprakdevx

Copy link
Copy Markdown
Contributor Author

Converted to source patch in gale-server/minecraft-patches/sources/net/minecraft/world/level/Level.java.patch

@toprakdevx
toprakdevx marked this pull request as draft July 29, 2026 17:35
@toprakdevx
toprakdevx marked this pull request as ready for review July 29, 2026 17:46
@toprakdevx
toprakdevx marked this pull request as draft July 29, 2026 17:47
@toprakdevx
toprakdevx force-pushed the feat/entity-collection-util branch from fcdb9f7 to bea85ef Compare July 29, 2026 18:06
@toprakdevx

Copy link
Copy Markdown
Contributor Author

I checked, you're right - getEntities(Entity, AABB) is a default method in EntityGetter, not in Level.java, so the source patch was wrong. I've removed it and kept only the utility class with capacity hints (64/32). It can be used wherever appropriate in the future.

@toprakdevx
toprakdevx force-pushed the feat/entity-collection-util branch from 6242a3d to ce4d0c7 Compare July 29, 2026 18:16
@toprakdevx

Copy link
Copy Markdown
Contributor Author

@MartijnMuijsers you're right, it's server-thread-only. Removed ThreadLocal. Just leaving the utility class here as a building block for whoever adds a caller later.

@toprakdevx
toprakdevx marked this pull request as ready for review July 29, 2026 18:23
@MartijnMuijsers

Copy link
Copy Markdown
Member

you're right, it's server-thread-only

Did you check this yourself or with AI?

Converted to source patch in gale-server/minecraft-patches/sources/net/minecraft/world/level/Level.java.patch

You forgot to stage the file.
You can run ./gradle-bin/fixupAndRebuild

There's also currently still lists that are unused. Do you plan to make those used in this PR?

@toprakdevx

Copy link
Copy Markdown
Contributor Author

@MartijnMuijsers yeah checked with IntelliJ call hierarchy, all server thread.

Removed VoxelShape and AABB lists since they had no callers. Re-added the Level.java override with the source patch, should be staged properly now.

Mind giving it another look?

@toprakdevx

Copy link
Copy Markdown
Contributor Author

Fixed the patch build failure and cleaned up:

Level.java.patch - Removed the broken hunk that was wrongly anchored on getEntities(Entity, AABB, Predicate) (which doesn't exist in Level.java - it's a default method in the EntityGetter interface). The getEntities(Entity, AABB) override is now merged into the existing getBiome hunk, anchored on isDebug() instead. The hunk header was updated from @@ -2127,6 +,13 @@ to @@ -2127,6 +,22 @@.

EntityCollectionUtil.java - No code changes needed (already clean). The stale thread-local comment in the patch was removed since the list is server-thread-only.

@MartijnMuijsers MartijnMuijsers left a comment

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.

Approved, but with a few changes to be made later:

  • Should be moved to ServerLevel
  • EXPECTED_ENTITIES doesn't have to be a separate field

You don't have to make these changes in this PR now.

The PR will be merged, but please don't merge it yet, for conflict reasons.

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