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

Jump to bottom

Show task performance #445

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Show task performance #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5acf1f5
threadpool: track and show task times
archbirdplus Jun 3, 2024
ad1aa46
debug window: subdivide task performance
archbirdplus Jun 4, 2024
af2962b
indent debug window code
archbirdplus Jun 4, 2024
ae11566
task perfs: remove meshgen for lighting
archbirdplus Jun 5, 2024
ce26e55
task perfs: don't divide by zero
archbirdplus Jun 5, 2024
93a9d04
task perf: move taskType to vtable
archbirdplus Jun 5, 2024
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
1 change: 1 addition & 0 deletions src/audio.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ const MusicLoadTask = struct {
.isStillNeeded = @ptrCast(&isStillNeeded),
.run = @ptrCast(&run),
.clean = @ptrCast(&clean),
.taskType = .misc,
};

pub fn schedule(musicId: []const u8) void {
Expand Down
16 changes: 15 additions & 1 deletion src/gui/windows/debug.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const graphics = main.graphics;
const draw = graphics.draw;
const Texture = graphics.Texture;
const Vec2f = main.vec.Vec2f;
const TaskType = main.utils.ThreadPool.TaskType;

const gui = @import("../gui.zig");
const GuiWindow = gui.GuiWindow;
Expand Down Expand Up @@ -38,6 +39,19 @@ pub fn render() void {
y += 8;
draw.print("Queue size: {}", .{main.threadPool.queueSize()}, 0, y, 8, .left);
y += 8;
const perf = main.threadPool.getPerformance();
const values = comptime std.enums.values(TaskType);
inline for(values) |t| {
const name = switch (t) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use @tagName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To separate source code and rendering options. Suppose we added localization?

Copy link
Member

Choose a reason for hiding this comment

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

Debug info does not need to be translated, since it shouldn't be used by the majority of players.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that will be a secondary concern after we get any localization in place.

.chunkgen => "chunkgen",
.lighting => "lighting",
.misc => "other",
else => continue,
};
const i = @intFromEnum(t);
draw.print(" " ++ name ++ " time: {} ms ({} µs/task)", .{@divFloor(perf.utime[i], 1000), @divFloor(perf.utime[i], @max(1, perf.tasks[i]))}, 0, y, 8, .left);
y += 8;
}
draw.print("Mesh Queue size: {}", .{main.renderer.mesh_storage.updatableList.items.len}, 0, y, 8, .left);
y += 8;
{
Expand All @@ -57,4 +71,4 @@ pub fn render() void {
draw.print("Opaque faces: {}, Transparent faces: {}", .{main.renderer.chunk_meshing.quadsDrawn, main.renderer.chunk_meshing.transparentQuadsDrawn}, 0, y, 8, .left);
y += 8;
}
}
}
1 change: 1 addition & 0 deletions src/network.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1739,6 +1739,7 @@ const ProtocolTask = struct {
.isStillNeeded = @ptrCast(&isStillNeeded),
.run = @ptrCast(&run),
.clean = @ptrCast(&clean),
.taskType = .misc,
};

pub fn schedule(conn: *Connection, protocol: u8, data: []const u8) void {
Expand Down
3 changes: 2 additions & 1 deletion src/renderer/chunk_meshing.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@ pub const ChunkMesh = struct {
.isStillNeeded = @ptrCast(&isStillNeeded),
.run = @ptrCast(&run),
.clean = @ptrCast(&clean),
.taskType = .lighting,
Copy link
Member

Choose a reason for hiding this comment

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

Also I just noticed you have three .lighting tasks, but they do completely different things.
Having multiple tasks with the same tag can hide performance problems. For example, this LightRefreshTask is relatively fast, which can skew the average mesh generation time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wouldn't be hard to change. Is there a better classification? I literally just classified it based on my impressions of the performance problems.

Copy link
Member

Choose a reason for hiding this comment

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

I would just put the LightRefreshTaks and the LightMapLoadTask under .misc, they shouldn't take much performance.

};

pub fn scheduleAndDecreaseRefCount(mesh: *ChunkMesh) void {
Expand Down Expand Up @@ -1237,4 +1238,4 @@ pub const ChunkMesh = struct {
chunkList.append(self.chunkAllocation.start);
transparentQuadsDrawn += self.culledSortingCount;
}
};
};
3 changes: 2 additions & 1 deletion src/renderer/mesh_storage.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,6 +1042,7 @@ pub const MeshGenerationTask = struct {
.isStillNeeded = @ptrCast(&isStillNeeded),
.run = @ptrCast(&run),
.clean = @ptrCast(&clean),
.taskType = .lighting,
};

pub fn schedule(mesh: *chunk.Chunk) void {
Expand Down Expand Up @@ -1092,4 +1093,4 @@ pub fn updateLightMap(map: *LightMap.LightMapFragment) void {
mutex.lock();
defer mutex.unlock();
mapUpdatableList.append(map);
}
}
3 changes: 3 additions & 0 deletions src/server/world.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const ChunkManager = struct {
.isStillNeeded = @ptrCast(&isStillNeeded),
.run = @ptrCast(&run),
.clean = @ptrCast(&clean),
.taskType = .chunkgen,
};

pub fn scheduleAndDecreaseRefCount(pos: ChunkPosition, source: ?*User) void {
Expand Down Expand Up @@ -105,6 +106,7 @@ const ChunkManager = struct {
.isStillNeeded = @ptrCast(&isStillNeeded),
.run = @ptrCast(&run),
.clean = @ptrCast(&clean),
.taskType = .lighting,
};

pub fn scheduleAndDecreaseRefCount(pos: terrain.SurfaceMap.MapFragmentPosition, source: ?*User) void {
Expand Down Expand Up @@ -424,6 +426,7 @@ pub const ServerWorld = struct {
.isStillNeeded = @ptrCast(&isStillNeeded),
.run = @ptrCast(&run),
.clean = @ptrCast(&clean),
.taskType = .chunkgen,
};

pub fn schedule(pos: ChunkPosition) void {
Expand Down
42 changes: 42 additions & 0 deletions src/utils.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,12 @@ pub fn BlockingMaxHeap(comptime T: type) type {
}

pub const ThreadPool = struct {
pub const TaskType = enum(usize) {
chunkgen,
lighting,
misc,
taskTypes,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having a C-style enum length field, I think it would be better to use std.enums.directEnumArrayLen to determine the length instead.
This would also make it possible to use @tagName above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes I wonder if Zig devs have a naming scheme.

};
const Task = struct {
cachedPriority: f32,
self: *anyopaque,
Expand All @@ -999,6 +1005,23 @@ pub const ThreadPool = struct {
isStillNeeded: *const fn(*anyopaque, milliTime: i64) bool,
run: *const fn(*anyopaque) void,
clean: *const fn(*anyopaque) void,
taskType: TaskType = .misc,
};
pub const Performance = struct {
mutex: std.Thread.Mutex = .{},
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be a great place to use atomics with monotonic ordering instead of locking.

tasks: [@intFromEnum(TaskType.taskTypes)]u32 = {},
utime: [@intFromEnum(TaskType.taskTypes)]i64 = {},

fn clear(self: *Performance) void {
for(0..@intFromEnum(TaskType.taskTypes)) |i| {
self.tasks[i] = 0;
self.utime[i] = 0;
}
}

fn init(self: Performance) void {
self.clear();
}
};
const refreshTime: u32 = 100; // The time after which all priorities get refreshed in milliseconds.

Expand All @@ -1007,11 +1030,14 @@ pub const ThreadPool = struct {
loadList: *BlockingMaxHeap(Task),
allocator: NeverFailingAllocator,

performance: *Performance,

pub fn init(allocator: NeverFailingAllocator, threadCount: usize) ThreadPool {
const self = ThreadPool {
.threads = allocator.alloc(std.Thread, threadCount),
.currentTasks = allocator.alloc(Atomic(?*const VTable), threadCount),
.loadList = BlockingMaxHeap(Task).init(allocator),
.performance = allocator.create(Performance),
.allocator = allocator,
};
for(self.threads, 0..) |*thread, i| {
Expand Down Expand Up @@ -1062,6 +1088,15 @@ pub const ThreadPool = struct {
}
}

pub fn getPerformance(self: ThreadPool) Performance {
self.performance.mutex.lock();
defer {
self.performance.clear();
self.performance.mutex.unlock();
}
return self.performance.*;
}

fn run(self: ThreadPool, id: usize) void {
// In case any of the tasks wants to allocate memory:
var sta = StackAllocator.init(main.globalAllocator, 1 << 23);
Expand All @@ -1073,7 +1108,14 @@ pub const ThreadPool = struct {
{
const task = self.loadList.extractMax() catch break;
self.currentTasks[id].store(task.vtable, .monotonic);
const start = std.time.microTimestamp();
task.vtable.run(task.self);
const end = std.time.microTimestamp();
const taskType = @intFromEnum(task.vtable.taskType);
self.performance.mutex.lock();
self.performance.tasks[taskType] += 1;
self.performance.utime[taskType] += end - start;
self.performance.mutex.unlock();
self.currentTasks[id].store(null, .monotonic);
}

Expand Down
Loading