Skip to content

Add PackageDependenciesDesignTime to cache assets file#28405

Merged
dsplaisted merged 3 commits intorelease/7.0.2xxfrom
github-27738
Nov 30, 2022
Merged

Add PackageDependenciesDesignTime to cache assets file#28405
dsplaisted merged 3 commits intorelease/7.0.2xxfrom
github-27738

Conversation

@ocallesp
Copy link
Contributor

@ocallesp ocallesp commented Oct 7, 2022

Fixes #27738

Implements reading the PackageDefinitions and PackageDependencies items in ResolvePackageAssets, so that ResolvePackageDependencies doesn't need to be called at all unless EmitLegacyAssetsFileItems is true. This improve incremental build perf and also significantly improve the design time build perf (23%) for this large solution scenario by saving to the cache file the same items generated from PreprocessPackageDependenciesDesignTime.


Note:
This implementation was taken from PreprocessPackageDependenciesDesignTime.ExecuteCore

@ocallesp ocallesp requested review from a team, ericstj and joperezr as code owners October 7, 2022 20:39
@ocallesp ocallesp changed the base branch from main to release/7.0.2xx October 7, 2022 20:40
@ocallesp ocallesp requested a review from dsplaisted October 7, 2022 20:40
@ocallesp ocallesp changed the title GitHub 27738 Add PackageDependenciesDesignTime to cache assets file Oct 7, 2022
@dotnet dotnet deleted a comment Oct 7, 2022
@ocallesp ocallesp force-pushed the github-27738 branch 2 times, most recently from 0f4c1e7 to d383ef0 Compare October 7, 2022 21:00
@ocallesp ocallesp marked this pull request as draft October 7, 2022 21:55
@ocallesp ocallesp force-pushed the github-27738 branch 4 times, most recently from 8a2df1e to 2bb5447 Compare October 8, 2022 00:31
@ocallesp ocallesp marked this pull request as ready for review October 8, 2022 01:01
@ocallesp ocallesp requested a review from dsplaisted October 8, 2022 01:01
Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

See comments for ways we should be able to eliminate a bunch of the code here and process less data.

@build-analysis build-analysis bot mentioned this pull request Oct 12, 2022
2 tasks
@ocallesp ocallesp force-pushed the github-27738 branch 3 times, most recently from 4cbdba9 to 3ea4ca2 Compare October 12, 2022 15:57
@ocallesp ocallesp requested a review from dsplaisted October 12, 2022 16:25
Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks.

Have you made any progress with @drewnoakes on running the project system tests with these changes?

Also do you have any perf testing results? It would be great to be able to present those (or a demo) at the next team showcase.

@ocallesp ocallesp requested review from dsplaisted and removed request for a team, TanayParikh, captainsafia, ericstj, javiercn, joperezr and mkArtakMSFT November 16, 2022 18:55
@ocallesp ocallesp force-pushed the github-27738 branch 8 times, most recently from a78a033 to 8ad26ff Compare November 18, 2022 21:40
Comment on lines +21 to +22
string projectAssetsJsonPath = Path.GetTempFileName();
string projectCacheAssetsJsonPath = Path.GetTempFileName();
Copy link
Member

@drewnoakes drewnoakes Nov 21, 2022

Choose a reason for hiding this comment

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

The test should delete both of these files when it completes, to avoid polluting the temp dir.

The same is true for other tests below.

@dsplaisted dsplaisted merged commit 866c1ac into release/7.0.2xx Nov 30, 2022
@ocallesp
Copy link
Contributor Author

@marcin-krystianc I see that the repository https://github.com/marcin-krystianc/TestSolutions/tree/master/LargeAppWithPrivatePackagesCentralisedNGBVRemoved does not contain any readme file nor a license.

We would like to know if we are allowed to share this with folks.

@marcin-krystianc
Copy link
Contributor

@marcin-krystianc I see that the repository https://github.com/marcin-krystianc/TestSolutions/tree/master/LargeAppWithPrivatePackagesCentralisedNGBVRemoved does not contain any readme file nor a license.

We would like to know if we are allowed to share this with folks.

Hi @ocallesp, I'll add some readme and license information, but of course it is ok to share it with others.

@ocallesp
Copy link
Contributor Author

@marcin-krystianc I appreciate your willingness to share your repository with others.

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.

5 participants