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

Static graph construction does not use a shared evaluation context #9678

Closed
rainersigwald opened this issue Jan 24, 2024 · 1 comment · Fixed by #9680
Closed

Static graph construction does not use a shared evaluation context #9678

rainersigwald opened this issue Jan 24, 2024 · 1 comment · Fixed by #9680
Assignees
Labels
Area: Static Graph Issues with -graph, -isolate, and the related APIs. performance Performance-Scenario-General This issue affects performance in general. triaged
Milestone

Comments

@rainersigwald
Copy link
Member

With a tiny API consumer project (reference Microsoft.Build.MSBuildLocator and Microsoft.Build:

using Microsoft.Build.Graph;
using Microsoft.Build.Locator;

namespace GraphBuilderApp
{
    internal class Program
    {
        static void Main(string[] args)
        {
            MSBuildLocator.RegisterDefaults();
            DoMSBuildStuff();
        }

        private static void DoMSBuildStuff()
        {
            ProjectGraph graph = new ProjectGraph(@"C:\src\msbuild\MSBuild.sln");

            System.Console.WriteLine($"Projects in the solution: {graph.ProjectNodes.Count}");
        }
    }
}

Run under tracing and look at events like Microsoft-Build/SdkResolverServiceFindResolversManifests/Stop and Microsoft-Build/SdkResolverResolveSdk/Stop. In my test run (on the MSBuild repo), I see a lot of wasted work:

Event Count Total time
FindResolversManifests 84 597ms
SdkResolverResolveSdk 314 17,328ms

Debugging through, each project evaluated by the graph is in its own EvaluationContext, so they share nothing--including the list of resolvers.

@rainersigwald rainersigwald added performance Area: Static Graph Issues with -graph, -isolate, and the related APIs. Performance-Scenario-General This issue affects performance in general. labels Jan 24, 2024
@rainersigwald rainersigwald added this to the VS 17.10 milestone Jan 24, 2024
@rainersigwald rainersigwald self-assigned this Jan 24, 2024
rainersigwald added a commit to rainersigwald/msbuild that referenced this issue Jan 24, 2024
Fix dotnet#9678 by creating a single shared `EvaluationContext` in the `Graph`
object and using it when creating `ProjectInstance`s using the default
factory.

This is similar to how NuGet static-graph restore already works: https://github.com/NuGet/NuGet.Client/blob/b83566ec2369c4e9fd07e6f95d734dfe370a1e66/src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildStaticGraphRestore.cs#L885

Since we're evaluating the projects in the graph in parallel and "all at
once", the shared caches in the `EvaluationContext` should be a solid
improvement.
@AR-May AR-May added the triaged label Jan 30, 2024
@AndyGerlicher
Copy link
Contributor

Just to add some info here. Here is my before/after fixing this on the client side.

With EvaluationContext.SharingPolicy.Shared:

Static graph loaded in 5.975 seconds: 811 nodes, 4299 edges

With default (EvaluationContext.SharingPolicy.Isolated):

Static graph loaded in 10.701 seconds: 811 nodes, 4299 edges

This is pretty significant! Definitely worth updating the default.

rainersigwald added a commit that referenced this issue Apr 24, 2024
Fix #9678 by creating a single shared `EvaluationContext` in the `Graph`
object and using it when creating `ProjectInstance`s using the default
factory.

This is similar to how NuGet static-graph restore already works: https://github.com/NuGet/NuGet.Client/blob/b83566ec2369c4e9fd07e6f95d734dfe370a1e66/src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildStaticGraphRestore.cs#L885

Since we're evaluating the projects in the graph in parallel and "all at
once", the shared caches in the `EvaluationContext` should be a solid
improvement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Static Graph Issues with -graph, -isolate, and the related APIs. performance Performance-Scenario-General This issue affects performance in general. triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants