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

Allow optional reuse of existing immutable collections when constructing a ProjectInstance. #9374

Merged

Conversation

sgreenmsft
Copy link
Contributor

@sgreenmsft sgreenmsft commented Oct 31, 2023

ProjectInstance is a snapshot of the Project values that duplicates the project data in "Instance" objects (e.g. ProjectPropertyInstance). In the special case of the ProjectInstance constructed from the CPS Evaluation Cache data, this duplication is unnecessary, as the original data is guaranteed to be immutable. This PR introduces the ProjectInstance.FromImmutableProjectSource method, which CPS will use when it wants ProjectInstance to reuse its immutable structures.

One complicating factor was a concern about taking a dependency on any particular version of MSBuild, as CPS ships in C# Dev Kit and in that context it must play nice with the installed version.

@rainersigwald rainersigwald requested a review from rokonec November 1, 2023 16:04
@AR-May AR-May self-requested a review November 14, 2023 14:35
@rokonec rokonec requested a review from ladipro November 20, 2023 14:37
Copy link
Member

@rokonec rokonec left a comment

Choose a reason for hiding this comment

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

Looks correct. However, it is hard for me to understand its full context as I don't known how exactly it will be usable for CPS.

However, it is lot of changes, as there is no unit teste coverage for those, how did you verified it?

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

I agree with @rokonec that it would be useful to describe the intended usage patterns.

Also, seeing several concrete types converted to interfaces, it would be good to verify that there are no perf regressions as interface calls are generally slower and on some of the hot evaluation code paths it may be measurable. I can help with the measurements.

@rokonec rokonec added the needs-attention Needs a member of the team to respond. label Dec 6, 2023
@sgreenmsft
Copy link
Contributor Author

@ladipro @rokonec - Thanks for the feedback! I've made changes as per your suggestions. When convenient, could you take another look? Thanks!

@sgreenmsft
Copy link
Contributor Author

sgreenmsft commented Jan 2, 2024

Looks correct. However, it is hard for me to understand its full context as I don't known how exactly it will be usable for CPS.

However, it is lot of changes, as there is no unit teste coverage for those, how did you verified it?

@rokonec - CPS tests and manual verification of impacted scenarios. If there are additional steps I can take to verify please let me know. :)

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@JanKrivanek
Copy link
Member

@sgreenmsft - can you please resolve comments that were reflected or doesn't apply anymore? That will make the second look easier. Plus please resolve the conflicts with main.

@rokonec - can you please have a second look if your concerns were addressed?

@JanKrivanek JanKrivanek enabled auto-merge (squash) January 3, 2024 14:02
@rokonec
Copy link
Member

rokonec commented Jan 4, 2024

Seems to be OK.

@JanKrivanek JanKrivanek merged commit 2a789cd into dotnet:main Jan 4, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-attention Needs a member of the team to respond.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants