-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Process-wide caching of ToolsetConfigurationSection #6832
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,49 @@ namespace Microsoft.Build.Evaluation | |
/// </summary> | ||
internal static class ToolsetConfigurationReaderHelpers | ||
{ | ||
/// <summary> | ||
/// Lock for process wide ToolsetConfigurationSection section cache | ||
/// </summary> | ||
private static readonly object s_syncLock = new(); | ||
|
||
/// <summary> | ||
/// Process wide ToolsetConfigurationSection section cache | ||
/// </summary> | ||
private static ToolsetConfigurationSection s_toolsetConfigurationSectionCache; | ||
private static Configuration s_configurationOfCachedSection; | ||
|
||
internal static ToolsetConfigurationSection ReadToolsetConfigurationSection(Configuration configuration) | ||
{ | ||
if (Environment.GetEnvironmentVariable("MSBUILDCACHETOOLSETCONFIGURATION") != "0") | ||
{ | ||
if (configuration == null) | ||
{ | ||
return null; | ||
} | ||
|
||
lock (s_syncLock) | ||
{ | ||
// Cache 1st requested configuration section. In unit tests, different Configuration is provided for particular test cases. | ||
// During runtime, however, only MSBuild exe configuration file is provided to read toolset configuration from, | ||
// and modifying MSBuild exe configuration during lifetime of msbuild nodes is neither expected nor supported. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is more supported if someone uses MSBuildLocator to get here, so msbuild.exe.config isn't the exe.config of relevance, and they proceed to use the same (in proc) node to do something else. It still sounds pretty unexpected to me, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MSBuild.exe.config should always be the only .exe.config of relevance, even in Locator scenarios. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't we find that with in-proc nodes from Locator, it didn't even know about msbuild.exe.config by default? It was using BuilderApp.exe.config, which I could have modified to fit my scenario. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! There are two things in the app.config:
|
||
if (s_toolsetConfigurationSectionCache == null) | ||
{ | ||
s_toolsetConfigurationSectionCache = GetToolsetConfigurationSection(configuration); | ||
s_configurationOfCachedSection = configuration; | ||
} | ||
|
||
return s_configurationOfCachedSection == configuration ? | ||
s_toolsetConfigurationSectionCache : | ||
GetToolsetConfigurationSection(configuration); | ||
} | ||
} | ||
else | ||
{ | ||
return GetToolsetConfigurationSection(configuration); | ||
} | ||
} | ||
|
||
private static ToolsetConfigurationSection GetToolsetConfigurationSection(Configuration configuration) | ||
{ | ||
ToolsetConfigurationSection configurationSection = null; | ||
|
||
|
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.
Why a separate opt-out rather than a changewave?
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.
I am not completely sure, when to use changewave vs escape-hatches. I am inclining to use changewave when msbuild change its behavior and when probability of regression is considerable.
The fact that user requires previous behavior of msbuild does not necessary means he wants to reject perf optimizations as well.
Though I might be looking at it wrong. Please let me know if you prefer to use changewave here.
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.
For a thing like this where we hope it is never needed, I think a changewave is best since they'll "naturally" expire as we follow the policy. I tend to favor explicit switches (usually in addition to a changewave) only if we expect to need the configurability "forever", like in #6560.
@benvillalobos or @marcpopMSFT might have more detailed or different thoughts though.
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.
Makes sense. I have moved it under 17_0 change wave.
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.
To add onto rainer's thoughts, if there were backlash such that we know a permanent workaround is needed then we can always add the secondary/permanent escape hatch later. Perhaps as part of the wave "rotating out" when we've gathered more real world data.