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

Several changes to reduce allocations #11273

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Erarndt
Copy link
Contributor

@Erarndt Erarndt commented Jan 13, 2025

Fixes #

Context

There is an appreciable amount of time all of the MSBuild process nodes spend doing GC during a build. Taking steps to reduce allocations. This PR contains a mix of approaches that reduce allocations in several areas.

Changes Made

Testing

Notes

@JanKrivanek
Copy link
Member

Related to #11160

@JanKrivanek JanKrivanek self-requested a review January 15, 2025 17:34
@JanKrivanek JanKrivanek marked this pull request as ready for review January 28, 2025 11:29
@SimaTian
Copy link
Member

Is the plan to split this PR into several smaller ones still in place please?

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

I went through the whole changese and do not have any strong concerns, but it's a bigger PR - so I want one more more detailed look before I sign-off

@@ -522,7 +520,7 @@ private void RegisterResolversManifests(ElementLocation location)
// The collections are never modified after this point.
// So I've made them ReadOnly
_specificResolversManifestsRegistry = specificResolversManifestsRegistry.AsReadOnly();
Copy link
Member

Choose a reason for hiding this comment

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

Why this AsReadOnly() is left and next one is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the other one as well.

@AR-May
Copy link
Member

AR-May commented Feb 26, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Erarndt
Copy link
Contributor Author

Erarndt commented Feb 26, 2025

Is the plan to split this PR into several smaller ones still in place please?

Curious about thoughts on splitting this PR. I can split it up primarily based on file if that's desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants