- Notifications
You must be signed in to change notification settings - Fork5.2k
Improve throughput of Environment.GetEnvironmentVariables()#45057
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
0a25ea4 toed592e4Comparesrc/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
jkotas left a comment
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!
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentStrings.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
ed592e4 tob87f3a3Comparesrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.FreeEnvironmentStrings.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Use IndexOf to search for positions rather than open-coded loops, taking advantage of vectorization to improve throughput.
b87f3a3 to8fe2a9bCompareadamsitnik commentedNov 23, 2020
@stephentoub do you have any plans on improving the Unix implementation as well? This could improve the developer loop experience on macOS |
am11 commentedNov 23, 2020
@adamsitnik, |
adamsitnik commentedNov 23, 2020
@am11 it might be, but according to the data shared in#41871, getting a single env var on macOS is still 4 times slower compared to Windows (using same hardware). And the time is not spent in the method that@stephentoub has optimized in this particular PR (see#866 (comment) for details) |
stephentoub commentedNov 23, 2020
As@am11 said, this is the implementation of GetEnvironmentVariables for both Windows and Unix. GetEnvironmentVariable (singular) doesn't use this, on any platform. |
adamsitnik commentedNov 23, 2020
Are there any plans to improve the performance of Unix implementation of the GetEnvironmentVariable (singular) method? |
stephentoub commentedNov 23, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The costs you highlight for GetEnvironmentVariable are because the runtime reads and stores the UTF8 env vars on first use, and then calls to GetEnvironmentVariable need to transcode the UTF16 key being searched for, which is done linearly. To make GetEnvironmentVariable faster in microbenchmarks, the runtime could instead pre-transcode them all and potentially store them in a hashtable. The problem, though, is trying to optimize for microbenchmarks on the throughput of GetEnvironmentVariable will likely lead you to do the "wrong" thing. No one should be calling GetEnvironmentVariable on a hot path; instead, while there are exceptions to this, it's generally used once for a given key, often during startup / first use of some code that checks it for configuration info and then never looks at it again; in my experience, there are also generally many more environment variables in the environment than the app actually cares about. We could spend way more time transcoding all env vars, find we made a benchmark.net test of GetEnvironmentVariable much faster, but actually slowed down the overall startup of an app with no measurable reward to show for it. If you'd like to prototype that and can demonstrate it does help with the overall startup of various app types (e.g. because it turns out we end up looking for lots of environment variables, maybe multiple times, or because we have to transcode them all anyway for an inevitable call to GetEnvironmentVariables, plural), then we could certainly consider that direction. It would make multiple calls to GetEnvironmentVariables faster as well. (It's also possible there are some simpler, smaller optimizations possible. For example, if the vast majority of keys are ASCII, maybe there could be a fast-path that skips the transcoding initially and tries to do the comparison between the byte and char unless a non-ASCII value is hit.) |
kunalspathak commentedNov 24, 2020
Perf lab results shows improvements -DrewScoggins/performance-2#3515 |
Uh oh!
There was an error while loading.Please reload this page.
Use IndexOf{Any} to search for positions rather than open-coded loops, taking advantage of vectorization to improve throughput.
With the environment variables in my command prompt:
Contributes to#44598