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

std.posix.getenv: early-return comparison #23265

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

g-logunov
Copy link

Addresses the issue described in #22917.

Possibly interferes with @andrewrk work on https://github.com/ziglang/zig/tree/main branch.

Original implementation https://github.com/ziglang/zig/blob/aa3db7cc15/lib/std/posix.zig#L2004 for each environment variable iterates until the end of its name (until =), and only then compares entire name to key.
Since some of the environment variables could be quite long (i.e. GHOSTTY_SHELL_INTEGRATION_NO_SUDO=1), these sizes add up.
Simply - in order to find a key in environ, it has to iterate over cumulative sizes of each env variable name before it.

Proposed implementation functionally does what strncmp would do: stops iterating and moves to the next variable on first character mismatch with key.

See the benchmarks below.
Disclaimer: this was tested on macOS, with a variation of #23264 fix applied.

// getenv-c.zig
const std = @import("std");

pub fn main() void {
    for (0..10_000_000) |_| {
        _ = std.mem.doNotOptimizeAway(std.c.getenv("FOOBAR"));
    }
}
// getenv-zig.zig
const std = @import("std");

pub fn main() void {
    for (0..10_000_000) |_| {
        _ = std.mem.doNotOptimizeAway(std.posix.getenv("FOOBAR"));
    }
}
> env | wc -l
      43
> hyperfine --warmup 3 ./getenv-c ./getenv-zig-aa3db7cc15 ./getenv-zig-speedup
Benchmark 1: ./getenv-c
  Time (mean ± σ):     294.1 ms ±   3.5 ms    [User: 293.1 ms, System: 0.8 ms]
  Range (min … max):   288.7 ms … 299.3 ms    10 runs

Benchmark 2: ./getenv-zig-aa3db7cc15
  Time (mean ± σ):      1.907 s ±  0.022 s    [User: 1.901 s, System: 0.005 s]
  Range (min … max):    1.881 s …  1.941 s    10 runs

Benchmark 3: ./getenv-zig-speedup
  Time (mean ± σ):     130.8 ms ±   0.5 ms    [User: 129.8 ms, System: 0.8 ms]
  Range (min … max):   129.7 ms … 131.7 ms    22 runs

Summary
  ./getenv-zig-speedup ran
    2.25 ± 0.03 times faster than ./getenv-c
   14.58 ± 0.17 times faster than ./getenv-zig-aa3db7cc15

To address the elephant in the room: this loop is duplicated, but I'm hesitant to refactor it in order to deduplicate because of
// TODO see https://github.com/ziglang/zig/issues/4524 preamble and ongoing work to fix it.

squeek502 added a commit to squeek502/zig that referenced this pull request Mar 17, 2025
Instead of parsing the full key and value for each environment variable before checking the key for (case-insensitive) equality, we skip to the next environment variable once it's no longer possible for the key to match.

This makes getting environment variables about 2x faster across the board on Windows.

Note: We still have to scan to find the end of each environment variable, even the ones that are skipped (we only know where it ends by a NUL terminator), so this strategy doesn't provide the same speedup on Windows as it does on POSIX (ziglang#23265)
squeek502 added a commit to squeek502/zig that referenced this pull request Mar 17, 2025
Instead of parsing the full key and value for each environment variable before checking the key for (case-insensitive) equality, we skip to the next environment variable once it's no longer possible for the key to match.

This makes getting environment variables about 2x faster across the board on Windows.

Note: We still have to scan to find the end of each environment variable, even the ones that are skipped (we only know where it ends by a NUL terminator), so this strategy doesn't provide the same speedup on Windows as it does on POSIX (ziglang#23265)
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