server micro optimizations #194
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| --- a/net/minecraft/world/entity/npc/villager/Villager.java | ||
| +++ b/net/minecraft/world/entity/npc/villager/Villager.java | ||
| @@ -887,7 +_,11 @@ | ||
| if (this.lastGossipDecayTime == 0L) { | ||
| this.lastGossipDecayTime = timestamp; | ||
| } else if (timestamp >= this.lastGossipDecayTime + 24000L) { | ||
| + // Gale start - Do less work - Skip gossip decay for villagers without any gossip | ||
| + if (!this.gossips.gossips.isEmpty()) { | ||
| this.gossips.decay(); | ||
| + } | ||
| + // Gale end - Do less work - Skip gossip decay for villagers without any gossip | ||
| this.lastGossipDecayTime = timestamp; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| --- a/net/minecraft/world/level/BaseSpawner.java | ||
| +++ b/net/minecraft/world/level/BaseSpawner.java | ||
| @@ -107,6 +_,7 @@ | ||
| RandomSource random = level.getRandom(); | ||
| SpawnData nextSpawnData = this.getOrCreateNextSpawnData(level, random, pos); | ||
|
|
||
| + AABB nearbyEntitiesBox = new AABB(pos.getX(), pos.getY(), pos.getZ(), pos.getX() + 1, pos.getY() + 1, pos.getZ() + 1).inflate(this.spawnRange); // Gale - Do less work - Hoist nearby entity query box out of the spawn loop | ||
| for (int c = 0; c < this.spawnCount; c++) { | ||
| try (ProblemReporter.ScopedCollector reporter = new ProblemReporter.ScopedCollector(this::toString, LOGGER)) { | ||
| ValueInput input = TagValueInput.create(reporter, level.registryAccess(), nextSpawnData.getEntityToSpawn()); | ||
| @@ -165,7 +_,7 @@ | ||
|
|
||
| int nearBy = level.getEntities( | ||
| EntityTypeTest.forExactClass(entity.getClass()), | ||
| - new AABB(pos.getX(), pos.getY(), pos.getZ(), pos.getX() + 1, pos.getY() + 1, pos.getZ() + 1).inflate(this.spawnRange), | ||
| + nearbyEntitiesBox, // Gale - Do less work - Hoist nearby entity query box out of the spawn loop | ||
| EntitySelector.NO_SPECTATORS | ||
| ) | ||
| .size(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- a/net/minecraft/world/level/block/entity/BeehiveBlockEntity.java | ||
| +++ b/net/minecraft/world/level/block/entity/BeehiveBlockEntity.java | ||
| @@ -335,6 +_,7 @@ | ||
| } | ||
|
|
||
| public static void serverTick(final Level level, final BlockPos blockPos, final BlockState state, final BeehiveBlockEntity entity) { | ||
| + if (entity.stored.isEmpty()) return; // Gale - Do less work - Skip bee hive ticking when it holds no bees | ||
| tickOccupants(level, blockPos, state, entity.stored, entity.savedFlowerPos); | ||
| if (!entity.stored.isEmpty() && level.getRandom().nextDouble() < 0.005) { | ||
| double x = blockPos.getX() + 0.5; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| --- a/net/minecraft/world/level/block/entity/HopperBlockEntity.java | ||
| +++ b/net/minecraft/world/level/block/entity/HopperBlockEntity.java | ||
| @@ -38,6 +_,8 @@ | ||
| public int cooldownTime = -1; | ||
| private long tickedGameTime; | ||
| private Direction facing; | ||
| + private @Nullable AABB cachedSuckAabb; // Gale - Do less work - Cache hopper suck AABB | ||
| + private @Nullable BlockPos cachedSuckPos; // Gale - Do less work - Cache hopper suck AABB (invalidate on position change) | ||
|
|
||
| // CraftBukkit start - add fields and methods | ||
| public List<org.bukkit.entity.HumanEntity> transaction = new java.util.ArrayList<>(); | ||
| @@ -668,7 +_,9 @@ | ||
| } | ||
|
|
||
| public static List<ItemEntity> getItemsAtAndAbove(final Level level, final Hopper hopper) { | ||
| - AABB aabb = hopper.getSuckAabb().move(hopper.getLevelX() - 0.5, hopper.getLevelY() - 0.5, hopper.getLevelZ() - 0.5); | ||
| + AABB aabb = hopper instanceof HopperBlockEntity hopperBlockEntity // Gale - Do less work - Cache hopper suck AABB | ||
| + ? hopperBlockEntity.gale$getCachedSuckAabb() | ||
| + : hopper.getSuckAabb().move(hopper.getLevelX() - 0.5, hopper.getLevelY() - 0.5, hopper.getLevelZ() - 0.5); | ||
| return level.getEntitiesOfClass(ItemEntity.class, aabb, EntitySelector.ENTITY_STILL_ALIVE); | ||
| } | ||
|
|
||
| @@ -742,6 +_,18 @@ | ||
| return true; | ||
| } | ||
|
|
||
| + // Gale start - Do less work - Cache hopper suck AABB | ||
| + public AABB gale$getCachedSuckAabb() { | ||
| + AABB aabb = this.cachedSuckAabb; | ||
| + BlockPos pos = this.getBlockPos(); | ||
| + if (aabb == null || !pos.equals(this.cachedSuckPos)) { | ||
| + this.cachedSuckAabb = aabb = this.getSuckAabb().move(this.getLevelX() - 0.5, this.getLevelY() - 0.5, this.getLevelZ() - 0.5); | ||
| + this.cachedSuckPos = pos; | ||
| + } | ||
| + return aabb; | ||
| + } | ||
| + // Gale end - Do less work - Cache hopper suck AABB | ||
| + | ||
| public void setCooldown(final int time) { | ||
| this.cooldownTime = time; | ||
| } | ||
| @@ -767,7 +_,7 @@ | ||
| public static void entityInside(final Level level, final BlockPos pos, final BlockState blockState, final Entity entity, final HopperBlockEntity hopper) { | ||
| if (entity instanceof ItemEntity itemEntity | ||
| && !itemEntity.getItem().isEmpty() | ||
| - && entity.getBoundingBox().move(-pos.getX(), -pos.getY(), -pos.getZ()).intersects(hopper.getSuckAabb())) { | ||
| + && entity.getBoundingBox().intersects(hopper.gale$getCachedSuckAabb())) { // Gale - Do less work - Cache hopper suck AABB, avoids AABB allocation per item entity | ||
|
Member
There was a problem hiding this comment. I haven't done the logic in my head to figure out whether this is actually the same. Is it?
Member
Author
There was a problem hiding this comment. Yes - it's the same, by translation invariance of AABB.intersects. gale() returns getSuckAabb().move(getLevelX() - 0.5, getLevelY() - 0.5, getLevelZ() - 0.5), and for a HopperBlockEntity getLevelX()/getLevelY()/getLevelZ() are worldPosition + 0.5, so the cache holds suckAabb.move(pos) in world coordinates. A.intersects(B) <=> A.move(v).intersects(B.move(v)) for any v, since both boxes shift by the same vector and the min/max comparisons cancel out. So box.move(-pos).intersects(suckAabb) <=> box.intersects(suckAabb.move(pos)), which is exactly what the cached path checks. (The non-HopperBlockEntity path, i.e. MinecartHopper, still uses the exact vanilla expression.) |
||
| tryMoveItems(level, pos, blockState, hopper, () -> addItem(hopper, itemEntity)); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| --- a/net/minecraft/world/level/gameevent/EuclideanGameEventListenerRegistry.java | ||
| +++ b/net/minecraft/world/level/gameevent/EuclideanGameEventListenerRegistry.java | ||
| @@ -88,11 +_,13 @@ | ||
| if (this.listenersToRemove.remove(listener)) { | ||
| iterator.remove(); | ||
| } else { | ||
| - Optional<Vec3> optionalPosition = getPostableListenerPosition(this.level, sourcePosition, listener); | ||
|
Member
There was a problem hiding this comment. I think it would be better to make a This way we have a clearer and more traceable diff
Member
Author
There was a problem hiding this comment. Done as suggested - gale returns @nullable Vec3, and getPostableListenerPosition is now a thin Optional.ofNullable wrapper around it. |
||
| - if (optionalPosition.isPresent()) { | ||
| - action.visit(listener, optionalPosition.get()); | ||
| + // Gale start - Do less work - Inline the range check to avoid Optional allocations per listener per event | ||
| + Vec3 listenerPosition = gale$getPostableListenerPositionNullable(this.level, sourcePosition, listener); | ||
| + if (listenerPosition != null) { | ||
| + action.visit(listener, listenerPosition); | ||
| applicable = true; | ||
| } | ||
| + // Gale end - Do less work - Inline the range check to avoid Optional allocations per listener per event | ||
| } | ||
| } | ||
| } finally { | ||
| @@ -112,16 +_,22 @@ | ||
| return applicable; | ||
| } | ||
|
|
||
| - private static Optional<Vec3> getPostableListenerPosition(final ServerLevel level, final Vec3 sourcePosition, final GameEventListener listener) { | ||
| - Optional<Vec3> position = listener.getListenerSource().getPosition(level); | ||
| - if (position.isEmpty()) { | ||
| - return Optional.empty(); | ||
| + // Gale start - Do less work - Avoid Optional allocations per listener per event | ||
| + private static @org.jspecify.annotations.Nullable Vec3 gale$getPostableListenerPositionNullable(final ServerLevel level, final Vec3 sourcePosition, final GameEventListener listener) { | ||
| + Vec3 position = listener.getListenerSource().getPosition(level).orElse(null); | ||
| + if (position == null) { | ||
| + return null; | ||
| } | ||
|
|
||
| - double distanceFromOrigin = BlockPos.containing(position.get()).distSqr(BlockPos.containing(sourcePosition)); | ||
| + double distanceFromOrigin = BlockPos.containing(position).distSqr(BlockPos.containing(sourcePosition)); | ||
| int radiusSqr = listener.getListenerRadius() * listener.getListenerRadius(); | ||
| - return distanceFromOrigin > radiusSqr ? Optional.empty() : position; | ||
| - } | ||
| + return distanceFromOrigin > radiusSqr ? null : position; | ||
| + } | ||
| + | ||
| + private static Optional<Vec3> getPostableListenerPosition(final ServerLevel level, final Vec3 sourcePosition, final GameEventListener listener) { | ||
| + return Optional.ofNullable(gale$getPostableListenerPositionNullable(level, sourcePosition, listener)); | ||
| + } | ||
| + // Gale end - Do less work - Avoid Optional allocations per listener per event | ||
|
|
||
| @FunctionalInterface | ||
| public interface OnEmptyAction { | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There must be a reason for the
toRemove.add(null)above, did you look into this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That line is Paper's, not ours - it's part of Paper's MC-117075 fix (blockEntityTickers can contain nulls, so null is added to the reference set so removeAll strips them too). It only appears as an addition here because our removeAny change pulled that region into the patch as context. Our change in this method is just the removeAny guard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why they add this
add(null)and expectremoveAllto also remove null blockEntity from the tick list, because I don't see there is a place to add null to the list (If I missed some places, correct me((( ).This line of change was added in this commit PaperMC/Paper-archive@6f064f9#diff-477840aa089f974ea049572e2fc59e5dec986abc597ea9b1ce0f1487caad587c
And Machine Maker didn't leave a note to explain why he added
add(null)compared to the original patch.If the
removeAnyhere is false, it will not remove null block entity from the list, but it can still possibly be removed in future ticks; not sure whether it's a big issue. (If we really have null elements, if they don't exist, then it's fine I think)