From 5f0d7b34d7c676996ed9b64ba557f33545e13933 Mon Sep 17 00:00:00 2001 From: torque Date: Thu, 20 Jul 2023 23:15:37 -0700 Subject: [PATCH] parser: shove an arena allocator in there Stay a while and listen to my story. Due to the design of the parser execution flow, the only reasonable way to avoid leaking memory in the parser is to use an arena allocator because the parser itself doesn't have direct access to everything it allocates, and everything it allocates needs to live for the duration of whatever callbacks are called. Now, you might say, if the items it allocates are stored for the lifetime of whatever callbacks, then that means that the items it allocates stay allocated for effectively the entire life of the program. In which case there's really not much point in freeing them at all, as it's just extra work on exit that the OS should normally clean up. And you'd be right, except for two details: if the user uses the current GeneralPurposeAllocator, it will complain about leaks when deinitialized, which simply isn't groovy. The other detail is that technically the user can run any code they want after the parser execution finishes, so forcing the user to leak memory by having an incomplete API is rude. The other option would be, as before, forcing the user to supply their own arena allocator if they don't want to leak, but that's kind of a rude thing to do and goes against the "all allocators look the same" design of the standard library, which is what makes it so easy to use and create allocators with advanced functionality. That seems like an ugly thing to do, so, instead, each parser gets to eat the memory cost of storing a pointer to its arena allocator (and the heap cost of the arena allocator itself). In theory, subcommands could borrow the arena allocator of their parent command to save a bit of heap space, but that would make a variety of creation and cleanup-related tasks less isomorphic between the parents and the subcommands. I like the current design where commands and subcommands are the same thing, and I'm not in a rush to disturb that. I don't think the overhead cost of the arena allocator itself, which can be measured in double digit bytes, is a particularly steep price to pay. --- demo/demo.zig | 24 +++++++------ source/command.zig | 14 +++++--- source/parser.zig | 84 ++++++++++++++++++++++++++++------------------ 3 files changed, 75 insertions(+), 47 deletions(-) diff --git a/demo/demo.zig b/demo/demo.zig index 52b746a..8663b35 100644 --- a/demo/demo.zig +++ b/demo/demo.zig @@ -62,10 +62,6 @@ const cli = cmd: { .env_var = "NOCLIP_ENVIRON", .description = "environment variable only option", }); - cmd.add_argument(.{ .OutputType = []const u8 }, .{ - .name = "arg", - .description = "This is an argument that doesn't really do anything, but it's very important.", - }); break :cmd cmd; }; @@ -73,9 +69,9 @@ const cli = cmd: { const subcommand = cmd: { var cmd = CommandBuilder([]const u8){ .description = - \\Perform some sort of work + \\Demonstrate subcommand functionality \\ - \\This subcommand is a mystery. It probably does something, but nobody is sure what. + \\This command demonstrates how subcommands work. , }; cmd.simple_flag(.{ @@ -85,6 +81,10 @@ const subcommand = cmd: { .env_var = "NOCLIP_SUBFLAG", }); cmd.add_argument(.{ .OutputType = []const u8 }, .{ .name = "argument" }); + cmd.add_argument(.{ .OutputType = []const u8 }, .{ + .name = "arg", + .description = "This is an argument that doesn't really do anything, but it's very important.", + }); break :cmd cmd; }; @@ -101,15 +101,17 @@ fn cli_handler(context: *u32, result: cli.Output()) !void { } pub fn main() !u8 { - var arena = std.heap.ArenaAllocator.init(std.heap.page_allocator); - defer arena.deinit(); - const allocator = arena.allocator(); + var gpa = std.heap.GeneralPurposeAllocator(.{}){}; + defer _ = gpa.deinit(); + const allocator = gpa.allocator(); + + var parser = try cli.create_parser(cli_handler, allocator); + defer parser.deinitTree(); - var parser = cli.create_parser(cli_handler, allocator); var context: u32 = 2; const sc: []const u8 = "whassup"; - var subcon = subcommand.create_parser(sub_handler, allocator); + var subcon = try subcommand.create_parser(sub_handler, allocator); try parser.add_subcommand("verb", subcon.interface(&sc)); const iface = parser.interface(&context); diff --git a/source/command.zig b/source/command.zig index 39845a0..2e6ff61 100644 --- a/source/command.zig +++ b/source/command.zig @@ -87,11 +87,17 @@ pub fn CommandBuilder(comptime UserContext: type) type { comptime self: @This(), comptime callback: self.CallbackSignature(), allocator: std.mem.Allocator, - ) Parser(self, callback) { + ) !Parser(self, callback) { + // note: this is freed in Parser.deinit + var arena = try allocator.create(std.heap.ArenaAllocator); + arena.* = std.heap.ArenaAllocator.init(allocator); + const arena_alloc = arena.allocator(); + return Parser(self, callback){ - .allocator = allocator, - .subcommands = std.hash_map.StringHashMap(ParserInterface).init(allocator), - .help_builder = help.HelpBuilder(self).init(allocator), + .arena = arena, + .allocator = arena_alloc, + .subcommands = std.hash_map.StringHashMap(ParserInterface).init(arena_alloc), + .help_builder = help.HelpBuilder(self).init(arena_alloc), }; } diff --git a/source/parser.zig b/source/parser.zig index 4054280..3e48eb9 100644 --- a/source/parser.zig +++ b/source/parser.zig @@ -13,12 +13,29 @@ pub const ParserInterface = struct { parse: *const fn (parser: *anyopaque, context: *anyopaque, name: []const u8, args: [][:0]u8, env: std.process.EnvMap) anyerror!void, finish: *const fn (parser: *anyopaque, context: *anyopaque) anyerror!void, describe: *const fn () []const u8, + deinit: *const fn (parser: *anyopaque) void, + deinitTree: *const fn (parser: *anyopaque) void, }; parser: *anyopaque, context: *anyopaque, methods: *const Vtable, + fn create(comptime ParserType: type, parser: *anyopaque, context: *anyopaque) @This() { + return .{ + .parser = parser, + .context = context, + .methods = &.{ + .execute = ParserType.wrap_execute, + .parse = ParserType.wrap_parse, + .finish = ParserType.wrap_finish, + .describe = ParserType.describe, + .deinit = ParserType.wrap_deinit, + .deinitTree = ParserType.wrap_deinitTree, + }, + }; + } + pub fn execute(self: @This()) anyerror!void { return try self.methods.execute(self.parser, self.context); } @@ -34,6 +51,14 @@ pub const ParserInterface = struct { pub fn describe(self: @This()) []const u8 { return self.methods.describe(); } + + pub fn deinit(self: @This()) void { + self.methods.deinit(self.parser); + } + + pub fn deinitTree(self: @This()) void { + self.methods.deinitTree(self.parser); + } }; fn InterfaceGen(comptime ParserType: type, comptime UserContext: type) type { @@ -41,16 +66,7 @@ fn InterfaceGen(comptime ParserType: type, comptime UserContext: type) type { return if (CtxInfo == .Void) struct { pub fn interface(self: *ParserType) ParserInterface { - return .{ - .parser = self, - .context = @constCast(&void{}), - .methods = &.{ - .execute = ParserType.wrap_execute, - .parse = ParserType.wrap_parse, - .finish = ParserType.wrap_finish, - .describe = ParserType.describe, - }, - }; + return ParserInterface.create(ParserType, self, @constCast(&void{})); } fn cast_context(_: *anyopaque) void { @@ -58,16 +74,7 @@ fn InterfaceGen(comptime ParserType: type, comptime UserContext: type) type { } } else if (CtxInfo == .Pointer and CtxInfo.Pointer.size != .Slice) struct { pub fn interface(self: *ParserType, context: UserContext) ParserInterface { - return .{ - .parser = self, - .context = @ptrCast(@constCast(context)), - .methods = &.{ - .execute = ParserType.wrap_execute, - .parse = ParserType.wrap_parse, - .finish = ParserType.wrap_finish, - .describe = ParserType.describe, - }, - }; + return ParserInterface.create(ParserType, self, @constCast(context)); } fn cast_context(ctx: *anyopaque) UserContext { @@ -75,16 +82,7 @@ fn InterfaceGen(comptime ParserType: type, comptime UserContext: type) type { } } else struct { pub fn interface(self: *ParserType, context: *const UserContext) ParserInterface { - return .{ - .parser = self, - .context = @ptrCast(@constCast(context)), - .methods = &.{ - .execute = ParserType.wrap_execute, - .parse = ParserType.wrap_parse, - .finish = ParserType.wrap_finish, - .describe = ParserType.describe, - }, - }; + return ParserInterface.create(ParserType, self, @ptrCast(@constCast(context))); } fn cast_context(ctx: *anyopaque) UserContext { @@ -109,6 +107,7 @@ pub fn Parser(comptime command: anytype, comptime callback: anytype) type { consumed_args: u32 = 0, progname: ?[]const u8 = null, has_global_tags: bool = false, + arena: *std.heap.ArenaAllocator, allocator: std.mem.Allocator, subcommands: CommandMap, subcommand: ?ParserInterface = null, @@ -148,6 +147,16 @@ pub fn Parser(comptime command: anytype, comptime callback: anytype) type { return try self.finish(context); } + fn wrap_deinit(parser: *anyopaque) void { + const self = cast_interface_parser(parser); + self.deinit(); + } + + fn wrap_deinitTree(parser: *anyopaque) void { + const self = cast_interface_parser(parser); + self.deinitTree(); + } + fn describe() []const u8 { return command.description; } @@ -178,11 +187,22 @@ pub fn Parser(comptime command: anytype, comptime callback: anytype) type { if (self.subcommand) |verb| try verb.finish(); } + pub fn deinit(self: @This()) void { + self.arena.deinit(); + self.arena.child_allocator.destroy(self.arena); + } + + pub fn deinitTree(self: @This()) void { + var iterator = self.subcommands.valueIterator(); + while (iterator.next()) |subcommand| { + subcommand.deinitTree(); + } + self.deinit(); + } + pub fn execute(self: *@This(), context: UserContext) anyerror!void { const args = try std.process.argsAlloc(self.allocator); - defer std.process.argsFree(self.allocator, args); var env = try std.process.getEnvMap(self.allocator); - defer env.deinit(); if (args.len < 1) return ParseError.EmptyArgs;