Skip to content

Conversation

@arBmind
Copy link

@arBmind arBmind commented Jan 23, 2022

No description provided.

@marler8997
Copy link
Collaborator

marler8997 commented Jan 24, 2022

Your branch made me realize that I should have been importing Guid instead of referencing its fully-qualified name each time. I pushed a commit that fixed that here: 449b7cd

I rebased your commits and added an additional one that removes Guid completely from zig.zig and updates all references to use the one from std directly.

For some reason this this is causing zig test zigwin32\win32.zig to take forever for me...

@marler8997 marler8997 force-pushed the feature/std-os-windows-guid branch from b2842b1 to 4c21952 Compare January 24, 2022 08:30
arBmind and others added 3 commits January 24, 2022 01:35
* custom parsing because Guid.parse expects curly braces.
* requires win32jsongen feature guid-with-curly-braces
@marler8997 marler8997 force-pushed the feature/std-os-windows-guid branch from 4c21952 to 6ab2afb Compare January 24, 2022 08:44
@arBmind
Copy link
Author

arBmind commented Jan 24, 2022

@marler8997 Thank you for considering my PR.

My first commit made it work, without any changes.
My second commit requires this PR marlersoft/win32jsongen#4 for win32jsongen to work properly. This adds curly braces to the GUIDs in the JSON files.

@marler8997
Copy link
Collaborator

marler8997 commented Jan 24, 2022

Ok, well the commit I added should also work, it just adds the curly braces when generating the code. However it's still making my zig test zigwin32\win32.zig take much longer but I'm not sure why (like 100 times longer, around 45 seconds compared to it's still running after 20 minutes or so). I created a small test app to see if std GUID was slower at comptime but I didn't see much difference. Will have to investigate more.

Here's the perf test app for reference:

const builtin = @import("builtin");
const std = @import("std");

pub fn main() void {
    comptime {
        const count = 1000000;
        @setEvalBranchQuota(count * 3);
        var i: usize = 0;
        while (i < count) : (i += 1) {
            //_ = std.os.windows.GUID.parse("{aa7a8931-80e4-4fec-8f3b-553f87b4966e}");
            _ = Guid.parse("aa7a8931-80e4-4fec-8f3b-553f87b4966e");
        }
    }
}


// TODO: this should probably be in the standard lib somewhere?
pub const Guid = extern union {
    Ints: extern struct {
        a: u32,
        b: u16,
        c: u16,
        d: [8]u8,
    },
    Bytes: [16]u8,

    const big_endian_hex_offsets = [16] u6 {0, 2, 4, 6, 9, 11, 14, 16, 19, 21, 24, 26, 28, 30, 32, 34};
    const little_endian_hex_offsets = [16] u6 {
        6, 4, 2, 0,
        11, 9,
        16, 14,
        19, 21, 24, 26, 28, 30, 32, 34};
    const hex_offsets = switch (builtin.target.cpu.arch.endian()) {
        .Big => big_endian_hex_offsets,
        .Little => little_endian_hex_offsets,
    };

    pub fn parse(s: []const u8) Guid {
        var guid = Guid { .Bytes = undefined };
        for (hex_offsets) |hex_offset, i| {
            //guid.Bytes[i] = decodeHexByte(s[offset..offset+2]);
            guid.Bytes[i] = decodeHexByte([2]u8 { s[hex_offset], s[hex_offset+1] });
        }
        return guid;
    }
};
comptime { std.debug.assert(@sizeOf(Guid) == 16); }

// TODO: is this in the standard lib somewhere?
fn hexVal(c: u8) u4 {
    if (c <= '9') return @intCast(u4, c - '0');
    if (c >= 'a') return @intCast(u4, c + 10 - 'a');
    return @intCast(u4, c + 10 - 'A');
}

// TODO: is this in the standard lib somewhere?
fn decodeHexByte(hex: [2]u8) u8 {
    return @intCast(u8, hexVal(hex[0])) << 4 | hexVal(hex[1]);
}

@marler8997
Copy link
Collaborator

marler8997 commented Jan 24, 2022

Ok I've grepped all of zigwin32 for all the Guids and put them into a single file to test performance (see https://gist.github.com/marler8997/2ab2fc12190792308d233efcafe89ac3). My custom Guid takes about 12 seconds to run zig test guids.zig, the one from std.os.windows takes about 6 minutes! To see for yourself, change the variable use_std_guid in the gist above to test the performance of each one.

@marler8997
Copy link
Collaborator

I've submitted a PR to create a benchmark for GUID.parse here: ziglang/gotta-go-fast#21

Once that's in I'll see about replacing the implementation in std with the one here, then we could switch to it.

@clembu
Copy link

clembu commented Apr 19, 2025

Ok I've grepped all of zigwin32 for all the Guids and put them into a single file to test performance (see https://gist.github.com/marler8997/2ab2fc12190792308d233efcafe89ac3). My custom Guid takes about 12 seconds to run zig test guids.zig, the one from std.os.windows takes about 6 minutes! To see for yourself, change the variable use_std_guid in the gist above to test the performance of each one.

I ran both versions against my 0.14.0 zig setup, and both run in about the same time. (there aren't any timers doing actual measurement so it's hard to tell precisely)
so maybe it's worth reconsidering?

@marler8997
Copy link
Collaborator

Yeah, looks like my changes to improve std guid performance are still intact. We could move to it. It doesn't look like the gotta-go-fast benchmark is still running though, so we'll need to be vigilant and looking out for whether the performance regresses again.

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.

3 participants