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

Reapply Support lazy instantiation of ProjectInstance values when CPS Evaluation cache is used #10350

Merged
merged 6 commits into from
Jul 29, 2024

Conversation

sgreenmsft
Copy link
Contributor

@sgreenmsft sgreenmsft commented Jul 10, 2024

Fixes AB#2067934

Context

A previous refactoring for performance optimization introduced a regression in memory usage in vcxprojreader (AB#2067934). Consequently, the new code was reverted until a fix was available. This PR contains the fix and reinstates the performance optimization code.

Changes Made

  • Reapplies commit eade720
  • Modifications to avoid regression of additional memory usage in vcxprojreader test scenario (specifically in the handling of enumeration to avoid enumerator allocation).

Testing

Manual verification of the impacted scenario.

@YuliiaKovalova
Copy link
Member

experimental insertion for checking the performance: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/563920

@JanKrivanek
Copy link
Member

@JanKrivanek
Copy link
Member

And yet another exp insertion: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/567802 (as there were couple failed perf tests in previous that seems as noise to me - comparing to variance of previous main builds)

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'm fine with the change:

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Fine by me if insertions are in good shape so we can isolate any regression this does cause.

@JanKrivanek JanKrivanek merged commit 491b0df into dotnet:main Jul 29, 2024
10 checks passed
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.

4 participants