-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Windows: Faster getenvW
and a standalone environment variable test
#23272
base: master
Are you sure you want to change the base?
Conversation
getenvW
and standalone environment variable testgetenvW
and a standalone environment variable test
Tests all environment variable APIs in std.process
77efe72
to
976ece0
Compare
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
976ece0
to
ebbf214
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice tests! I've got a bunch of comments, but they're mostly suggestions for more work, so feel free to ignore them as this seems like an improvement to me as-is.
Oh, one more suggestion: maybe expand the comment on getenvW
to say that it does linear searches and if the caller is going to frequently fetch environment variables, it may be worthwhile to use std.process.getEnvMap
to be able to query for them more efficiently?
run.clearEnvironment(); | ||
run.setEnvironmentVariable("FOO", "123"); | ||
run.setEnvironmentVariable("NO_VALUE", ""); | ||
run.setEnvironmentVariable("КИРиллИЦА", "non-ascii"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Maybe test with non-ASCII in the "value" part too?
And can you test with a key with a leading =
? That seems like a special case from the code.
{ | ||
try std.testing.expect(try std.process.hasNonEmptyEnvVar(allocator, "FOO")); | ||
try std.testing.expect(!(try std.process.hasNonEmptyEnvVar(allocator, "FO"))); | ||
try std.testing.expect(!(try std.process.hasNonEmptyEnvVar(allocator, "FOOO"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does looking up "FOO=" fail gracefully too?
@@ -529,29 +529,26 @@ pub fn getenvW(key: [*:0]const u16) ?[:0]const u16 { | |||
const key_slice = mem.sliceTo(key, 0); | |||
const ptr = windows.peb().ProcessParameters.Environment; | |||
var i: usize = 0; | |||
while (ptr[i] != 0) { | |||
while (ptr[i] != 0) : (i += mem.sliceTo(ptr[i..], 0).len + 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is concise, but seems like it might create unnecessary state (but I guess the optimizer can remove all that?). I think it'd be easier to read (and cleaner to implement) as something like: i = skipPastNextNull(ptr, i)
that just does a while loop over ptr? Feel free to leave this if you've got faith in the optimizer, though.
const std = @import("std"); | ||
const builtin = @import("builtin"); | ||
|
||
pub fn main() !void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here that this expects to be run by the build.zig which defines the expected environment.
const upcaseW = windows.ntdll.RtlUpcaseUnicodeChar; | ||
while (ptr[i] != 0 and key[i - key_start] != 0) : (i += 1) { | ||
if (i - key_start == key_slice.len) break; | ||
if (upcaseW(ptr[i]) != upcaseW(key[i - key_start])) break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code ends up upcasing the key for each entry in the environment. Can you stash a pre-upcased key on the stack that you can reuse? I don't see a well-defined limit on the key length ... ah, 32k is the limit on the overall environment length, so I guess that's a maximum key length? That seems a bit large, so maybe there isn't a good way to do this ...
Inspired by #23265, I thought I'd try applying the same strategy to the Windows implementation. Also adds a standalone test to make sure the functionality remains the same.
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, since 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 (#23265)
Benchmark code
(presumably, the magnitude of any speedup would also (on average) increase as the number of environment variables in the environment increases)