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.

Sign up for GitHub

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

Async chunk loading for spigot/paper 1.21.3/1.21.4 #4188

Open
wants to merge 1 commit into
base: v3.0
Choose a base branch
from
Open

Async chunk loading for spigot/paper 1.21.3/1.21.4 #4188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...r-121-3/src/main/java/org/dynmap/bukkit/helper/v121_3/BukkitVersionHelperSpigot121_3.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public class BukkitVersionHelperSpigot121_3 extends BukkitVersionHelper {

@Override
public boolean isUnsafeAsync() {
return true;
return false;
}

/**
Expand Down
47 changes: 47 additions & 0 deletions bukkit-helper-121-3/src/main/java/org/dynmap/bukkit/helper/v121_3/MapChunkCache121_3.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.bukkit.World;
import org.bukkit.craftbukkit.v1_21_R2.CraftWorld;
import org.dynmap.DynmapChunk;
import org.dynmap.Log;
import org.dynmap.bukkit.helper.BukkitWorld;
import org.dynmap.common.BiomeMap;
import org.dynmap.common.chunk.GenericChunk;
Expand All @@ -18,6 +19,9 @@
import java.util.List;
import java.util.NoSuchElementException;
import java.util.concurrent.CancellationException;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.function.Supplier;

/**
* Container for managing chunks - dependent upon using chunk snapshots, since rendering is off server thread
Expand All @@ -31,6 +35,31 @@ public MapChunkCache121_3(GenericChunkCache cc) {
super(cc);
}

@Override
protected Supplier<GenericChunk> getLoadedChunkAsync(DynmapChunk chunk) {
CompletableFuture<GenericChunk> genericChunk = CompletableFuture.supplyAsync(() -> {
CraftWorld cw = (CraftWorld) w;
Chunk c = cw.getHandle().getChunkIfLoaded(chunk.x, chunk.z);
if (c == null || !c.q) { //!c.loaded
return null;
}
SerializableChunkData chunkData = SerializableChunkData.a(cw.getHandle(), c); //SerializableChunkData.copyOf
NBTTagCompound nbt = chunkData.a(); // SerializableChunkData.write
return parseChunkFromNBT(new NBT.NBTCompound(nbt));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda missed a bit of the impl description, up to SerializableChunkData.a(cw.getHandle(), c) needs to be executed on main thread, the part bellow it should be moved into the supplier so it would be executed async

Basically the cf should return SerializableChunkData and not the parsed chunk

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why? I can see no obvious reason to not load in on the parallel thread. No need to stress the main thread here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's saving data that is really not liking concurrent access, and accessing chunk in async in Minecraft is always calling for issues

});
return () -> {
try {
return genericChunk.get();
} catch (InterruptedException e) {
Log.severe("Error loading cached chunk async", e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
Log.severe("Error loading cached chunk async", e);
}
return null;
};
}

protected GenericChunk getLoadedChunk(DynmapChunk chunk) {
CraftWorld cw = (CraftWorld) w;
if (!cw.isChunkLoaded(chunk.x, chunk.z)) return null;
Expand All @@ -41,6 +70,24 @@ protected GenericChunk getLoadedChunk(DynmapChunk chunk) {
return nbt != null ? parseChunkFromNBT(new NBT.NBTCompound(nbt)) : null;
}

@Override
protected Supplier<GenericChunk> loadChunkAsync(DynmapChunk chunk){
CraftWorld cw = (CraftWorld) w;
CompletableFuture<GenericChunk> genericChunk = cw.getHandle().m().a.d(new ChunkCoordIntPair(chunk.x, chunk.z)) // WorldServer.getChunkSource().chunkMap.read(new ChunkCoordIntPair(chunk.x, chunk.z))
.thenApply(opt -> opt.map(NBT.NBTCompound::new).map(this::parseChunkFromNBT).orElse(null));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to supplier to make sure it's not gonna be executed on main thread

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how async and cf work. All operations chained with "then" will get executed on the thread the load itself is executed. So basically the return value is not that elegant. In theory the cf itself should be returned, but we are bound to the given interface. So moving this to the supplier will result in getting it executed async when the supplier is requested, but why wait when we can start loading it right now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All operations chained with "then" will get executed on the thread the load itself is executed.

Yep, and that's what you should avoid, it's not a thread that you can say with confidence that you can stress with additional load. And this is the reason why you should move it to supplier, as you'll for sure be in async thread pool that you can load with stuff like parsing the chunk

return () -> {
try {
return genericChunk.get();
} catch (InterruptedException e) {
Log.severe("Error loading chunk async", e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
Log.severe("Error loading chunk async", e);
}
return null;
};
}

protected GenericChunk loadChunk(DynmapChunk chunk) {
CraftWorld cw = (CraftWorld) w;
NBTTagCompound nbt = null;
Expand Down
2 changes: 1 addition & 1 deletion ...r-121-4/src/main/java/org/dynmap/bukkit/helper/v121_4/BukkitVersionHelperSpigot121_4.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public class BukkitVersionHelperSpigot121_4 extends BukkitVersionHelper {

@Override
public boolean isUnsafeAsync() {
return true;
return false;
}

/**
Expand Down
47 changes: 47 additions & 0 deletions bukkit-helper-121-4/src/main/java/org/dynmap/bukkit/helper/v121_4/MapChunkCache121_4.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.bukkit.World;
import org.bukkit.craftbukkit.v1_21_R3.CraftWorld;
import org.dynmap.DynmapChunk;
import org.dynmap.Log;
import org.dynmap.bukkit.helper.BukkitWorld;
import org.dynmap.common.BiomeMap;
import org.dynmap.common.chunk.GenericChunk;
Expand All @@ -18,6 +19,9 @@
import java.util.List;
import java.util.NoSuchElementException;
import java.util.concurrent.CancellationException;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
import java.util.function.Supplier;

/**
* Container for managing chunks - dependent upon using chunk snapshots, since rendering is off server thread
Expand All @@ -31,6 +35,31 @@ public MapChunkCache121_4(GenericChunkCache cc) {
super(cc);
}

@Override
protected Supplier<GenericChunk> getLoadedChunkAsync(DynmapChunk chunk) {
CompletableFuture<GenericChunk> genericChunk = CompletableFuture.supplyAsync(() -> {
CraftWorld cw = (CraftWorld) w;
Chunk c = cw.getHandle().getChunkIfLoaded(chunk.x, chunk.z);
if (c == null || !c.q) { //!c.loaded
return null;
}
SerializableChunkData chunkData = SerializableChunkData.a(cw.getHandle(), c); //SerializableChunkData.copyOf
NBTTagCompound nbt = chunkData.a(); // SerializableChunkData.write
return parseChunkFromNBT(new NBT.NBTCompound(nbt));
});
return () -> {
try {
return genericChunk.get();
} catch (InterruptedException e) {
Log.severe("Error loading cached chunk async", e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
Log.severe("Error loading cached chunk async", e);
}
return null;
};
}

protected GenericChunk getLoadedChunk(DynmapChunk chunk) {
CraftWorld cw = (CraftWorld) w;
if (!cw.isChunkLoaded(chunk.x, chunk.z)) return null;
Expand All @@ -41,6 +70,24 @@ protected GenericChunk getLoadedChunk(DynmapChunk chunk) {
return nbt != null ? parseChunkFromNBT(new NBT.NBTCompound(nbt)) : null;
}

@Override
protected Supplier<GenericChunk> loadChunkAsync(DynmapChunk chunk){
CraftWorld cw = (CraftWorld) w;
CompletableFuture<GenericChunk> genericChunk = cw.getHandle().m().a.d(new ChunkCoordIntPair(chunk.x, chunk.z)) // WorldServer.getChunkSource().chunkMap.read(new ChunkCoordIntPair(chunk.x, chunk.z))
.thenApply(opt -> opt.map(NBT.NBTCompound::new).map(this::parseChunkFromNBT).orElse(null));
return () -> {
try {
return genericChunk.get();
} catch (InterruptedException e) {
Log.severe("Error loading chunk async", e);
Thread.currentThread().interrupt();
} catch (ExecutionException e) {
Log.severe("Error loading chunk async", e);
}
return null;
};
}

protected GenericChunk loadChunk(DynmapChunk chunk) {
CraftWorld cw = (CraftWorld) w;
NBTTagCompound nbt = null;
Expand Down