-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Perf: Frozen environment variable caching #12019
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
Perf: Frozen environment variable caching #12019
Conversation
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.
Pull Request Overview
This PR improves performance and memory usage during build by caching and reusing a frozen snapshot of environment variables instead of repeatedly re-allocating mutable dictionaries. Key changes include introducing a new EnvironmentState record for caching, switching from mutable Dictionary to FrozenDictionary across various modules, and updating translator method signatures to use IDictionary.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/Shared/TranslatorHelpers.cs | Added an extension for translating FrozenDictionary values. |
src/Shared/CommunicationsUtilities.cs | Implemented environment block comparison and caching. |
src/Framework/ITranslator.cs & BinaryTranslator.cs | Updated TranslateDictionary signatures to use IDictionary. |
src/Build/BackEnd/* (various files) | Replaced Dictionary with FrozenDictionary for environment caching. |
Comments suppressed due to low confidence (2)
src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs:1382
- Review the change from using BuildProcessEnvironment (mutable) to BuildProcessEnvironmentInternal (FrozenDictionary) to ensure that all consumers are compatible with an immutable API.
SetEnvironmentVariableBlock(_componentHost.BuildParameters.BuildProcessEnvironmentInternal);
src/Shared/TranslatorHelpers.cs:189
- Ensure that the conversion to FrozenDictionary preserves the intended comparer semantics and that all code relying on a mutable dictionary has been updated to work with an immutable collection.
dictionary = localDict?.ToFrozenDictionary(comparer);
Fixes
CPU + allocations + memory usage by repeated construction of environment variables by returning a cached
FrozenDictionary
instance when no changes exist.These are also currently held onto by
ConfigCache -> BuildRequestConfiguration
, so these will actually stay in the working set for the entire build if not de-duped.Before:
After:
Context
On .NET Core,
Environment.GetEnvironmentVariables()
internally creates a new Hashtable instance and string allocations for each key-value pair. Since the rest of MSBuild expects a typedDictionary<string, string>
instance, we need to allocate yet another dictionary and copy all the results.On .NET Framework, we use the native Win32 APIs so we directly parse into a
Dictionary
instance, but we still end up allocating a new instance on every call and a bunch of additional strings.The environment set only changes a handful of times throughout a build, so in reality we're just creating the same set over and over again.
Given that these are then handed to long lived objects, we also just end up with a ton of duplicated strings in memory. Here's a time slice of
OrchardCode
taken at the same point of the build graph, where ~100k strings less are being held on to in memory after this:Changes Made
This introduces a container for caching the previous environment state. If we read the environment variables and the count, keys, and values are an exact match, we skip the allocation altogether and return a
FrozenDictionary
instance.This also changes .NET Core on Windows to use the native codepath. In this case, we can use the entire environment block string as our cache comparison, and utilize
StringTools
to avoid unnecessary extra string allocations if we need to build a new set.On Unix, we still need to call
Environment.GetEnvironmentVariables()
, but we can still avoid the extra dictionary allocation and duplicated memory by doing a set comparison.The
TaskHost
path is ifdef-ed to behave as before, due to the absence ofSystem.Collections.Frozen
.Notes
My one main concern was whether the dictionary is exposed in some public API that expects a mutable dictionary. As far as I can tell, that isn't the case since the only place we expose it (in
BuildParameters.BuildProcessEnvironment
) already returns aReadOnlyDictionary
, and the rest of our uses are internal and already don't mutate