Apply DataArray patch and update Bukkit adapters. #2181

JayemCeekay · 2023-04-17T16:00:55Z

Overview

Fixes #1938

Description

This pull request implements the patch for the use of type ambiguous DataArrays created by @SirYwell. This is necessary for the upcoming implementation of updated Forge and Fabric adapters in order to allow the registration and use of more than 65535 blockstates.

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.

SirYwell · 2023-06-01T17:21:27Z

I updated this branch to reflect the latest changes on main

SirYwell · 2023-08-18T19:21:05Z

Retargeted to v3 and rebased, though I need to re-review the changes myself because there were some ugly merge conflicts that probably broke something.

github-actions · 2023-10-22T11:10:37Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

github-actions · 2023-10-22T11:10:37Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

dordsor21

I kinda wonder if we should just move entirely to int based arrays. The amount of extra method calls for setting blocks required it quite substantial and will have a performance impact. I don't really see the memory impact being particularly large.

Also, clipboards will need to be investigated as they are very char based (and moving to an int-based system will halve the capacity of disk-based solutions)

dordsor21 · 2023-11-18T14:18:29Z

.../main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_17_R1_2/PaperweightGetBlocks.java

@@ -894,9 +881,9 @@ public char[] update(int layer, char[] data, boolean aggressive) {
                } else {
                    // The section's palette is the global block palette.
                    for (int i = 0; i < 4096; i++) {
-                        char paletteVal = data[i];
+                        char paletteVal = (char) data.getAt(i);


I think this (and the equivalent in all other adapters) should use int rather than char, else, the method breaks down when using an IntDataArray. Methods that return char should also be changed to int in adapters (e.g. adaptToChar, adaptToChar etc. The cached char[] array for ibdToStateOrdinal can also just be int[] - the memory impact is pretty negligible (in the 100s of KB)

dordsor21 · 2023-11-18T14:24:05Z

...ain/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R2/PaperweightPostProcessor.java

@@ -115,39 +116,39 @@ public ProcessorScope getScope() {
        return ProcessorScope.READING_SET_BLOCKS;
    }

-    private boolean wasAdjacentToWater(char[] get, char[] set, int i, int x, int y, int z) {
+    private boolean wasAdjacentToWater(DataArray get, DataArray set, int i, int x, int y, int z) {


this can also be moved to the method in the super PostProcessor class?

dordsor21 · 2023-11-18T14:27:28Z

...in/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R1/PaperweightGetBlocks_Copy.java

@@ -236,7 +237,7 @@ public <T extends Future<T>> T call(IChunkSet set, Runnable finalize) {
    public char get(int x, int y, int z) {
        final int layer = (y >> 4) - getMinSectionPosition();
        final int index = (y & 15) << 8 | z << 4 | x;
-        return blocks[layer][index];
+        return (char) blocks[layer].getAt(index);


likewise move to int for this method in all adapters too

dordsor21 · 2023-11-18T14:28:07Z

worldedit-bukkit/src/main/java/com/fastasyncworldedit/bukkit/adapter/NMSAdapter.java

            CachedBukkitAdapter adapter,
            short[] nonEmptyBlockCount
    ) {
        short nonAir = 4096;
        int num_palette = 0;
        for (int i = 0; i < 4096; i++) {
-            char ordinal = set[i];
+            char ordinal = (char) set.getAt(i);


This should all be moved to int as well

dordsor21 · 2023-11-18T14:28:41Z

worldedit-bukkit/src/main/java/com/fastasyncworldedit/bukkit/adapter/PostProcessor.java

+        if (set == null || get == null) {
+            return false;
+        }
+        char ordinal;


dordsor21 · 2023-11-18T14:31:30Z

worldedit-core/src/main/java/com/fastasyncworldedit/core/FaweCache.java

+    });
+
+    public final CleanableThreadLocal<int[]> PALETTE_TO_BLOCK_INT = new CleanableThreadLocal<>(
+            () -> new int[Character.MAX_VALUE + 1], a -> {


This should probably be initialised to block types cache size

dordsor21 · 2023-11-18T14:32:35Z

worldedit-core/src/main/java/com/fastasyncworldedit/core/FaweCache.java

-     * Convert raw int array to palette
-     * @return palette
-     */
-    public Palette toPalette(int layerOffset, int[] blocks) {


These methods being removed should be deprecated for removal and overloaded

dordsor21 · 2023-11-18T14:38:56Z

...c/main/java/com/fastasyncworldedit/core/queue/implementation/blocks/ChunkSectionedChunk.java

+    protected int maxSectionPosition;
+    protected int sectionCount;
+
+    static BiomeType getBiomeType(


I'm not a particular fan of having a random biome method here, I'm not really sure that this class is needed and I think it ends up adding more clutter than it aims to remove

dordsor21 · 2023-11-18T14:40:55Z

...ain/java/com/fastasyncworldedit/core/queue/implementation/blocks/ThreadUnsafeCharBlocks.java

- * Equivalent to {@link CharSetBlocks} without any attempt to make thread-safe for improved performance.
- * This is currently only used as a "copy" of {@link CharSetBlocks} to provide to
+ * Equivalent to {@link DataArraySetBlocks} without any attempt to make thread-safe for improved performance.
+ * This is currently only used as a "copy" of {@link DataArraySetBlocks} to provide to
 * {@link com.fastasyncworldedit.core.queue.IBatchProcessor} instances for processing without overlapping the continuing edit.
 *
 * @since 2.6.2


I suppose this should be renamed to ThreadUnsaveDataArrayBlocks

dordsor21 · 2023-11-18T14:42:48Z

worldedit-core/src/main/java/com/sk89q/worldedit/regions/Region.java

@@ -485,6 +477,23 @@ default IChunkSet processSet(IChunk chunk, IChunkGet get, IChunkSet set, boolean
        }
    }

+    private boolean isProcessExtra(IChunkSet set, boolean processExtra, int layer, DataArray arr) {


shouldProcessExtra?

github-actions · 2023-11-18T14:48:44Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

SirYwell · 2023-11-19T15:11:00Z

The casts to char should only be present in the bukkit module, but I'm fine with changing it to int there too.

I kinda wonder if we should just move entirely to int based arrays. The amount of extra method calls for setting blocks required it quite substantial and will have a performance impact. I don't really see the memory impact being particularly large.

I don't expect the methods to have measurable overhead over direct array access (we have one additional memory indirection until we have value classes, the methods are small, the JVM can inline them easily). Memory overhead of always using int[] might be fine, but that also mean more cache misses. It also might be interesting in future to explore Foreign Memory based approaches, in which case the DataArray abstraction would be useful too. So I think it's not worth to not have the DataArray abstraction.

Also, clipboards will need to be investigated as they are very char based (and moving to an int-based system will halve the capacity of disk-based solutions)

I started investigating that in https://github.com/IntellectualSites/FastAsyncWorldEdit/tree/feature/disk-based-clipboard, but there are more things to consider.

dordsor21 · 2023-11-20T14:10:34Z

The casts to char should only be present in the bukkit module, but I'm fine with changing it to int there too.

I'm not sure that's something we can 100% assume with datapacks, etc?

I don't expect the methods to have measurable overhead over direct array access (we have one additional memory indirection until we have value classes, the methods are small, the JVM can inline them easily). Memory overhead of always using int[] might be fine, but that also mean more cache misses. It also might be interesting in future to explore Foreign Memory based approaches, in which case the DataArray abstraction would be useful too. So I think it's not worth to not have the DataArray abstraction.

I'm not sure if an increased cache miss likelihood is really a large performance hit? I already assume that large edits that cannot be done per-chunk (large clipboard operations, any recursive operations) are already missing L1 (and very possibly L2 and L3 cache too) as the number of chunks being loaded and re-loaded is quite large. And then if a whole chunk's block arrays (max 200 KiB with char, 400 with int) are missing L3 cache we should be attempting to do something about that. Besides, I doubt that most servers have sole access to a single CPU to be able to be making full using the L1-3 cache anyway, and I would expect lots of cache misses due to running websites, other servers, services etc anyway. Switching to all int[] is more maintainable too

I started investigating that in feature/disk-based-clipboard, but there are more things to consider.

Ah yeah I'd forgotten about that, looks to make it work

SirYwell · 2023-11-20T16:46:36Z

I'm not sure that's something we can 100% assume with datapacks, etc?

AFAIK it's still not possible to have additional block types/states. Custom biomes are possible though.

I'm not sure if an increased cache miss likelihood is really a large performance hit

Most likely not, my point was more that we'd need to measure to really know the impact here (and even then, there are many parts that probably aren't true anymore in the next Java release).
I'd really like to keep the DataArray abstraction, there are a few things that are far easier to understand with that approach (e.g. avoiding System.arraycopy in the middle of already complex code, filling arrays, etc). It generally is more future-proof and will potentially allow for a bunch of optimizations in future.

JayemCeekay requested a review from a team as a code owner April 17, 2023 16:00

github-actions bot added the Feature This PR adds a new feature label Apr 17, 2023

SirYwell requested a review from dordsor21 April 17, 2023 16:25

SirYwell mentioned this pull request May 26, 2023

fix: Improve edit processing #2247

Merged

SirYwell changed the base branch from main to v3 August 18, 2023 18:34

JayemCeekay and others added 2 commits August 18, 2023 20:59

Apply DataArray Patch and update bukkit adapters.

4b9b60f

fix compile issues after merge

95bc238

SirYwell force-pushed the feature/DataArrays branch from f84b635 to 95bc238 Compare August 18, 2023 19:20

SirYwell added 4 commits August 19, 2023 09:16

Fix compilation issues and minimize diff

db01902

Reintroduce fast path for invalid blocks

565e6a6

Deduplicate getBiomeType logic

6c9ef7c

Fix index bugs

e79fa6f

SirYwell added the v3 label Aug 20, 2023

github-actions bot added unresolved-merge-conflict labels Oct 22, 2023

JayemCeekay added 5 commits October 31, 2023 12:01

Merge branch 'v3' into feature/DataArrays

e1babb7

Update HeightmapProcessor.java to fix merge conflict mistake

77b07ab

Update AbstractChangeSet.java

f69dc03

Update AbstractChangeSet.java

6fbd229

update 1.20.2 adapters to use DataArrays

6ce7f42

github-actions bot removed the unresolved-merge-conflict label Nov 2, 2023

dordsor21 requested changes Nov 18, 2023

View reviewed changes

github-actions bot added the unresolved-merge-conflict label Nov 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply DataArray patch and update Bukkit adapters. #2181

Apply DataArray patch and update Bukkit adapters. #2181

JayemCeekay commented Apr 17, 2023

SirYwell commented Jun 1, 2023

SirYwell commented Aug 18, 2023

github-actions bot commented Oct 22, 2023

github-actions bot commented Oct 22, 2023

dordsor21 left a comment

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023

github-actions bot commented Nov 18, 2023

SirYwell commented Nov 19, 2023

dordsor21 commented Nov 20, 2023 •

edited

SirYwell commented Nov 20, 2023

Apply DataArray patch and update Bukkit adapters. #2181

Are you sure you want to change the base?

Apply DataArray patch and update Bukkit adapters. #2181

Conversation

JayemCeekay commented Apr 17, 2023

Overview

Description

Submitter Checklist

SirYwell commented Jun 1, 2023

SirYwell commented Aug 18, 2023

github-actions bot commented Oct 22, 2023

github-actions bot commented Oct 22, 2023

dordsor21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 18, 2023

SirYwell commented Nov 19, 2023

dordsor21 commented Nov 20, 2023 • edited

SirYwell commented Nov 20, 2023

dordsor21 commented Nov 20, 2023 •

edited