From b5b79abc7434edd3ebe7a42439d2d6e0fb7f93f8 Mon Sep 17 00:00:00 2001 From: Thiakil Date: Tue, 24 Dec 2024 15:54:04 +0800 Subject: [PATCH 1/3] move SavedData.save to IO worker to avoid synchronous IO on main thread by default --- .../server/level/ServerLevel.java.patch | 5 +++- .../util/worldupdate/WorldUpgrader.java.patch | 10 +++++++ .../level/saveddata/SavedData.java.patch | 9 ++++-- .../neoforge/common/IOUtilities.java | 28 +++++++++++++++++ .../unittest/IOUtilitiesWorkerTest.java | 30 +++++++++++++++++++ 5 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 patches/net/minecraft/util/worldupdate/WorldUpgrader.java.patch create mode 100644 tests/src/junit/java/net/neoforged/neoforge/unittest/IOUtilitiesWorkerTest.java diff --git a/patches/net/minecraft/server/level/ServerLevel.java.patch b/patches/net/minecraft/server/level/ServerLevel.java.patch index 411052eadcc..51b6932ad67 100644 --- a/patches/net/minecraft/server/level/ServerLevel.java.patch +++ b/patches/net/minecraft/server/level/ServerLevel.java.patch @@ -112,11 +112,14 @@ this.getProfiler().pop(); for (Entity entity : p_8648_.getPassengers()) { -@@ -807,6 +_,7 @@ +@@ -807,6 +_,10 @@ } else { this.entityManager.autoSave(); } + net.neoforged.neoforge.common.NeoForge.EVENT_BUS.post(new net.neoforged.neoforge.event.level.LevelEvent.Save(this)); ++ if (p_8645_) { ++ net.neoforged.neoforge.common.IOUtilities.waitUntilIOWorkerComplete(); ++ } } } diff --git a/patches/net/minecraft/util/worldupdate/WorldUpgrader.java.patch b/patches/net/minecraft/util/worldupdate/WorldUpgrader.java.patch new file mode 100644 index 00000000000..8057bb1050f --- /dev/null +++ b/patches/net/minecraft/util/worldupdate/WorldUpgrader.java.patch @@ -0,0 +1,10 @@ +--- a/net/minecraft/util/worldupdate/WorldUpgrader.java ++++ b/net/minecraft/util/worldupdate/WorldUpgrader.java +@@ -111,6 +_,7 @@ + LOGGER.info("Upgrading blocks"); + new WorldUpgrader.ChunkUpgrader().upgrade(); + this.overworldDataStorage.save(); ++ net.neoforged.neoforge.common.IOUtilities.waitUntilIOWorkerComplete(); + i = Util.getMillis() - i; + LOGGER.info("World optimizaton finished after {} seconds", i / 1000L); + this.finished = true; diff --git a/patches/net/minecraft/world/level/saveddata/SavedData.java.patch b/patches/net/minecraft/world/level/saveddata/SavedData.java.patch index ac130d73e8a..c793017cf33 100644 --- a/patches/net/minecraft/world/level/saveddata/SavedData.java.patch +++ b/patches/net/minecraft/world/level/saveddata/SavedData.java.patch @@ -1,15 +1,20 @@ --- a/net/minecraft/world/level/saveddata/SavedData.java +++ b/net/minecraft/world/level/saveddata/SavedData.java -@@ -37,7 +_,7 @@ +@@ -36,18 +_,23 @@ + compoundtag.put("data", this.save(new CompoundTag(), p_324088_)); NbtUtils.addCurrentDataVersion(compoundtag); ++ net.neoforged.neoforge.common.IOUtilities.withIOWorker(()->{ try { - NbtIo.writeCompressed(compoundtag, p_77758_.toPath()); + net.neoforged.neoforge.common.IOUtilities.writeNbtCompressed(compoundtag, p_77758_.toPath()); } catch (IOException ioexception) { LOGGER.error("Could not save data {}", this, ioexception); } -@@ -47,7 +_,10 @@ ++ }); + + this.setDirty(false); + } } public static record Factory( diff --git a/src/main/java/net/neoforged/neoforge/common/IOUtilities.java b/src/main/java/net/neoforged/neoforge/common/IOUtilities.java index 8c2165a2639..daa9defa032 100644 --- a/src/main/java/net/neoforged/neoforge/common/IOUtilities.java +++ b/src/main/java/net/neoforged/neoforge/common/IOUtilities.java @@ -17,10 +17,15 @@ import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.nio.file.attribute.BasicFileAttributes; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; import java.util.function.BiPredicate; import java.util.function.Consumer; import net.minecraft.nbt.CompoundTag; import net.minecraft.nbt.NbtIo; +import net.minecraft.util.thread.NamedThreadFactory; import org.apache.commons.io.output.CloseShieldOutputStream; import org.jetbrains.annotations.Nullable; @@ -33,9 +38,15 @@ public final class IOUtilities { StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING }; + private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("Neoforge-IOUtilities"); + private static ExecutorService ioExecutor = newIOExecutor(); private IOUtilities() {} + private static ExecutorService newIOExecutor() { + return Executors.newSingleThreadExecutor(THREAD_FACTORY); + } + /** * Cleans up any temporary files that may have been left over from interrupted * calls to {@link #atomicWrite(Path, WriteCallback)}. @@ -152,6 +163,23 @@ public static void atomicWrite(Path targetPath, WriteCallback writeCallback) thr } } + public static void withIOWorker(Runnable task) { + ioExecutor.execute(task); + } + + public static void waitUntilIOWorkerComplete() { + ExecutorService old = ioExecutor; + ioExecutor = newIOExecutor(); + old.shutdown(); + try { + if (!old.awaitTermination(1L, TimeUnit.HOURS)) { + throw new RuntimeException("Timed out waiting for IO worker to complete"); + } + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + /** * Declares an interface which is functionally equivalent to {@link Consumer}, * except supports the ability to throw IOExceptions that may occur. diff --git a/tests/src/junit/java/net/neoforged/neoforge/unittest/IOUtilitiesWorkerTest.java b/tests/src/junit/java/net/neoforged/neoforge/unittest/IOUtilitiesWorkerTest.java new file mode 100644 index 00000000000..6a636dc1275 --- /dev/null +++ b/tests/src/junit/java/net/neoforged/neoforge/unittest/IOUtilitiesWorkerTest.java @@ -0,0 +1,30 @@ +/* + * Copyright (c) NeoForged and contributors + * SPDX-License-Identifier: LGPL-2.1-only + */ + +package net.neoforged.neoforge.unittest; + +import java.util.concurrent.atomic.AtomicBoolean; +import net.neoforged.neoforge.common.IOUtilities; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class IOUtilitiesWorkerTest { + /** + * Tests that waiting on the IO worker will complete remaining tasks + */ + @Test + void testWaitOnWorker() { + AtomicBoolean value = new AtomicBoolean(false); + IOUtilities.withIOWorker(() -> { + try { + Thread.sleep(2000); + } catch (InterruptedException ignored) {} + value.set(true); + }); + Assertions.assertFalse(value.get(), "Value should not be set yet"); + IOUtilities.waitUntilIOWorkerComplete(); + Assertions.assertTrue(value.get(), "Value should be set after waiting"); + } +} From c9501e341e0f36252131502c319b3233bd7a0d1a Mon Sep 17 00:00:00 2001 From: Xander Date: Wed, 25 Dec 2024 10:39:41 +0800 Subject: [PATCH 2/3] Update thread name per feedback Co-authored-by: Bruno Ploumhans <13494793+Technici4n@users.noreply.github.com> --- src/main/java/net/neoforged/neoforge/common/IOUtilities.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/net/neoforged/neoforge/common/IOUtilities.java b/src/main/java/net/neoforged/neoforge/common/IOUtilities.java index daa9defa032..f91dc097f7f 100644 --- a/src/main/java/net/neoforged/neoforge/common/IOUtilities.java +++ b/src/main/java/net/neoforged/neoforge/common/IOUtilities.java @@ -38,7 +38,7 @@ public final class IOUtilities { StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING }; - private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("Neoforge-IOUtilities"); + private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("NeoForge-IOUtilities"); private static ExecutorService ioExecutor = newIOExecutor(); private IOUtilities() {} From 4a54dc029b0d741f52a2afa54b36ba9aea84e544 Mon Sep 17 00:00:00 2001 From: Thiakil Date: Fri, 27 Dec 2024 20:20:18 +0800 Subject: [PATCH 3/3] add a copy to account for mods appending existing data --- .../minecraft/world/level/saveddata/SavedData.java.patch | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/patches/net/minecraft/world/level/saveddata/SavedData.java.patch b/patches/net/minecraft/world/level/saveddata/SavedData.java.patch index c793017cf33..8d1edf0946b 100644 --- a/patches/net/minecraft/world/level/saveddata/SavedData.java.patch +++ b/patches/net/minecraft/world/level/saveddata/SavedData.java.patch @@ -1,13 +1,16 @@ --- a/net/minecraft/world/level/saveddata/SavedData.java +++ b/net/minecraft/world/level/saveddata/SavedData.java -@@ -36,18 +_,23 @@ +@@ -36,18 +_,26 @@ compoundtag.put("data", this.save(new CompoundTag(), p_324088_)); NbtUtils.addCurrentDataVersion(compoundtag); ++ //copy the contents to handle mods that just append an existing Tag to the Compound ++ CompoundTag copied = compoundtag.copy(); ++ + net.neoforged.neoforge.common.IOUtilities.withIOWorker(()->{ try { - NbtIo.writeCompressed(compoundtag, p_77758_.toPath()); -+ net.neoforged.neoforge.common.IOUtilities.writeNbtCompressed(compoundtag, p_77758_.toPath()); ++ net.neoforged.neoforge.common.IOUtilities.writeNbtCompressed(copied, p_77758_.toPath()); } catch (IOException ioexception) { LOGGER.error("Could not save data {}", this, ioexception); }