Skip to content

Conversation

@ProdPreva1l
Copy link

This removes the absurd allocation from the stream.

Optimises the CraftWorld#spawnParticle method by moving some logic from ServerLevel#sendParticlesSource to avoid a heavy stream and collect operation.

This optimisation is technically performance but the biggest improvement is with allocations from streams.

Optimises the CraftWorld#spawnParticle method by moving some logic from ServerLevel#sendParticlesSource to avoid a heavy stream and collect operation.
@ProdPreva1l ProdPreva1l requested a review from a team as a code owner October 28, 2025 14:00
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Oct 28, 2025
@lynxplay
Copy link
Contributor

Your PR contains a lot of unrelated changes from some dev branch Owen worked on. Please remove these.

@ProdPreva1l ProdPreva1l force-pushed the perf/optimise-spawn-particles branch from 47dbe32 to dd501fc Compare October 29, 2025 01:18
@ProdPreva1l
Copy link
Author

not too sure how i managed to mess that up

@SirYwell
Copy link
Contributor

Have you tried just using Lists.transform(receivers, p -> ((CraftPlayer) p).getHandle()) instead of the stream? That should cover most of the cost and would avoid copying NMS code that otherwise needs to be kept in sync manually.

@ProdPreva1l
Copy link
Author

Have you tried just using Lists.transform(receivers, p -> ((CraftPlayer) p).getHandle()) instead of the stream? That should cover most of the cost and would avoid copying NMS code that otherwise needs to be kept in sync manually.

This is no different to the stream in terms of allocations.
This change is meant to avoid that allocation.

I was originally just going to change the access of ServerLevel#sendParticlePacket to public so i didnt have to copy the entire method but didnt want to increase the patch diff

@SirYwell
Copy link
Contributor

This is no different to the stream in terms of allocations.
This change is meant to avoid that allocation.

It is very different because the Stream will allocate a materialized list while Lists.transform creates a small wrapper and applies the function on access. That's why I asked if you gave it a try. Generally seeing some profiling information would be helpful.

@masmc05
Copy link
Contributor

masmc05 commented Oct 29, 2025

This is no different to the stream in terms of allocations.
This change is meant to avoid that allocation.

How is allocation of a small helper list wrapper and a highly reusable lambda (jvm can be very smart about lambdas that don't use anything from the context) is the same as a complex structure like streams (and as just mentioned above a full list)?

@ProdPreva1l
Copy link
Author

after a quick test list transform is better than the stream, still not quite as good as directly looping over the original array but the difference is barely noticable

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

Labels

None yet

Projects

Status: Awaiting review

Development

Successfully merging this pull request may close these issues.

4 participants