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

Consider caching toolset information in memory #6667

Closed
ladipro opened this issue Jul 13, 2021 · 6 comments · Fixed by #6832
Closed

Consider caching toolset information in memory #6667

ladipro opened this issue Jul 13, 2021 · 6 comments · Fixed by #6832
Assignees
Labels
grab-next The issue is at the top of the backlog and should be worked on soon. performance triaged
Milestone

Comments

@ladipro
Copy link
Member

ladipro commented Jul 13, 2021

Issue Description

From @rainersigwald:
MSBuild stores “toolset” information in the .exe.config, which is used to populate default properties and do some other configuration. Today we will read that once per ProjectCollection — we can consider caching it for process lifetime since “manually editing msbuild.exe.config” is not a common/supported scenario and VS upgrades would always kill processes.

Steps to Reproduce

Create ProjectCollection and notice that several config files are read (machine.config, msbuild.exe.config).

Data

BytesRead(DuplicationFactor) FilePath
1,323,008(~80.8x) C:\Program Files\Microsoft Visual Studio\2022\Preview\MSBuild\Current\Bin\amd64\MSBuild.exe.config
@ladipro ladipro added performance needs-triage Have yet to determine what bucket this goes in. labels Jul 13, 2021
@nohwnd
Copy link
Member

nohwnd commented Jul 14, 2021

Caching the data would also affect reading machine.config which is read before the .exe.config.

BytesRead(DuplicationFactor) FilePath
1,966,080(~49.1x) C:\Windows\Microsoft.NET\Framework64\v4.0.30319\Config\machine.config

@rainersigwald
Copy link
Member

@nohwnd isn't machine.config a CLR thing? I don't see how we could affect that.

@nohwnd
Copy link
Member

nohwnd commented Jul 14, 2021

Under devenv.exe I saw it loaded when the .exe.config was loaded by System.Configuration. So reducing the number of times the .exe.config gets loaded by ProjectCollection, will reduce the number of times machine.config gets loaded.

@Forgind Forgind removed the needs-triage Have yet to determine what bucket this goes in. label Jul 14, 2021
@Forgind Forgind added this to the Backlog milestone Jul 14, 2021
@nohwnd
Copy link
Member

nohwnd commented Jul 14, 2021

MicrosoftTeams-image (3)

This is how it looks like in my toy project just calling new ProjectCollection();

@rainersigwald
Copy link
Member

Ah, interesting! We could probably avoid that by reading the configuration directly as XML rather than using System.Configuration; I don't think we really care about the extra stuff that provides over straight XML (other than "find the location of MSBuild.exe.config" which is doable).

@nohwnd
Copy link
Member

nohwnd commented Jul 14, 2021

Is that worth doing if you will cache it per process?

But I don't see any other way to avoid it. The OpenMappedExeConfiguration offers user level, None is provided, and that load machine.config, the others just try to resolve other config paths.

MicrosoftTeams-image (2)

@ladipro ladipro added the grab-next The issue is at the top of the backlog and should be worked on soon. label Aug 4, 2021
@rokonec rokonec self-assigned this Aug 12, 2021
AR-May pushed a commit that referenced this issue Sep 14, 2021
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.
@ladipro ladipro modified the milestones: Backlog, VS 17.0 Dec 8, 2021
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grab-next The issue is at the top of the backlog and should be worked on soon. performance triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants