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

Read PackageDefinitions and PackageDependencies from the cache assets file instead of ResolvePackageDependencies #28263

Closed
wants to merge 5 commits into from

Conversation

ocallesp
Copy link
Contributor

Fixes #27738

We want to avoid reading the assets file because it is a large file, which can cause performance degradation in design-time builds for large projects when executing ResolvePackageDependencies.

To improve this, the assets cache file in ResolvePackageAssets will now keep track of PackageDefinitions and PackageDependencies.

The change:
Output PackageDefinitions and PackageDependencies items from ResolvePackageAssets, and modified the targets so that ResolvePackageDependencies is only called when EmitLegacyAssetsFileItems is true.

All the code related to EmitLegacyAssetsFileItems in ResolvePackageDependencies was removed since there is case for when the value is false.
Unit-tests were modified to remove EmitLegacyAssetsFileItems in ResolvePackageDependencies.

Comment on lines +1498 to +1503
if (messages.Count() == 0)
{
return string.Empty;
}

return messages.Max(log => log.Level).ToString();
Copy link
Member

Choose a reason for hiding this comment

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

This is double-enumerating messages. Use a foreach here to ensure items are only enumerated once.

@dsplaisted
Copy link
Member

dsplaisted commented Oct 6, 2022

@ocallesp I started diving into reviewing this, which led me to investigate in more detail how some of this data is used. Based on that I'd like to try changing how we handle this.

We are trying to improve the perf of the design-time builds. I believe the design-time build does not depend directly on the PackageDefinitions and PackageDependencies items, but rather depends on the items returned from the ResolvePackageDependenciesDesignTime target, which passes the PackageDefinitions and PackageDependencies items to the PreprocessPackageDependenciesDesignTime task which then creates the items which will be returned.

The PackageDefinitions and PackageDependencies items include information about packages for all target frameworks, and include all the transitive package dependencies too. However, the PreprocessPackageDependenciesDesignTime task filters out all of the data that is for a different TargetFramework or that is a transitive (not direct) dependency of the project.

So instead of generating PackageDefinitions and PackageDependencies items in ResolvePackageAssets, I think what we can do is just to generate the same items that PreprocessPackageDependenciesDesignTime currently generates. That could potentially cut down significantly on the amount of data that ResolvePackageAssets generates and caches.

If we do this, PackageDefinitions and PackageDependencies would still be generated by the ResolvePackageDependencies task, but it would only run if EmitLegacyAssetsFileItems is set to true.

@drewnoakes Does this sound like it would work? Is it correct that the project system only depends on the items returned by the ResolvePackageDependenciesDesignTime target?

@dsplaisted
Copy link
Member

Also, we'd like to target this feature for release/7.0.2xx, and we now have a branch for that.

@drewnoakes
Copy link
Member

@dsplaisted that's my understanding, yes. Sounds like a nice optimisation for design-time builds.

@ocallesp
Copy link
Contributor Author

ocallesp commented Oct 7, 2022

The suggested approach is different from the original one.

This pr #28405 implements the new approach.

@ocallesp ocallesp closed this Oct 7, 2022
@akoeplinger akoeplinger deleted the fix-github27738 branch February 14, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow design-time builds for large solution
4 participants