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)

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

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 on main branch

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 ThreadUnsafeDataArrayBlocks

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?

Should probably actually call the method sometihng that implies it's doing something to the data at all

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.

github-actions · 2024-09-26T18:33:28Z

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

SirYwell · 2024-10-26T09:32:37Z

I made this PR compatible with the Vector API changes now - somewhat hacky, but seems to work.

dordsor21

Most of previous comments still apply

dordsor21 · 2024-10-26T10:49:42Z

.../src/main/java/com/fastasyncworldedit/core/queue/implementation/SingleThreadQueueExtent.java

@@ -188,6 +188,7 @@ public synchronized void init(Extent extent, IChunkCache<IChunkGet> get, IChunkC
        }
    }

+


Remove newline

dordsor21 · 2024-10-26T11:12:51Z

...core/src/main/java/com/fastasyncworldedit/core/queue/implementation/blocks/IntDataArray.java

+
+    @Override
+    public <V> void processSet(final DataArray get, final BinaryOperator<? extends Vector<V>> op) {
+        // TODO is this sane???


I think the alternative would be to litter generics for vectors through a lot of classes (if that's even possible?)

dordsor21 · 2024-10-26T11:13:55Z

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

@@ -184,18 +183,12 @@ public char get(int x, int y, int z) {
            return defaultOrdinal;
        }
        final int index = (y & 15) << 8 | z << 4 | x;
-        return blocks[layer - minSectionPosition][index];
+        return (char) blocks[layer - minSectionPosition].getAt(index);


Needs moving to int

dordsor21 · 2024-10-26T13:10:57Z

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) {


Should probably actually call the method sometihng that implies it's doing something to the data at all

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

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

SirYwell added the v3 label Aug 20, 2023

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

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

SirYwell mentioned this pull request Aug 21, 2024

Introduce basic support for Vector API #2890

Merged

SirYwell force-pushed the v3 branch from 111eebe to c0edab6 Compare August 22, 2024 09:38

SirYwell force-pushed the feature/DataArrays branch from 6ce7f42 to aadd6ac Compare August 22, 2024 11:22

github-actions bot removed the unresolved-merge-conflict label Aug 22, 2024

dordsor21 force-pushed the v3 branch from c0edab6 to de49924 Compare September 26, 2024 18:31

github-actions bot added the unresolved-merge-conflict label Sep 26, 2024

SirYwell force-pushed the v3 branch 2 times, most recently from a0de1d0 to 9fc47e9 Compare October 26, 2024 09:26

SirYwell force-pushed the feature/DataArrays branch from aadd6ac to 6dc38ef Compare October 26, 2024 09:31

github-actions bot removed the unresolved-merge-conflict label Oct 26, 2024

Apply DataArray Patch and update bukkit adapters.

4684f16

SirYwell force-pushed the feature/DataArrays branch from 6dc38ef to 4684f16 Compare October 26, 2024 10:07

dordsor21 reviewed Oct 26, 2024

View reviewed changes

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 •

edited

Loading

dordsor21 Nov 18, 2023

dordsor21 Nov 18, 2023 •

edited

Loading

dordsor21 Nov 18, 2023

dordsor21 Oct 26, 2024

github-actions bot commented Nov 18, 2023

SirYwell commented Nov 19, 2023

dordsor21 commented Nov 20, 2023 •

edited

Loading

SirYwell commented Nov 20, 2023

github-actions bot commented Sep 26, 2024

SirYwell commented Oct 26, 2024

dordsor21 left a comment

dordsor21 Oct 26, 2024

dordsor21 Oct 26, 2024

dordsor21 Oct 26, 2024

dordsor21 Oct 26, 2024

		@@ -188,6 +188,7 @@ public synchronized void init(Extent extent, IChunkCache<IChunkGet> get, IChunkC
		}
		}

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

dordsor21 Nov 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dordsor21 Nov 18, 2023 • edited Loading

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 Loading

SirYwell commented Nov 20, 2023

github-actions bot commented Sep 26, 2024

SirYwell commented Oct 26, 2024

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

dordsor21 Nov 18, 2023 •

edited

Loading

dordsor21 Nov 18, 2023 •

edited

Loading

dordsor21 commented Nov 20, 2023 •

edited

Loading