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

Still another array access performance issue (in debug) #15685

Open
IntegratedQuantum opened this issue May 13, 2023 · 3 comments
Open

Still another array access performance issue (in debug) #15685

IntegratedQuantum opened this issue May 13, 2023 · 3 comments
Labels
bug Observed behavior contradicts documented or intended behavior optimization regression It worked in a previous version of Zig, but stopped working.
Milestone

Comments

@IntegratedQuantum
Copy link
Contributor

IntegratedQuantum commented May 13, 2023

Zig Version

0.11.0-dev.3105+e46d7a369

Steps to Reproduce and Observed Behavior

This is a follow-up to #13938 which was only fixed in release mode(which means I still have to rely on the workaround to be able to run my code in debug mode). The reproduction code is basically identical:

const std = @import("std");

const len = 32*32*32;

fn getIndex(i: u16) u16 {
	return i;
}

pub const Chunk = struct {
	blocks: [len]u16 = undefined,
};

pub noinline fn regenerateMainMesh(chunk: *Chunk) u32 {
	var sum: u32 = 0;
	var i: u16 = 0;
	while(i < len) : (i += 1) {
		sum += chunk.blocks[getIndex(i)]; // ← workaround: (&chunk.blocks)[...]
	}
	return sum;
}

pub fn main() void {
	var chunk: Chunk = Chunk{};
	for(&chunk.blocks, 0..) |*block, i| {
		block.* = @intCast(i);
	}
	const start = std.time.nanoTimestamp();
	const sum = regenerateMainMesh(&chunk);
	const end = std.time.nanoTimestamp();
	std.log.err("Time: {} Sum: {}", .{end - start, sum});
}

Running this in debug mode is taking 93 ms for iterating over a 32768 element array:

$ zig run test.zig
error: Time: 93325090 Sum: 536854528

Running this in a profiler, I can see that the bottleneck still is a memcpy that is called once every iteration and appears to be copying the entire array:
Screenshot at 2023-05-13 10-14-00
Screenshot at 2023-05-13 10-08-58

Expected Behavior

When applying the workaround

- sum += chunk.blocks[getIndex(i)]; // ← workaround: (&chunk.blocks)[...]
+ sum += (&chunk.blocks)[getIndex(i)]; // ← workaround: (&chunk.blocks)[...]

the performance is significantly better, taking only 0.4 ms instead of 93 ms:

$ zig run test.zig
error: Time: 357398 Sum: 536854528
@IntegratedQuantum IntegratedQuantum added the bug Observed behavior contradicts documented or intended behavior label May 13, 2023
@Vexu Vexu added this to the 0.12.0 milestone May 13, 2023
@IntegratedQuantum
Copy link
Contributor Author

IntegratedQuantum commented Jun 6, 2023

I just updated my zig version and the problem got a lot worse.

EDIT: I updated again and it is now back to how it was originally.

Expand

This simplified example was not affected in the past:

const std = @import("std");

const len = 32*32*32;

var blocks: [len]u16 = undefined;

pub noinline fn regenerateMainMesh() u32 {
	var sum: u32 = 0;
	var i: u16 = 0;
	while(i < len) : (i += 1) {
		sum += blocks[i]; // ← workaround: (&blocks)[...]
	}
	return sum;
}

pub fn main() void {
	for(&blocks, 0..) |*block, i| {
		block.* = @intCast(u16, i);
	}
	const start = std.time.nanoTimestamp();
	const sum = regenerateMainMesh();
	const end = std.time.nanoTimestamp();
	std.log.err("Time: {} Sum: {}", .{end - start, sum});
}

But now it is also slow:

$ zig version
0.11.0-dev.3380+7e0a02ee2
$ zig run test.zig
error: Time: 98298061 Sum: 536854528

$ ~/Downloads/zig-old/zig version
0.11.0-dev.3132+465272921
$ ~/Downloads/zig-old/zig run test.zig
error: Time: 632556 Sum: 536854528

@andrewrk
Copy link
Member

Fix landed in 2ab7893.

andrewrk added a commit that referenced this issue Jan 24, 2024
…ers"

This reverts commit 2ab7893.

Premature merge - apologies for the disruption.

Reopens #15685
Reopens #17580
@andrewrk
Copy link
Member

Fix reverted in 9d5a133.

@andrewrk andrewrk reopened this Jan 24, 2024
silver-signal pushed a commit to silver-signal/zig that referenced this issue Jan 25, 2024
…ers"

This reverts commit 2ab7893.

Premature merge - apologies for the disruption.

Reopens ziglang#15685
Reopens ziglang#17580
bilaliscarioth pushed a commit to bilaliscarioth/zig that referenced this issue Jan 27, 2024
…ers"

This reverts commit 2ab7893.

Premature merge - apologies for the disruption.

Reopens ziglang#15685
Reopens ziglang#17580
Vexu added a commit to Vexu/zig that referenced this issue Mar 18, 2024
Vexu added a commit to Vexu/zig that referenced this issue Mar 19, 2024
@andrewrk andrewrk modified the milestones: 0.12.0, 0.13.0 Mar 20, 2024
Vexu added a commit to Vexu/zig that referenced this issue Mar 22, 2024
@mlugg mlugg moved this to Optimize Machine Code in Performance Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior optimization regression It worked in a previous version of Zig, but stopped working.
Projects
Status: Optimize Machine Code
3 participants