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

Allocator interface: make shrinkFn optional #2292

Closed
andrewrk opened this issue Apr 16, 2019 · 2 comments
Closed

Allocator interface: make shrinkFn optional #2292

andrewrk opened this issue Apr 16, 2019 · 2 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@andrewrk
Copy link
Member

I propose that shrinkFn be an optional pointer. An implementation of the Allocator interface would set this to null when it does not provide a way to reclaim memory. The Allocator interface wrapper code would provide a shim implementation of shrink which simply did return old_mem[0..new_size];.

The benefit of this proposal is that the Allocator interface could have a new function called, perhaps, canReclaimMemory() which is simply self.shrinkFn != null. If canReclaimMemory returns false then the user of the allocator knows they don't have to bother with free().

This would be used, for example, here:

zig/std/zig/parse.zig

Lines 12 to 13 in bddbbef

pub fn parse(allocator: *mem.Allocator, source: []const u8) !ast.Tree {
var tree_arena = std.heap.ArenaAllocator.init(allocator);

This code currently unconditionally creates an arena allocator (which does not reclaim memory with free()) on top of the provided allocator. This is a waste of memory and computations if the provided allocator already does not reclaim memory. So it would be adjusted to only create the arena allocator if the provided allocator reported true for canReclaimMemory.

Finally, this would also simplify some allocator implementations since they could just do .shrinkFn = null rather than provide a useless shrink function.

@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Apr 16, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone Apr 16, 2019
@andrewrk andrewrk added the accepted This proposal is planned. label Apr 30, 2019
andrewrk added a commit that referenced this issue Apr 30, 2019
This is work-in-progress because it's blocked by coroutines depending on
the Allocator interface, which will be solved with the coroutine rewrite
(#2377).

closes #2292
@andrewrk
Copy link
Member Author

I've got this solved in the null-shrink-fn branch, but it's blocking on #2377.

@andrewrk
Copy link
Member Author

andrewrk commented May 5, 2020

Taking this in the direction of #4431

@andrewrk andrewrk closed this as completed May 5, 2020
@andrewrk andrewrk removed the accepted This proposal is planned. label May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant