-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add memory allocation tracking #69
Add memory allocation tracking #69
Conversation
d4b8c21
to
b3b2b91
Compare
Man really cool stuff that you recently provided to zbench 🙌 |
Cheers, and thank you for the valuable reviews, hopefully the code is solid ;) Yeah, trying out the hardware performance counters would be interesting. I guessed that using an allocator wrapper would be very lightweight and trivially cross platform; poop is linux only currently though there are probably methods for other OSes too. FWIW, from benchmarking on my laptop with and without allocation tracking I can't detect any measurable overhead. |
util/runner.zig
Outdated
.state = .{ .running = .{ | ||
.iterations_count = iterations, | ||
.iterations_remaining = iterations, | ||
.timings_ns = try allocator.alloc(u64, iterations), | ||
.allocation_maxes = if (track_allocations) try allocator.alloc(usize, iterations) else null, |
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.
nit: What do you think of encapsulating the tracking logic within a dedicated struct?
It could encapsulate the tracking logic in a single place, making it easier to manage and modify.
Something like this?
const AllocationTracker = struct {
maxes: []usize,
counts: []usize,
pub fn init(allocator: *std.mem.Allocator, iterations: usize, track: bool) !?AllocationTracker {
if (!track) return null;
return AllocationTracker{
.maxes = try allocator.alloc(usize, iterations),
.counts = try allocator.alloc(usize, iterations),
};
}
pub fn update(self: *AllocationTracker, index: usize, max: ?usize, count: ?usize) void {
if (max) |m| self.maxes[index] = m;
if (count) |c| self.counts[index] = c;
}
pub fn deinit(self: *AllocationTracker, allocator: *std.mem.Allocator) void {
allocator.free(self.maxes);
allocator.free(self.counts);
}
};
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.
That's a good idea I think, then the maxes and counts are always null or not null in synch.
zbench.zig
Outdated
@@ -68,13 +73,27 @@ const Definition = struct { | |||
if (self.config.hooks.before_each) |hook| hook(); | |||
defer if (self.config.hooks.after_each) |hook| hook(); | |||
|
|||
var tracking = TrackingAllocator.init(allocator); |
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 one i haven't really verified, but maybe avoid creating initialization of TrackingAllocator
when self.config.track_allocations
is false.
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.
That's what I tried to do initially, but I was struggling to factor out choosing which allocator while not duplicating the rest of the function in an if
. If the TrackingAllocator
is defined on the stack inside a sub block it'll be freed outside of that sub block, so I couldn't do something like const alloc = if (self.config.track_allocations) TrackingAllocator.init(allocator).allocator() else allocator;
. I can try other ways but always creating the tracking allocator was a simple method.
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.
Ah okay, understood. No need to change it, but a small comment would be helpful.
from my side it looks good, should we merge it @bens? |
There are still a few things to take care of. |
Adds a custom Allocator wrapper which keeps track of the allocated memory, then record the maximum allocation readings in the Runner alongside timing readings and carry them into the Result.
Ok, I'm happy with it now. Thanks @hendriknielaender :) |
Adds a custom
Allocator
wrapper which keeps track of the allocated memory, then record the maximum allocation readings in theRunner
alongside timing readings and carry them into theResult
. I've added equivalent statistics for memory use as for timings, but maybe we only need something simpler?Current output: