IntellectualSites
Set Extent on the Copy -m Mask flag
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):
Copying a large selection with a "complex" mask & pasting (After fix):
Submitter Checklist
- [x] Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
- [x] Ensure that the pull request title represents the desired changelog entry.
- [x] New public fields and methods are annotated with
@since TODO. - [x] I read and followed the contribution guidelines.
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.
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 :)
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?
1.21 (fast case): https://spark.lucko.me/EjX9IPlk75 1.20.6 (slow case): https://spark.lucko.me/T0zzmO5OQx
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?
Is it
that in 1.20.6
net.minecraft.world.level.Level.getChunk() calls
net.minecraft.world.level.ServerChunkCache.getChunk()
while in 1.21
net.minecraft.world.level.Level.getChunk() calls
net.minecraft.server.level.ServerChunkCache.getChunkAtIfLoadedImmediately()
?
Interesting - though it seems we currently directly access the world here, which is what we should avoid and what this PR addresses I guess.
I will test out your suggestion to move this to setSourceMask tomorrow and update :)
After taking a closer look, setSourceMask is probably not the right place. But we have similar code already here: https://github.com/IntellectualSites/FastAsyncWorldEdit/blob/f3860618ad157e406a8b6999348f8477317defbe/worldedit-core/src/main/java/com/sk89q/worldedit/function/operation/ForwardExtentCopy.java#L352-L355 This only affects situations with transformations involved, where https://github.com/IntellectualSites/FastAsyncWorldEdit/blob/f3860618ad157e406a8b6999348f8477317defbe/worldedit-core/src/main/java/com/sk89q/worldedit/function/operation/ForwardExtentCopy.java#L414-L420 is missing that part, basically. Adding it in that if would be good I think.
