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

Improve throughput of Environment.GetEnvironmentVariables() #45057

Merged
merged 2 commits into from
Nov 22, 2020

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Nov 21, 2020

Use IndexOf{Any} to search for positions rather than open-coded loops, taking advantage of vectorization to improve throughput.

[Benchmark]
public IDictionary Get() => Environment.GetEnvironmentVariables();

With the environment variables in my command prompt:

Method Job Toolchain Mean Error StdDev Median Ratio Gen 0 Gen 1 Gen 2 Allocated
Get Job-PPEVTD \master\corerun.exe 35.17 us 0.423 us 0.396 us 35.10 us 1.00 5.1270 0.8545 - 31.43 KB
Get Job-PCZKGH \pr\corerun.exe 14.61 us 0.278 us 0.726 us 14.33 us 0.41 5.1270 0.8545 - 31.43 KB

Contributes to #44598

@stephentoub stephentoub added this to the 6.0.0 milestone Nov 21, 2020
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Use IndexOf to search for positions rather than open-coded loops, taking advantage of vectorization to improve throughput.
@jkotas jkotas merged commit bf302de into dotnet:master Nov 22, 2020
@stephentoub stephentoub deleted the envvarsindexof branch November 22, 2020 23:32
@adamsitnik
Copy link
Member

@stephentoub do you have any plans on improving the Unix implementation as well? This could improve the developer loop experience on macOS

@am11
Copy link
Member

am11 commented Nov 23, 2020

@adamsitnik, Environment.Variables.Windows.cs is compiled for both Windows and Unix under feature coreclr.

@adamsitnik
Copy link
Member

is compiled for both Windows and Unix under feature coreclr

@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
Copy link
Member Author

do you have any plans on improving the Unix implementation as well? This could improve the developer loop experience on macOS

As @am11 said, this is the implementation of GetEnvironmentVariables for both Windows and Unix. GetEnvironmentVariable (singular) doesn't use this, on any platform.

@adamsitnik
Copy link
Member

Are there any plans to improve the performance of Unix implementation of the GetEnvironmentVariable (singular) method?

@stephentoub
Copy link
Member Author

stephentoub commented Nov 23, 2020

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
Copy link
Member

Perf lab results shows improvements - DrewScoggins/performance-2#3515

@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants