Yes, of course. I must have missed that the saved values have a program lifetime.
1 Like
Does not follow. You’re assuming all allocations use the same allocator.
1 Like
One thing that might be worth thinking about is that the DebugAllocator leak detection is just one way to enforce some desired properties of your allocations. If you have different things you want enforced/detected, you can likely find a way to do that, too.
For example, to detect these types of “leaks” of an arena, you could create an allocator implementation that wraps an arena and fails/panics/whatever when the total arena size goes over some threshold (and then only use that wrapper when in debug mode, during tests, or when some build flag is set). May not lead to perfect detection in cases where the expected arena size is extremely variable, but it would still probably be able to catch egregious leaks (e.g. if you forget to reset in a loop and the loop iterates 1000 times).
EDIT: This could actually be done without even needing a custom Allocator by giving your arena a FixedBufferAllocator as its backing allocator. Also, you can probably come up with something more clever/tailored to your specific use case, this was just what came to mind for me.
A while back I wrote a bit about the general idea here: Improving Fuzz Testing with Zig Allocators - ryanliptak.com
2 Likes
What do you mean with this?
std.heap.ArenaAllocator’s deinit method frees whatever memory is still held by the arena, thus it doesn’t matter whether you ever called reset or not.
Just make sure you call deinit and leaking will be impossible; if there is nothing to do, because a previous reset cleared the memory, it will basically be a noop.
Quite right, I should have specified: leaks in an arena allocator that lives for the lifetime of the program that are a result of not calling reset() when you could/should have can never be caught by calling deinit() since at that point they aren’t leaked.
In otherwords, calling deinit() on program lifetime arenas is code that only does anything when you want to ensure that other temporary arena allocators aren’t failing to call deinit() themselves. if you have no other temporary allocators (i.e. you claim that page space for the duration of the program, and only clear it (reset(.retain_capacity)) instead of init/deinit) then this type of leak detection doesn’t really work.
I’m mostly pushing back on moralistic good hygiene argument that doesn’t reflect the realities of the system. I’ve already learned a lot, like std.process.cleanExit() and fuzz testing, etc.
Say you have a program general scratch space. You init at startup, and immediately defer .deinit(). Technically, no leaks possible, ever. Obviously this isn’t useful if you actually leak anyway. here is an example:
fn parseBlob(arena: std.mem.Allocator, blob: []u8) ParsedBlob {
const result: *ParsedBlob = .{}; // parsed
return result;
}
fn bigComputation(
arena: std.mem.Allocator,
scratch: std.mem.Allocator,
blobs: [][]u8,
) std.ArrayList(ParsedBlob) {
var result: std.ArrayList(ParsedBlob) = .initCapacity(arena, blobs.len);
for (blobs) |blob| {
const parsedBlob = parseBlob(scratch, blob);
result.appendAssumeCapacity(blob);
}
return result;
}
pub fn main() !void {
const program_arena: std.heap.ArenaAllocator = .init(std.heap.page_allocator);
const scratch_arena: std.heap.ArenaAllocator = .init(std.heap.page_allocator);
const program = program_arena.allocator();
const scratch = scratch_arena.allocator();
// imagine blobs are defined somehow idk
const blob_list = bigComputation(program, scratch, blobs);
// imagine more functions that use scratch here...
// we have `leaked` the scratch arena space, it's not used anymore
// but no leaks will be reported
std.process.cleanExit();
program_arena.deinit();
scratch_arena.deinit();
}
I think everybody in this thread is in violent agreement with you about this. The point of defer deinit as good hygiene, which you seem to think is “moralistic”, is the ability for your testing, be it unit or integration, to catch bugs like this by, e.g., using a different allocation strategy.
1 Like
OK I think it’s coming into focus more. Say I have a suspected leak in my scratch space (as per my previous example). If I switch the scratch arena to the debug allocator, that will find where reset wasn’t called, allowing me to fix the leak? I wasn’t aware that worked like that, I haven’t had occasion to use the debug allocator yet for leak detection with arenas. That’s a nice tool to have available.
And to be clear, I never thought anyone was in disagreement with anything, since I’m not trying to prove anything anyway, just getting clarification on the why of things. I’m sorry if I come across that way, it’s hard to communicate tone online. (I agree the moralism quip was unnecessary, it’s just a pet-peeve of mine in programming dogma. But when people drop it as a one-line explanation, it’s hard to swallow)
3 Likes
Basically every time you use an arena you obscure whether the individual lifetime handling of objects in that arena works correctly, but that is fine, because the purpose of the arena is to group life times together and leak until the group as a whole can be freed.
Then if you are interested in testing whether that program handles leaks you can switch the arena to non-arena allocators with some direct or parent allocator as debug-allocator to check for leaks.
Another way to check for leak handling is to write tests that use std.testing.checkAllAllocationFailures that way your test code can systematically check whether all possible allocation failures are handled.
That said you could still have a logic bug somewhere, that results in a leak, I think for that you could use fuzzing.
3 Likes
Switching to a debug allocator from arena will show leaks for code that intentionally omits free/deinit assuming the frees aren’t needed with an arena. That makes it a pointless exercise for that case because the omissions are intentional, and the contract for that code is that it must be given an arena allocator.
For code the frees/deinits as if it uses a general purpose allocator (the frees are no-ops on an arena hence nearly no cost), it works properly to detect leaks. This is a good strategy in library code: it allows caller maximum flexibility in allocator strategy for clients of the library.
It can be useful to use a debug allocator as backing allocator to an arena - these will detect failures to deinit the arena.
1 Like
I’ve found this to be a good tool for checking memory safety. Squeek also has an article here.
You are right, this will not be detected.
You cannot change for a debug allocator directly, as the debug allocator does not have a reset function. You only can use it as backing allocator.
But also then a “missing”reset(.retain_capacity) cannot be detected. I would say, there is no general rule for detecting those ‘temporary leaks’. As others said, that is basically the purpose of an arena.
Thinking about it, I thought, while forgetting to call deinit will cause a memory leak, not calling reset(.retain_capacity) probably is more like a missed optimization.
Also a question: Do others use reset(.retain_capacity) in long running loops?
I think the (valid) concern is with the kind of “leaks” which can be characterized as merely “waiting too long” to reset() (or even deinit(), actually, though idiomatic practice would make this harder). By “waiting too long” I mean that you have a “growing” amount of actually-unused data in memory, which you could have reset()/deinit()ed by now, but you haven’t, and perhaps your code logic is so bad that you keep looping and heaping onto the heap … before you know it, your end-user is checking top and wondering why you’re such a memory hog. Sure, perhaps in a minute, you’d reset(), and sure, at the end of the program, all will be well - it’ll all be freed, but… in the meantime? This seems to be a case-by-case sleuthing task that is simply “programming”, and tools like swap-in (or backing) allocators, fuzzers, etc., can be helpful in tuning, but an important ingredient is to take protective measures, programming according to best practices, minimizing the amount of now-useless data that might be lying around, not yet deallocated, by design. (Hope I’m not misreading.)
Often a collection (ArrayList) is a better “arena” than a true general purpose arena, when the objects are all the same type. Neglecting to clear such a collection when its contents are no longer needed is the same bug. I don’t see this as an allocator related issue.
2 Likes