Skip to content

Commit

Permalink
perf(trace): reduce allocations when formatting fields
Browse files Browse the repository at this point in the history
  • Loading branch information
@dnut
dnut committed Oct 8, 2024
1 parent 2460920 commit 2869887
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 32 deletions.
2 changes: 2 additions & 0 deletions src/trace/entry.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ pub const ChannelEntry = struct {
}

pub fn field(self: *Self, name: []const u8, value: anytype) *Self {
const min_capacity = self.fields.items.len + logfmt.countField(name, value);
self.fields.ensureTotalCapacity(min_capacity) catch return self;
logfmt.fmtField(self.fields.writer(), name, value);
return self;
}
Expand Down
57 changes: 25 additions & 32 deletions src/trace/logfmt.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,45 +11,38 @@ pub const LogMsg = struct {
maybe_fmt: ?[]const u8 = null,
};

pub fn fmtMsg(
writer: anytype,
comptime maybe_fmt: ?[]const u8,
args: anytype,
) void {
pub fn fmtMsg(writer: anytype, comptime maybe_fmt: ?[]const u8, args: anytype) void {
if (maybe_fmt) |fmt| {
std.fmt.format(writer, fmt, args) catch @panic("could not format");
}
}

pub fn fmtField(
writer: anytype,
key: []const u8,
value: anytype,
) void {
switch (@typeInfo(@TypeOf(value))) {
.Pointer, .Array => {
// Assume it's a string type
std.fmt.format(writer, "{s}={s} ", .{ key, value }) catch return;

This comment has been minimized.

Sign in to view

Copy link
@dadepo

dadepo Oct 9, 2024

Contributor

The issue with the previous implementation is that if it is an array like this:

const Num = enum{one, two};
const arr = [1]Num{Num.one};

ie not an array/pointer of u8 (string)

it would attempt to be formatted with {s}, which would be a runtime error.

This comment has been minimized.

Sign in to view

Copy link
@dnut

dnut Oct 9, 2024

Author Contributor

Yes it would have an error here at comptime if the child type is not u8.
},
.Int, .ComptimeInt, .Float, .ComptimeFloat => {
// Handle numeric types
std.fmt.format(writer, "{s}={} ", .{ key, value }) catch return;
},
.Null => {
// Handle null values
std.fmt.format(writer, "{s}=null ", .{key}) catch return;
},
else => {
// Fallback for unsupported types
std.fmt.format(writer, "{s}={any} ", .{ key, value }) catch return;
},
}
pub fn fmtField(writer: anytype, key: []const u8, value: anytype) void {

This comment has been minimized.

Sign in to view

Copy link
@dadepo

dadepo Oct 9, 2024

Contributor

Do we have a convention (or should we have one) regarding how long the params need to be before having it vertical?. I kinda went with the vertical alignment arbitrarily...

This comment has been minimized.

Sign in to view

Copy link
@dnut

dnut Oct 9, 2024

Author Contributor

We do not have an explicit convention for this. Personally I try to limit line lengths in all code that I write to be no more than 100 characters. If a function signature is less than 100 characters, I use a single line for it. Otherwise I break it up.
std.fmt.format(writer, fieldFmtString(@TypeOf(value)), .{ key, value }) catch return;
}

/// Return the number of bytes needed to write the field.
pub fn countField(key: []const u8, value: anytype) u64 {
return std.fmt.count(fieldFmtString(@TypeOf(value)), .{ key, value });
}

/// Return the format string for the type when used as a value in a field.
fn fieldFmtString(comptime Value: type) []const u8 {

This comment has been minimized.

Sign in to view

Copy link
@dadepo

dadepo Oct 9, 2024

Contributor

Seems like having the function params as anytype, needs @TypeOf before the @typeinfo can be used...but type does not?

Is there any recommendation regarding the usage of anytype and type? It seems in most use cases either can be used.

This comment has been minimized.

Sign in to view

Copy link
@dnut

dnut Oct 9, 2024

Author Contributor

It's hard to come up with one simple general rule that applies to all type vs anytype situations, but I can explain the pattern here.

Consider: Values of type type only exist at comptime. Usually*, when anytype is used, it's representing a runtime value.

Scenario: Often, you need a function to be called at comptime, and provide a return value that will be used at comptime. In cases like this, the value of every argument passed to that function also needs to be known at comptime**. That is the case here, since the return value is a fmt string that needs to be passed to another function as a comptime parameter.

So, the parameter can't be anytype in this case because the item that you're going to use as an anytype is not going to be resolved until runtime. But the anytype's type information is known at comptime. So, even within a function that only runs at runtime, you can resolve the types of values at comptime, and pass those types to functions that needs to run at comptime.

Comptime is a bit confusing at first because you can't always look at a block of code and say whether it runs at comptime or runtime. Often you'll have a block of code where certain parts are executed at comptime, and some at runtime. The same is true in many other languages, but zig significantly expands the possibilities of what can run at comptime. It just takes time and experimentation to get used to it. The compiler helps with this. I still run into surprises.

* You can pass a comptime known value as an argument where a function expects an anytype, but that's not common, and it's not what's happening here, so we can ignore that scenario.

** In theory, it is probably possible to implement a language that allows functions to return at comptime even if some parameters are not known at comptime, as long as the parameter value is not used by the function, but that's not how zig works.

This comment has been minimized.

Sign in to view

Copy link
@dadepo

dadepo Oct 10, 2024

Contributor

Thanks for the detailed explanation!
return switch (@typeInfo(Value)) {
// Assume arrays of u8 are strings.
.Pointer => |ptr| if (ptr.size == .One)
fieldFmtString(ptr.child)
else if (ptr.size == .One and ptr.child == u8)

This comment has been minimized.

Sign in to view

Copy link
@dadepo

dadepo Oct 9, 2024

Contributor

Will the else if branch get executed? Given it starts with ptr.size == .One check in the if branch?

This comment has been minimized.

Sign in to view

Copy link
@dnut

dnut Oct 9, 2024

Author Contributor

It will get executed any time you have an ordinary pointer. So it won't get executed for slices, but it will get executed for something like *u64 or *SomeStruct.

Importantly, this is executed for ordinary pointers that point to fixed size arrays. This is not the same as a slice. *[7]u8 is an example of this kind of type. This else if branch was specifically needed for some code that we have already. I think it was the word "Firefox" in a test that was not working until I added this.
"{s}={s} "
else
"{s}={any} ",
.Array => |arr| if (arr.child == u8) "{s}={s} " else "{s}={any} ",
.Int, .ComptimeInt, .Float, .ComptimeFloat => "{s}={} ",
else => "{s}={any} ",
};
}

pub fn writeLog(
writer: anytype,
message: LogMsg,
) !void {
pub fn writeLog(writer: anytype, message: LogMsg) !void {
if (message.maybe_scope) |scope| {
try std.fmt.format(writer, "[{s}] ", .{scope});
}
Expand Down

0 comments on commit 2869887

Please sign in to comment.