-
Notifications
You must be signed in to change notification settings - Fork 6
Fix: Attempt delayed thread creation as long-term viable f3f lag spike fix #38
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.
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
Changes from all commits
feac345
aca35fc
c613ace
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,6 +103,7 @@ public ChunkRenderManager(SodiumWorldRenderer renderer, ChunkRenderBackend<T> ba | |
| this.world = world; | ||
|
|
||
| this.builder = new ChunkBuilder<>(backend.getVertexType(), this.backend); | ||
| this.builder.SetupWithRender(renderDistance); // Split out for compat... | ||
| this.builder.init(world, renderPassManager); | ||
|
|
||
| this.dirty = true; | ||
|
|
@@ -467,6 +468,8 @@ public void updateChunks() { | |
| if (!futures.isEmpty()) { | ||
| this.backend.upload(RenderDevice.INSTANCE.createCommandList(), new FutureDequeDrain<>(futures)); | ||
| } | ||
|
|
||
| this.builder.maybeIncreaseThreadLimit(); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| public void markDirty() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,14 +50,30 @@ public class ChunkBuilder<T extends ChunkGraphicsState> { | |
| private World world; | ||
| private BlockRenderPassManager renderPassManager; | ||
|
|
||
| private final int limitThreads; | ||
| private int limitThreads; | ||
| private int initialThreads; | ||
| private boolean hasThreadSpace = false; | ||
| private int updates = 0; | ||
|
|
||
| private final ChunkVertexType vertexType; | ||
| private final ChunkRenderBackend<T> backend; | ||
|
|
||
| public ChunkBuilder(ChunkVertexType vertexType, ChunkRenderBackend<T> backend) { | ||
| this.vertexType = vertexType; | ||
| this.backend = backend; | ||
| this.limitThreads = getOptimalThreadCount(); | ||
| // We assume nobody is running with 1 CPU, I guess. | ||
| this.initialThreads = this.limitThreads; | ||
| } | ||
|
|
||
| public void SetupWithRender(int viewDistance) { | ||
| // Choose threads based on viewDistance. | ||
| final int maxThreads = getOptimalThreadCount(); | ||
| this.limitThreads = maxThreads * Math.min(Math.max(viewDistance, 5), 32) / 32; | ||
| // We assume nobody is running with 1 CPU, I guess. | ||
| this.initialThreads = Math.max(2, this.limitThreads / 2 /* Arbitrarily chosen :) */); | ||
|
|
||
| this.hasThreadSpace = this.limitThreads > this.initialThreads; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -68,6 +84,19 @@ public int getSchedulingBudget() { | |
| return Math.max(0, (this.limitThreads * TASK_QUEUE_LIMIT_PER_WORKER) - this.buildQueue.size()); | ||
| } | ||
|
|
||
| private void spawnThread(MinecraftClient client) { | ||
| ChunkBuildBuffers buffers = new ChunkBuildBuffers(this.vertexType, this.renderPassManager); | ||
| ChunkRenderCacheLocal pipeline = new ChunkRenderCacheLocal(client, this.world); | ||
|
|
||
| WorkerRunnable worker = new WorkerRunnable(buffers, pipeline); | ||
|
|
||
| Thread thread = new Thread(worker, "Chunk Render Task Executor #" + this.threads.size()); | ||
| thread.setPriority(Math.max(0, Thread.NORM_PRIORITY - 2)); | ||
| thread.start(); | ||
|
|
||
| this.threads.add(thread); | ||
| } | ||
|
|
||
| /** | ||
| * Spawns a number of work-stealing threads to process results in the build queue. If the builder is already | ||
| * running, this method does nothing and exits. | ||
|
|
@@ -83,22 +112,42 @@ public void startWorkers() { | |
|
|
||
| MinecraftClient client = MinecraftClient.getInstance(); | ||
|
|
||
| for (int i = 0; i < this.limitThreads; i++) { | ||
| for (int i = 0; i < this.initialThreads; i++) { | ||
| // Don't change any of this code (even though we factored it out into spawnThread()) because SeedQueue | ||
| // injects like crazy in here. | ||
| ChunkBuildBuffers buffers = new ChunkBuildBuffers(this.vertexType, this.renderPassManager); | ||
| ChunkRenderCacheLocal pipeline = new ChunkRenderCacheLocal(client, this.world); | ||
|
|
||
| WorkerRunnable worker = new WorkerRunnable(buffers, pipeline); | ||
|
|
||
| Thread thread = new Thread(worker, "Chunk Render Task Executor #" + i); | ||
| Thread thread = new Thread(worker, "Chunk Render Task Executor #" + this.threads.size()); | ||
| thread.setPriority(Math.max(0, Thread.NORM_PRIORITY - 2)); | ||
| thread.start(); | ||
|
|
||
| this.threads.add(thread); | ||
| } | ||
|
|
||
| // More SeedQueue compatibility, ideally we would log the number of threads. | ||
| LOGGER.info("Started {} worker threads", this.threads.size()); | ||
| } | ||
|
|
||
| public void maybeIncreaseThreadLimit() | ||
| { | ||
| // Sometimes, we want more threads :) | ||
| // In that case, let's slowly add them! | ||
| // :) Doesn't everyone love more threads? | ||
| if (!this.hasThreadSpace || (this.updates++ < 60 /* Arbitrary! */) || !this.running.get()) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completely arbitrary value. I'm not sure if calls of this function are tied to ticks or to render updates (i.e. fps). I think it's likely the latter. In that case, we almost want to just use a clock, but I did it this way because polling system clocks can be a bit less performant. |
||
| return; | ||
| } | ||
| this.updates = 0; // Only gradually increase thread count. | ||
| this.spawnThread(MinecraftClient.getInstance()); | ||
| this.hasThreadSpace = this.threads.size() < this.limitThreads; | ||
| if (!this.hasThreadSpace) | ||
| { | ||
| LOGGER.info("Maxed out chunk builder threads at {}", this.threads.size()); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Notifies all worker threads to stop and blocks until all workers terminate. After the workers have been shut | ||
| * down, all tasks are cancelled and the pending queues are cleared. If the builder is already stopped, this | ||
|
|
@@ -113,6 +162,8 @@ public void stopWorkers() { | |
| throw new IllegalStateException("No threads are alive but the executor is in the RUNNING state"); | ||
| } | ||
|
|
||
| // Temporary seedqueue compatibility - log less so that its inject works | ||
| // LOGGER.info("Stopping {} worker threads", this.threads.size()); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SeedQueue suppresses some log messages, which limits our ability to change them. Ideally we would log the worker thread count here. Could just format a string then pass it in. |
||
| LOGGER.info("Stopping worker threads"); | ||
|
|
||
| // Notify all worker threads to wake up, where they will then terminate | ||
|
|
@@ -184,6 +235,7 @@ public boolean isBuildQueueEmpty() { | |
| * Initializes this chunk builder for the given world. If the builder is already running (which can happen during | ||
| * a world teleportation event), the worker threads will first be stopped and all pending tasks will be discarded | ||
| * before being started again. | ||
| * Note: The above comment appears to be false. We only ever call init() once, directly after ChunkBuilder creation. | ||
| * @param world The world instance | ||
| * @param renderPassManager The render pass manager used for the world | ||
| */ | ||
|
|
@@ -192,7 +244,11 @@ public void init(ClientWorld world, BlockRenderPassManager renderPassManager) { | |
| throw new NullPointerException("World is null"); | ||
| } | ||
|
|
||
| this.stopWorkers(); | ||
| if (this.running.get()) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this mostly for debugging purposes. However, unless another mod is directly calling in to |
||
| throw new IllegalStateException("Init called on already-running chunk builder."); | ||
| } | ||
|
|
||
| this.stopWorkers(); // Does nothing, ever. | ||
|
|
||
| this.world = world; | ||
| this.renderPassManager = renderPassManager; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is split out because other mods love injecting into Sodium. Ideally we would just pass this into the constructor.