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

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

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

squeek502
Copy link
Collaborator

@squeek502 squeek502 commented Mar 8, 2023

  • 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
  • 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

- 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)
- Replaces kernel32.GetFileInformationByHandleEx call with ntdll.NtQueryInformationFile
  + Contributes towards ziglang#1840
- Checks that the handle is a named pipe first before querying and checking the name, which is a much faster call than NtQueryInformationFile (this was about a 10x speedup in my probably-not-so-good/take-it-with-a-grain-of-salt benchmarking)
@squeek502 squeek502 force-pushed the is-cygwin-pty-stuff branch from 18c0bec to 93b35c6 Compare March 8, 2023 12:26
@Vexu Vexu merged commit 5e161c1 into ziglang:master Mar 21, 2023
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.

2 participants