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

Set Extent on the Copy -m Mask flag #3130

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Zeranny
Copy link
Contributor

@Zeranny Zeranny commented Mar 2, 2025

Overview

While developing ezEdits @eztaK-red and I noticed that not using a MaskTraverser to set the extent on a mask results in significantly slower performance.

We assumed that this could also be why we noticed slow performance in the -m flag for //copy, and that does appear to be the case.

There may be other instances of masks in FAWE not having an extent set where performance could be improved, but I didn't want to get ahead of myself in case there were issues with the way that I have fixed it here.
Especially as during testing it appears older versions of FAWE (2.11.1) were even slower, so changes since then may have already had a positive impact on -m performance.

Description

Sets the extent for the finalMask in the createCopy() method used by //copy which (going by previous FAWE commits) avoids using a WorldWrapper.

Rough runtime comparison using -m #solid,>[!air],[/[0][90]],[|air] (Before & After fix):
2025-03-02_17 53 41
2025-03-02_18 03 13

Copying a large selection with a "complex" mask & pasting (After fix):
2025-03-02_17 35 57
2025-03-02_17 36 21

Submitter Checklist

  • Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
  • Ensure that the pull request title represents the desired changelog entry.
  • New public fields and methods are annotated with @since TODO.
  • I read and followed the contribution guidelines.

@Zeranny Zeranny requested a review from a team as a code owner March 2, 2025 17:42
@Zeranny Zeranny marked this pull request as draft March 2, 2025 17:46
@Zeranny Zeranny changed the title Set Extent on the Copy Mask to greatly improve performance Set Extent on the Copy -m Mask flag Mar 2, 2025
@eztaK-red
Copy link
Contributor

eztaK-red commented Mar 4, 2025

Investigating a bit further:

Paper-1.20.6 (tested #151) or older:

  • newest FAWE (#1061): //copy -m: 52s
  • this-PR's FAWE version: //copy -m: <1s

Paper-1.21 (tested #103) or newer:

  • newest FAWE (#1061): //copy -m: <1s
  • this-PR's FAWE version: //copy -m: <1s

This bug, of //copy -m and //cut -m taking significantly longer than it should, appears to affect even the newest FAWE versions if they are running on Paper 1.20.6 or older.

Furthermore, somehow, it's much better when the server is running on 1.21, independent of the FAWE version.

@Zeranny Zeranny marked this pull request as ready for review March 4, 2025 17:44
@Zeranny
Copy link
Contributor Author

Zeranny commented Mar 4, 2025

Re-activated this PR. I had set it back to draft as it was unclear exactly what scenarios led to slow performance.
From my own testing it does appear that on MC 1.21.4 the performance for very large copies is still sometimes faster with this change, but that my be a bit random.

I would appreciate any further insight into the underlying performance difference :)

@SirYwell
Copy link
Member

SirYwell commented Mar 4, 2025

I don't know if you tested that on the same FAWE build on the different versions, and also with the same configuration. There were quite a few changes in the last months that could affect performance. Can you provide profiles (e.g. through spark, or the IntelliJ profiler) especially of the slow case?

Other than that: Would it make sense to move your change into setSourceMask instead?

@eztaK-red
Copy link
Contributor

1.21 (fast case): https://spark.lucko.me/EjX9IPlk75
1.20.6 (slow case): https://spark.lucko.me/T0zzmO5OQx

@SirYwell
Copy link
Member

SirYwell commented Mar 4, 2025

Hm, from a quick look, I assume that chunks are loaded in the fast case but not in the slow one. Could that be the case?

@eztaK-red
Copy link
Contributor

Is it
that in 1.20.6
net.minecraft.world.level.Level.getChunk() calls
net.minecraft.world.level.ServerChunkCache.getChunk()
Screenshot_20250304_193942

while in 1.21
net.minecraft.world.level.Level.getChunk() calls
net.minecraft.server.level.ServerChunkCache.getChunkAtIfLoadedImmediately()
image
?

@SirYwell
Copy link
Member

SirYwell commented Mar 4, 2025

Interesting - though it seems we currently directly access the world here, which is what we should avoid and what this PR addresses I guess.

@Zeranny
Copy link
Contributor Author

Zeranny commented Mar 4, 2025

I will test out your suggestion to move this to setSourceMask tomorrow and update :)

@SirYwell
Copy link
Member

SirYwell commented Mar 5, 2025

After taking a closer look, setSourceMask is probably not the right place. But we have similar code already here:

if (sourceMask != Masks.alwaysTrue()) {
new MaskTraverser(sourceMask).reset(transExt);
copy = new RegionMaskingFilter(sourceMask, copy);
}

This only affects situations with transformations involved, where
if (sourceMask != Masks.alwaysTrue()) {
if (maskFunc != null) {
copy = new RegionMaskTestFunction(sourceMask, copy, maskFunc);
} else {
copy = new RegionMaskingFilter(sourceMask, copy);
}
}

is missing that part, basically. Adding it in that if would be good I think.

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