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.

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

feat: make number of chunk processing threads configurable #5237

Merged
merged 3 commits into from
Apr 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
import org.terasology.engine.config.Config;
import org.terasology.engine.config.RenderingConfig;
import org.terasology.engine.entitySystem.entity.EntityManager;
import org.terasology.engine.entitySystem.entity.EntityRef;
import org.terasology.engine.world.BlockEntityRegistry;
Expand Down Expand Up @@ -43,6 +45,7 @@
import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

class LocalChunkProviderTest {

Expand All @@ -54,6 +57,7 @@ class LocalChunkProviderTest {
private ExtraBlockDataManager extraDataManager;
private BlockEntityRegistry blockEntityRegistry;
private EntityRef worldEntity;
private Config config;
private Map<Vector3ic, Chunk> chunkCache;
private Block blockAtBlockManager;
private TestStorageManager storageManager;
Expand All @@ -71,13 +75,18 @@ public void setUp() {
blockEntityRegistry = mock(BlockEntityRegistry.class);
worldEntity = mock(EntityRef.class);
chunkCache = Maps.newConcurrentMap();
config = mock(Config.class);
RenderingConfig renderConfig = mock(RenderingConfig.class);
when(renderConfig.getChunkThreads()).thenReturn(0);
when(config.getRendering()).thenReturn(renderConfig);
storageManager = new TestStorageManager();
generator = new TestWorldGenerator(blockManager);
chunkProvider = new LocalChunkProvider(storageManager,
entityManager,
generator,
blockManager,
extraDataManager,
config,
chunkCache);
chunkProvider.setBlockEntityRegistry(blockEntityRegistry);
chunkProvider.setWorldEntity(worldEntity);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class ChunkProcessingPipelineTest extends TerasologyTestingEnvironment {

@Test
void simpleProcessingSuccess() throws ExecutionException, InterruptedException, TimeoutException {
pipeline = new ChunkProcessingPipeline((p) -> null, (o1, o2) -> 0);
pipeline = new ChunkProcessingPipeline(0, (p) -> null, (o1, o2) -> 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A value of 0 to represent "Automatic" does seem a bit unexpected in hindsight, Do you think another value would make sense? Or maybe wrapping it in a constant would be enough?


Vector3i chunkPos = new Vector3i(0, 0, 0);
Chunk chunk = createChunkAt(chunkPos);
Expand All @@ -67,7 +67,7 @@ void simpleProcessingSuccess() throws ExecutionException, InterruptedException,

@Test
void simpleStopProcessingSuccess() {
pipeline = new ChunkProcessingPipeline((p) -> null, (o1, o2) -> 0);
pipeline = new ChunkProcessingPipeline(0, (p) -> null, (o1, o2) -> 0);

Vector3i position = new Vector3i(0, 0, 0);
Chunk chunk = createChunkAt(position);
Expand Down Expand Up @@ -106,7 +106,7 @@ void multiRequirementsChunksExistsSuccess() throws ExecutionException, Interrupt
Function.identity()
));

pipeline = new ChunkProcessingPipeline(chunkCache::get, (o1, o2) -> 0);
pipeline = new ChunkProcessingPipeline(0, chunkCache::get, (o1, o2) -> 0);
pipeline.addStage(ChunkTaskProvider.createMulti(
"flat merging task",
(chunks) -> chunks.stream()
Expand Down Expand Up @@ -140,7 +140,7 @@ void multiRequirementsChunksWillGeneratedSuccess() throws ExecutionException, In
Function.identity()
));

pipeline = new ChunkProcessingPipeline((p) -> null, (o1, o2) -> 0);
pipeline = new ChunkProcessingPipeline(0, (p) -> null, (o1, o2) -> 0);
pipeline.addStage(ChunkTaskProvider.createMulti(
"flat merging task",
(chunks) -> chunks.stream()
Expand Down Expand Up @@ -169,7 +169,7 @@ void emulateEntityMoving() throws InterruptedException {
final AtomicReference<Vector3ic> position = new AtomicReference<>();
Map<Vector3ic, Future<Chunk>> futures = Maps.newHashMap();
Map<Vector3ic, Chunk> chunkCache = Maps.newConcurrentMap();
pipeline = new ChunkProcessingPipeline(chunkCache::get, (o1, o2) -> {
pipeline = new ChunkProcessingPipeline(0, chunkCache::get, (o1, o2) -> {
if (position.get() != null) {
Vector3ic entityPos = position.get();
return (int) (entityPos.distance(((PositionFuture<?>) o1).getPosition())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public class RenderingConfig extends AbstractSubscribable {
public static final String ANIMATED_MENU = "AnimatedMenu";
public static final String VIEW_DISTANCE = "viewDistance";
public static final String CHUNK_LODS = "chunkLods";
public static final String CHUNK_THREADS = "chunkThreads";
public static final String BILLBOARD_LIMIT = "billboardLimit";
public static final String FLICKERING_LIGHT = "FlickeringLight";
public static final String ANIMATE_GRASS = "AnimateGrass";
Expand Down Expand Up @@ -76,6 +77,7 @@ public class RenderingConfig extends AbstractSubscribable {
private boolean animatedMenu;
private ViewDistance viewDistance;
private float chunkLods;
private int chunkThreads;
private float billboardLimit;
private boolean flickeringLight;
private boolean animateGrass;
Expand Down Expand Up @@ -270,6 +272,16 @@ public void setChunkLods(float chunkLods) {
propertyChangeSupport.firePropertyChange(CHUNK_LODS, oldLods, chunkLods);
}

public int getChunkThreads() {
return chunkThreads;
}

public void setChunkThreads(int chunkThreads) {
float oldChunkThreads = this.chunkThreads;
this.chunkThreads = chunkThreads;
propertyChangeSupport.firePropertyChange(CHUNK_THREADS, oldChunkThreads, chunkThreads);
}

public float getBillboardLimit() {
return billboardLimit;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

package org.terasology.engine.core.modes.loadProcesses;

import org.terasology.engine.config.Config;
import org.terasology.engine.context.Context;
import org.terasology.engine.core.ComponentSystemManager;
import org.terasology.engine.core.TerasologyConstants;
Expand Down Expand Up @@ -54,7 +55,7 @@ public boolean step() {
BlockManager blockManager = context.get(BlockManager.class);
ExtraBlockDataManager extraDataManager = context.get(ExtraBlockDataManager.class);

RemoteChunkProvider chunkProvider = new RemoteChunkProvider(blockManager, localPlayer);
RemoteChunkProvider chunkProvider = new RemoteChunkProvider(blockManager, localPlayer, context.get(Config.class));

WorldProviderCoreImpl worldProviderCore = new WorldProviderCoreImpl(gameManifest.getWorldInfo(TerasologyConstants.MAIN_WORLD), chunkProvider,
blockManager.getBlock(BlockManager.UNLOADED_ID), context);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.google.common.collect.Maps;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.terasology.engine.config.Config;
import org.terasology.engine.config.SystemConfig;
import org.terasology.engine.context.Context;
import org.terasology.engine.core.ComponentSystemManager;
Expand Down Expand Up @@ -136,6 +137,7 @@ public boolean step() {
worldGenerator,
blockManager,
extraDataManager,
context.get(Config.class),
Maps.newConcurrentMap());
RelevanceSystem relevanceSystem = new RelevanceSystem(chunkProvider);
context.put(RelevanceSystem.class, relevanceSystem);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,33 @@ public void initialise() {
viewDistance.bindSelection(BindHelper.bindBeanProperty("viewDistance", config.getRendering(), ViewDistance.class));
}

UISlider chunkThreads = find("chunkThreads", UISlider.class);
if (chunkThreads != null) {
chunkThreads.setIncrement(1.0f);
chunkThreads.setPrecision(0);
chunkThreads.setMinimum(0);
chunkThreads.setRange(Runtime.getRuntime().availableProcessors());
chunkThreads.setLabelFunction(input -> {
if (input == 0) {
return "Auto";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this string be translated? I am not sure about how dynamic values are handled at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

no, that seems very international.

} else {
return String.valueOf(input.intValue());
}
});
chunkThreads.bindValue(new Binding<Float>() {
@Override
public Float get() {
return (float) config.getRendering().getChunkThreads();
}

@Override
public void set(Float value) {
int chunkThreads = value.intValue();
config.getRendering().setChunkThreads(chunkThreads);
}
});
}

UIDropdown<WaterReflection> waterReflection = find("reflections", UIDropdown.class);
if (waterReflection != null) {
waterReflection.setOptionRenderer(new ToStringTextRenderer<>(translationSystem));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.joml.Vector3ic;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.terasology.engine.config.Config;
import org.terasology.engine.entitySystem.entity.EntityManager;
import org.terasology.engine.entitySystem.entity.EntityRef;
import org.terasology.engine.entitySystem.entity.EntityStore;
Expand Down Expand Up @@ -94,6 +95,7 @@
private final WorldGenerator generator;
private final BlockManager blockManager;
private final ExtraBlockDataManager extraDataManager;
private final Config config;
private ChunkProcessingPipeline loadingPipeline;
private TaskMaster<ChunkUnloadRequest> unloadRequestTaskMaster;
private EntityRef worldEntity = EntityRef.NULL;
Expand All @@ -102,13 +104,14 @@
private RelevanceSystem relevanceSystem;

public LocalChunkProvider(StorageManager storageManager, EntityManager entityManager, WorldGenerator generator,
BlockManager blockManager, ExtraBlockDataManager extraDataManager,
BlockManager blockManager, ExtraBlockDataManager extraDataManager, Config config,
Map<Vector3ic, Chunk> chunkCache) {
this.storageManager = storageManager;
this.entityManager = entityManager;
this.generator = generator;
this.blockManager = blockManager;
this.extraDataManager = extraDataManager;
this.config = config;
this.unloadRequestTaskMaster = TaskMaster.createFIFOTaskMaster("Chunk-Unloader", 4);
this.chunkCache = chunkCache;
ChunkMonitor.fireChunkProviderInitialized(this);
Expand Down Expand Up @@ -406,7 +409,8 @@
storageManager.deleteWorld();
worldEntity.send(new PurgeWorldEvent());

loadingPipeline = new ChunkProcessingPipeline(this::getChunk, relevanceSystem.createChunkTaskComporator());
loadingPipeline = new ChunkProcessingPipeline(config.getRendering().getChunkThreads(), this::getChunk,
relevanceSystem.createChunkTaskComporator());
loadingPipeline.addStage(
ChunkTaskProvider.create("Chunk generate internal lightning",
(Consumer<Chunk>) InternalLightProcessor::generateInternalLighting))
Expand Down Expand Up @@ -438,10 +442,11 @@
return chunk != null && chunk.isReady();
}

// TODO: move loadingPipeline initialization into constructor.

Check warning on line 445 in engine/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProvider.java

View check run for this annotation

Terasology Jenkins.io / Open Tasks Scanner

TODO

NORMAL: move loadingPipeline initialization into constructor.
public void setRelevanceSystem(RelevanceSystem relevanceSystem) {
this.relevanceSystem = relevanceSystem;
loadingPipeline = new ChunkProcessingPipeline(this::getChunk, relevanceSystem.createChunkTaskComporator());
loadingPipeline = new ChunkProcessingPipeline(config.getRendering().getChunkThreads(), this::getChunk,
relevanceSystem.createChunkTaskComporator());
loadingPipeline.addStage(
ChunkTaskProvider.create("Chunk generate internal lightning",
(Consumer<Chunk>) InternalLightProcessor::generateInternalLighting))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@
public class ChunkProcessingPipeline {

@SuppressWarnings("UnstableApiUsage")
private static final int NUM_TASK_THREADS = constrainToRange(
Runtime.getRuntime().availableProcessors() - 1, 1, 8);
private static final int DEFAULT_TASK_THREADS = constrainToRange(
Runtime.getRuntime().availableProcessors() - 2, 1, 4);
private static final Logger logger = LoggerFactory.getLogger(ChunkProcessingPipeline.class);

private final List<ChunkTaskProvider> stages = Lists.newArrayList();
Expand All @@ -54,17 +54,18 @@ public class ChunkProcessingPipeline {
/**
* Create ChunkProcessingPipeline.
*/
public ChunkProcessingPipeline(Function<Vector3ic, Chunk> chunkProvider, Comparator<Future<Chunk>> comparable) {
public ChunkProcessingPipeline(int chunkThreads, Function<Vector3ic, Chunk> chunkProvider, Comparator<Future<Chunk>> comparable) {
this.chunkProvider = chunkProvider;

int taskThreads = (chunkThreads == 0) ? DEFAULT_TASK_THREADS : chunkThreads;
executor = new ThreadPoolExecutor(
NUM_TASK_THREADS,
NUM_TASK_THREADS, 0L,
taskThreads,
taskThreads, 0L,
TimeUnit.MILLISECONDS,
new PriorityBlockingQueue(800, comparable),
this::threadFactory,
this::rejectQueueHandler);
logger.debug("allocated {} threads", NUM_TASK_THREADS);
logger.debug("allocated {} threads", taskThreads);
chunkProcessor = new ChunkExecutorCompletionService(executor,
new PriorityBlockingQueue<>(800, comparable));
reactor = new Thread(this::chunkTaskHandler);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.joml.Vector3f;
import org.joml.Vector3i;
import org.joml.Vector3ic;
import org.terasology.engine.config.Config;
import org.terasology.engine.entitySystem.entity.EntityRef;
import org.terasology.engine.logic.players.LocalPlayer;
import org.terasology.engine.monitoring.chunk.ChunkMonitor;
Expand Down Expand Up @@ -56,9 +57,9 @@ public class RemoteChunkProvider implements ChunkProvider {
private EntityRef worldEntity = EntityRef.NULL;
private ChunkReadyListener listener;

public RemoteChunkProvider(BlockManager blockManager, LocalPlayer localPlayer) {
public RemoteChunkProvider(BlockManager blockManager, LocalPlayer localPlayer, Config config) {
this.blockManager = blockManager;
loadingPipeline = new ChunkProcessingPipeline(this::getChunk,
loadingPipeline = new ChunkProcessingPipeline(config.getRendering().getChunkThreads(), this::getChunk,
new LocalPlayerRelativeChunkComparator(localPlayer));

loadingPipeline.addStage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@
"view-distance-moderate": "view-distance-moderate",
"view-distance-near": "view-distance-near",
"view-distance-ultra": "view-distance-ultra",
"chunk-threads": "chunk-threads",
"vignette": "vignette",
"volumetric-fog": "volumetric-fog",
"warn-modules": "warn-modules",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@
"view-distance-moderate": "Moderate",
"view-distance-near": "Near",
"view-distance-ultra": "Ultra",
"chunk-threads": "Chunk Threads",
"vignette": "Vignette",
"volumetric-fog": "Volumetric Fog",
"warn-modules": "WARNING: Enabling extra modules, especially all or those not in the stable lineup can cause game crashes or broken worlds.\nPlease handle with care.",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,14 @@
"type": "UIDropdown",
"id": "viewDistance"
},
{
"type": "UILabel",
"text": "${engine:menu#chunk-threads}: "
},
{
"type": "UISlider",
"id": "chunkThreads"
},
{
"type": "UISpace",
"size": [1, 13]
Expand Down
Loading