Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse the biome and the list of potential mob spawns #1474

Open
wants to merge 3 commits into
base: 1.21.x
Choose a base branch
from

Conversation

FiniteReality
Copy link
Member

A spark profile showing allocations over 30s shows that NaturalSpawner#spawnForChunk allocates a lot of data:
image
Most of this is spent in NaturalSpawner#mobsAt, which is called twice in the process of computing which mob(s) to spawn in a given chunk. So, to alleviate this, I've implemented a simple change which simply pre-computes that list, and reuses it across the two methods it would normally be used, NaturalSpawner#canSpawnMobAt and NaturalSpawner#getRandomSpawnMobAt. In the process of doing this, I also moved the getBiome call here too, as it's the third most expensive call in NaturalSpawner#mobsAt, at 75MB allocated.

While I've attempted to keep binary compatibility, one potentially breaking change in this is that we no longer call EventHooks#getPotentialSpawns twice. This hook fires the LevelEvent.PotentialSpawns event, which also has a fairly expensive overhead (~190MB, though this may be related to other mods on the server) but my assumption is that most mods were likely just returning the same data twice in this event, potentially doing expensive calculations more than necessary.

This is certainly not all of the allocations in NaturalSpawner#spawnForChunk, but by percentage, it should approximately halve its allocation rate:
image

@FiniteReality FiniteReality added enhancement New (or improvement to existing) feature or request performance Related to performance 1.21.1 Targeted at Minecraft 1.21.1 labels Aug 20, 2024
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Aug 20, 2024

  • Publish PR to GitHub Packages

Last commit published: 646731751e6763b2964f93e50ca89c04b8da9a50.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1474' // https://github.com/neoforged/NeoForge/pull/1474
        url 'https://prmaven.neoforged.net/NeoForge/pr1474'
        content {
            includeModule('net.neoforged', 'testframework')
            includeModule('net.neoforged', 'neoforge')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1474.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1474
cd NeoForge-pr1474
curl -L https://prmaven.neoforged.net/NeoForge/pr1474/net/neoforged/neoforge/21.1.26-pr-1474-feature-naturalspawner-allocation-reduction/mdk-pr1474.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

@embeddedt
Copy link
Member

Personally, I think we should consider removing the deprecated methods in 21.2, the patch is unwieldy in its current state.

@HenryLoenwind
Copy link
Contributor

Personally, I think we should consider removing the deprecated methods in 21.2, the patch is unwieldy in its current state.

We don't remove vanilla methods. If we did, it would make a lot of patches way cleaner, though. That begs the question---are the reasons for that policy still valid?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.1 Targeted at Minecraft 1.21.1 enhancement New (or improvement to existing) feature or request performance Related to performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants