Skip to content

fix: add compatibility for Zig 0.16 ArrayList and timer changes#23

Open
jinzhongjia wants to merge 2 commits intomainfrom
016
Open

fix: add compatibility for Zig 0.16 ArrayList and timer changes#23
jinzhongjia wants to merge 2 commits intomainfrom
016

Conversation

@jinzhongjia
Copy link
Copy Markdown
Member

  • Replace legacy ArrayList initializers with initCapacity() in relevant places to support Zig 0.16, preserving compatibility with older releases.
  • Update benchmarks to use platform time routines when std.time.Timer is removed in Zig 0.16, and switch to page_allocator where GeneralPurposeAllocator is no longer available.
  • Expand workflow support to include Zig 0.16.0.
  • Clarify version support and adaptation details in documentation.

- Replace legacy ArrayList initializers with `initCapacity()` in relevant
  places to support Zig 0.16, preserving compatibility with older
  releases.
- Update benchmarks to use platform time routines when std.time.Timer is
  removed in Zig 0.16, and switch to page_allocator where
  GeneralPurposeAllocator is no longer available.
- Expand workflow support to include Zig 0.16.0.
- Clarify version support and adaptation details in documentation.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for Zig 0.16.0, updating version compatibility documentation and adapting the codebase to API changes in ArrayList initialization, timing, and memory allocation. Feedback focuses on correcting the false claim that GeneralPurposeAllocator was removed, suggesting the use of std.time.Instant for cross-platform timing, and recommending more idiomatic ArrayList initialization for Zig 0.16. Additionally, the reviewer advises against using page_allocator in benchmarks to avoid performance regressions.

Comment thread CLAUDE.md
- ArrayList initialization changes in Zig 0.16 (`std.ArrayList(T){}` removed, use `initCapacity`)
- Build system API differences between versions
- `std.time.Timer` removed in 0.16 (replaced with platform-specific timing in benchmarks)
- `std.heap.GeneralPurposeAllocator` removed in 0.16 (replaced with `page_allocator`)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The claim that std.heap.GeneralPurposeAllocator is removed in Zig 0.16 is incorrect. It remains a core part of the standard library, although its internal implementation details or its location within std.heap may have evolved. Switching to page_allocator in benchmarks is a regression in measurement quality as it is significantly slower due to per-allocation system calls.

Comment thread src/bench.zig
Comment on lines +22 to +38
fn getTimeNs() u64 {
if (builtin.os.tag == .windows) {
const w = std.os.windows;
var counter: w.LARGE_INTEGER = undefined;
_ = w.ntdll.RtlQueryPerformanceCounter(&counter);
var freq: w.LARGE_INTEGER = undefined;
_ = w.ntdll.RtlQueryPerformanceFrequency(&freq);
const ns = std.time.ns_per_s;
const c: u64 = @bitCast(counter);
const f: u64 = @bitCast(freq);
return @intCast(@divTrunc(@as(u128, c) * ns, @as(u128, f)));
} else if (builtin.os.tag != .windows) {
var ts: std.c.timespec = undefined;
_ = std.c.clock_gettime(std.c.CLOCK.MONOTONIC, &ts);
return @as(u64, @intCast(ts.tv_sec)) * std.time.ns_per_s + @as(u64, @intCast(ts.tv_nsec));
} else @compileError("unsupported OS for benchmark");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The manual implementation of getTimeNs introduces significant boilerplate and potential portability issues (e.g., dependency on libc for clock_gettime on non-Windows platforms). For Zig 0.16, the recommended replacement for std.time.Timer is std.time.Instant, which provides a cross-platform monotonic clock API. Additionally, the Windows implementation lacks a check for a zero frequency from RtlQueryPerformanceFrequency, which would cause a division by zero.

Comment thread src/bench.zig
Comment on lines +33 to +37
} else if (builtin.os.tag != .windows) {
var ts: std.c.timespec = undefined;
_ = std.c.clock_gettime(std.c.CLOCK.MONOTONIC, &ts);
return @as(u64, @intCast(ts.tv_sec)) * std.time.ns_per_s + @as(u64, @intCast(ts.tv_nsec));
} else @compileError("unsupported OS for benchmark");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The else if (builtin.os.tag != .windows) condition is redundant following the initial if (builtin.os.tag == .windows). This makes the final else block containing the @compileError unreachable. It should be simplified to a standard else block.

    } else {
        var ts: std.c.timespec = undefined;
        _ = std.c.clock_gettime(std.c.CLOCK.MONOTONIC, &ts);
        return @as(u64, @intCast(ts.tv_sec)) * std.time.ns_per_s + @as(u64, @intCast(ts.tv_nsec));
    }

Comment thread src/bench.zig
var gpa = std.heap.GeneralPurposeAllocator(.{}){};
defer _ = gpa.deinit();
const allocator = gpa.allocator();
const allocator = std.heap.page_allocator;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using std.heap.page_allocator for benchmarks is generally discouraged because it performs a system call for every allocation and deallocation, which can dominate the benchmark results and hide the actual performance characteristics of the code being tested. Since GeneralPurposeAllocator is still available in Zig 0.16, it should be used instead.

Comment thread src/bench.zig
Comment on lines +55 to +57
var legacy_timer: if (is_zig_16) void else std.time.Timer = if (is_zig_16) {} else undefined;
if (!is_zig_16) legacy_timer = try std.time.Timer.start();
const start_ns = if (is_zig_16) getTimeNs() else 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The conditional logic for handling the timer across Zig versions is quite clunky. Instead of manually implementing getTimeNs and using a void type for legacy_timer, it is recommended to use std.time.Instant for Zig 0.16. This would allow for a cleaner abstraction or a simple wrapper that works across all supported versions.

Comment thread src/msgpack.zig
Comment on lines 1316 to +1319
var new_heap = if (current_zig.minor == 14)
std.ArrayList(Payload).init(alloc)
else
std.ArrayList(Payload){};
std.ArrayList(Payload).initCapacity(alloc, 0) catch unreachable;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using initCapacity(alloc, 0) catch unreachable is suboptimal and introduces a potential crash point during memory cleanup. In Zig 0.16, std.ArrayList(T).init(allocator) is the preferred way to initialize a managed ArrayList as it does not perform an initial allocation and thus cannot fail. The logic can be simplified to use init for both 0.14 and 0.16, only using the struct literal for 0.15 where the ArrayList was unmanaged.

                    var new_heap = if (current_zig.minor == 15)
                        std.ArrayList(Payload){}
                    else
                        std.ArrayList(Payload).init(alloc);

Comment thread src/msgpack.zig
Comment on lines 2857 to +2860
var parse_stack = if (current_zig.minor == 14)
std.ArrayList(ParseState).init(allocator)
else
std.ArrayList(ParseState){};
try std.ArrayList(ParseState).initCapacity(allocator, 0);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the previous comment, std.ArrayList(T).init(allocator) should be used for Zig 0.14 and 0.16 to avoid the unnecessary try or catch unreachable on an operation that doesn't need to allocate memory initially.

            var parse_stack = if (current_zig.minor == 15)
                std.ArrayList(ParseState){}
            else
                std.ArrayList(ParseState).init(allocator);

Comment thread src/test.zig
Comment on lines 78 to +81
var input = if (builtin.zig_version.minor == 14)
std.ArrayList(u8).init(allocator)
else
std.ArrayList(u8){};
std.ArrayList(u8).initCapacity(allocator, 0) catch unreachable;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of initCapacity(allocator, 0) catch unreachable throughout the test suite is repetitive and less idiomatic than using init(allocator) for versions where the ArrayList is managed (0.14 and 0.16).

    var input = if (builtin.zig_version.minor == 15)
        std.ArrayList(u8){}
    else
        std.ArrayList(u8).init(allocator);

- goto-bus-stop/setup-zig 仓库已归档,后续不再维护
- 替换为 mlugg/setup-zig 保证后续兼容和安全
- 避免旧 action 可能引起的集成失效或安全风险
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant