zig icon indicating copy to clipboard operation
zig copied to clipboard

`os.isCygwinPty`: Fix a bug, replace kernel32 call, and optimize

Open squeek502 opened this issue 2 years ago • 0 comments

  • Fixes the first few code units of the name being omitted (it was using @sizeOf(FILE_NAME_INFO) as the start of the name bytes, but that includes the length of the dummy [1]u16 field and padding; instead the start should be the offset of the dummy [1]u16 field). This effectively meant that it was only doing the -pty check, as the m in msys- was always being cut off.
  • Replaces kernel32.GetFileInformationByHandleEx call with ntdll.NtQueryInformationFile
    • Contributes towards #1840
  • Checks that the handle is a named pipe first (which is a much faster call than NtQueryInformationFile) before querying and checking the name (this was about a 10x speedup in my probably-not-so-good/take-it-with-a-grain-of-salt benchmarking)
Benchmark 1: iscyg-new.exe
  Time (mean ± σ):     320.7 ms ±   1.0 ms    [User: 43.8 ms, System: 277.2 ms]
  Range (min … max):   319.7 ms … 323.3 ms    10 runs

Benchmark 2: iscyg-old.exe
  Time (mean ± σ):      3.047 s ±  0.032 s    [User: 0.237 s, System: 2.810 s]
  Range (min … max):    3.017 s …  3.100 s    10 runs

Summary
  'iscyg-new.exe' ran
    9.50 ± 0.11 times faster than 'iscyg-old.exe'
Benchmarking code
const std = @import("std");

pub fn main() !void {
    const stderr = std.io.getStdErr().handle;
    const self_file = try std.fs.openSelfExe(.{});
    defer self_file.close();

    var timer = try std.time.Timer.start();

    {
        const iterations = 1_000_000;
        for (0..iterations) |_| {
            _ = std.os.isCygwinPty(stderr);
        }

        const time_taken = timer.read();
        const ns_per_call = time_taken / iterations;
        std.debug.print("stderr: {}ns per call\n", .{ns_per_call});
    }

    {
        const iterations = 1_000_000;
        for (0..iterations) |_| {
            _ = std.os.isCygwinPty(self_file.handle);
        }

        const time_taken = timer.read();
        const ns_per_call = time_taken / iterations;
        std.debug.print("self_exe: {}ns per call\n", .{ns_per_call});
    }
}

Also, tested with Git Bash to confirm that the function still detects msys- pipes correctly (left is git bash, right is cmd.exe):

iscyg

Test code
const std = @import("std");

pub fn main() !void {
    const stderr = std.io.getStdErr().handle;

    const is_cyg = std.os.isCygwinPty(stderr);
    std.debug.print("isCygwinPty: {}\n", .{is_cyg});
}

Also threw in some comments to other NtQueryInformationFile calls giving some context to BUFFER_OVERFLOW handling since that's something I had to figure out while implementing this.

Slightly related to #14829

squeek502 avatar Mar 08 '23 12:03 squeek502