Process-wide caching of ToolsetConfigurationSection #6832
Conversation
to eliminate multiple loading MSBuild.exe.config
| @@ -250,6 +257,18 @@ protected override IEnumerable<ToolsetPropertyDefinition> GetSubToolsetPropertyD | |||
| /// Unit tests wish to avoid reading (nunit.exe) application configuration file. | |||
| /// </summary> | |||
| private static Configuration ReadApplicationConfiguration() | |||
| { | |||
| if (Environment.GetEnvironmentVariable("MSBUILDCACHETOOLSETCONFIGURATION") != "0") | |||
There was a problem hiding this comment.
Why a separate opt-out rather than a changewave?
There was a problem hiding this comment.
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.
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.
Makes sense. I have moved it under 17_0 change wave.
There was a problem hiding this comment.
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.
| { | ||
| // 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.
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.
MSBuild.exe.config should always be the only .exe.config of relevance, even in Locator scenarios.
There was a problem hiding this comment.
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.
Ah! There are two things in the app.config:
- .NET Framework configuration, such as binding redirects. These are pulled from
{TheActualApp}.exe.config. - MSBuild's configuration section, with toolsets and definitions of things like
$(MSBuildExtensionsPath). This is always fromMSBuild.exe.configand is what is relevant to this PR.
Forgind
added
the
merge-when-branch-open
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #6667
Context
Tools configuration is read from MSBuild.exe.config multiple time. We can simply cache related configuration section to optimize config file access.
Changes are under opt-out escape hatch.
Changes Made
Cached in static variable. Caching is not applied in unit tests cases.
Testing
Rebuilds and incremental build of orchard core.
ETL has been captured to verify reading from MSBuild.exe.config was optimized.
Notes
When measured the real perf difference in both CPU and wall-clock time was negligible/non-verifiable.