perf: write fixed-size values directly into the writer buffer#18
Merged
Conversation
Encoding built every multi-byte value in a stack array and handed it to writeAll, whose @memcpy sees a runtime length and lowers to a call into compiler_rt. Encoding a 4-field struct cost 7 writeAll and 7 memcpy calls to move chunks of 2 to 8 bytes. Add reserveArray, which hands back a *[len]u8 into the writer's buffer with len comptime-known, so stores through it become immediate stores. Build packHeaderAndInt on top of it and route every header-plus-payload write through it: fixed-size ints, floats, and the str/bin/array/map headers. This also makes those writes atomic with respect to a draining writer, which previously could split a header from its length. Pack struct and union map keys derived from field names as comptime literals, so the header byte and the string bytes are a single store. packIntValue is gone; nothing needs it now that headers and payloads are written together. packUnionAsMap and packUnionAsTagged take comptime opts, matching packStructAsMap, so their key names reach packStringLiteral. Encoding a {u64, i32, f64, bool} struct as a map with field-name keys, on this machine, instructions per operation: ReleaseSafe ReleaseFast before 1001 594 after 249 184 The encode path for that struct now issues no calls at all.
📝 WalkthroughWalkthroughChangesThe serialization code centralizes header-and-integer writes through new writer utilities. A compile-time string literal encoder is exported and used for struct and union metadata keys, while the previous public integer-packing helper is removed. Packing and literal encoding
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StructOrUnion
participant packStringLiteral
participant Writer
StructOrUnion->>packStringLiteral: provide comptime field name or prefix
packStringLiteral->>Writer: write encoded string header, length, and bytes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/float.zig (1)
35-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCombine the two duplicate switches into one.
Both switches operate on
type_info.float.bitswith identical ranges (0...32,33...64). The second switch'selse => unreachableis safe today only because the first switch's@compileErrorguards it at comptime — but the duplication invites divergence if a new float size is added to one switch and not the other.♻️ Proposed refactor: single switch returning both TargetType and header
- const TargetType = switch (type_info.float.bits) { - 0...32 => f32, - 33...64 => f64, - else => `@compileError`("Unsupported float size"), - }; - const header = switch (type_info.float.bits) { - 0...32 => hdrs.FLOAT32, - 33...64 => hdrs.FLOAT64, - else => unreachable, - }; + const float_info = switch (type_info.float.bits) { + 0...32 => .{ .target_type = f32, .header = hdrs.FLOAT32 }, + 33...64 => .{ .target_type = f64, .header = hdrs.FLOAT64 }, + else => `@compileError`("Unsupported float size"), + };Then update the downstream references:
- const IntType = std.meta.Int(.unsigned, `@bitSizeOf`(TargetType)); - const int_value = `@as`(IntType, `@bitCast`(`@as`(TargetType, `@floatCast`(value)))); - return packHeaderAndInt(writer, header, IntType, int_value); + const IntType = std.meta.Int(.unsigned, `@bitSizeOf`(float_info.target_type)); + const int_value = `@as`(IntType, `@bitCast`(`@as`(float_info.target_type, `@floatCast`(value)))); + return packHeaderAndInt(writer, float_info.header, IntType, int_value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/float.zig` around lines 35 - 44, Combine the duplicate switches on type_info.float.bits in the relevant declaration into one switch that returns both TargetType and header for each supported range, while preserving `@compileError` for unsupported sizes. Update downstream references to use the combined result’s fields and remove the separate header switch.src/string.zig (1)
65-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
packStringLiteralis well-implemented; consider a direct test for STR8+ paths.The comptime block correctly mirrors
packStringHeaderencoding across all four format branches, and thereserveArray/writeAllfallback pattern is sound. The existing struct and union tests only exercise the fixstr path (field names ≤ 31 bytes). A direct unit test covering the STR8 boundary (e.g., a 32-byte comptime string) would protect the untested branches.🧪 Suggested test for STR8+ coverage
test "packStringLiteral: str8 boundary" { const value = "abcdefghijklmnopqrstuvwxyz012345"; // 32 bytes var buffer: [64]u8 = undefined; var writer = std.Io.Writer.fixed(&buffer); try packStringLiteral(&writer, value); const expected = [_]u8{ hdrs.STR8, 32 } ++ value[0..].*; try std.testing.expectEqualSlices(u8, &expected, writer.buffered()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/string.zig` around lines 65 - 93, Add a direct unit test for packStringLiteral using a 32-byte comptime string to exercise the STR8 boundary, asserting the output contains the STR8 header, length byte, and string contents. Include the test alongside the existing string tests and use the fixed writer pattern already used in the suggested coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/float.zig`:
- Around line 35-44: Combine the duplicate switches on type_info.float.bits in
the relevant declaration into one switch that returns both TargetType and header
for each supported range, while preserving `@compileError` for unsupported sizes.
Update downstream references to use the combined result’s fields and remove the
separate header switch.
In `@src/string.zig`:
- Around line 65-93: Add a direct unit test for packStringLiteral using a
32-byte comptime string to exercise the STR8 boundary, asserting the output
contains the STR8 header, length byte, and string contents. Include the test
alongside the existing string tests and use the fixed writer pattern already
used in the suggested coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fc9d5d8-d119-4be1-bee6-7fd6267b5848
📒 Files selected for processing (10)
src/array.zigsrc/binary.zigsrc/float.zigsrc/int.zigsrc/map.zigsrc/msgpack.zigsrc/string.zigsrc/struct.zigsrc/union.zigsrc/utils.zig
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every multi-byte value on the encode path was built in a stack array and handed to
writeAll.writeAllcallsWriter.write, whose@memcpyreceives a runtime length and therefore lowers to a call intocompiler_rt's generic byte mover — even for 2 bytes.Encoding a 4-field struct as a map with field-name keys cost 7
writeAllcalls and 7memcpycalls, to move chunks of 2 to 8 bytes each. Callgrind put 57% of the instructions in those two functions. For comparison, msgpuck's equivalent encode issues zero calls; GCC folds itsmp_encode_*inline functions into immediate stores.writeBytewas never part of the problem — it has an inlined buffer-store fast path, which is why single-byte headers andboolfields cost nothing measurable.Changes
reserveArray(utils.zig) hands back a*[len]u8into the writer's buffer withlencomptime-known, so stores through it are immediate stores. Returns null when the value doesn't fit, and callers fall back towriteAll.packHeaderAndIntbuilds on it: one capacity check, one contiguous store of header + big-endian payload. Every fixed-size int, float, andstr/bin/array/mapheader now goes through it. Previously each of those tested the buffer capacity twice — once inwriteBytefor the header, once again for the length.packStringLiteralpacks a comptime-known string as a single store, with the header byte and string bytes concatenated at comptime. Used for all struct and union map keys derived from field names.packIntValueis removed. Nothing needs it now that headers and payloads are written together.packUnionAsMapandpackUnionAsTaggedtakecomptime opts, matchingpackStructAsMap, so their field names reachpackStringLiteral.The header/payload fusing also fixes a latent correctness wart: against a draining writer whose buffer filled between the two writes, a header could land in a different
draincall than its own length. Each is now a singlewriteAllon the slow path.Results
Encoding a
{u64, i32, f64, bool}struct as a map with field-name keys. Instructions per operation, measured with callgrind (deterministic;perfagrees to within 0.5%):-O3)The encode path for that struct now issues no calls at all — no
writeAll, nomemcpy.A struct with two runtime-length string values goes from 1056 to 479 (ReleaseSafe) and 467 to 315 (ReleaseFast); its keys are literals now, but the string payloads still need a real
memcpy.Wall-clock numbers are omitted deliberately: the machine I measured on was drifting by ~20% between runs on unmodified code. Instruction counts reproduce exactly.
Notes
zig build testpasses, including the msgpack conformance vectors.unpackIntValueandreadFloatValuestillreadSliceAllinto a stack buffer, which is the same pattern this PR eliminated on the write side. Worth a follow-up.Summary by CodeRabbit
packIntValuehelper.