Add reference metadata to indicate that RAR can skip dependency searching

[nguerrera]

MSBuild half: dotnet/msbuild#2716

On .NET Core, I'm seeing 2.5-3X improvement in RAR and about 0.5 seconds shaved off of full and incremental builds.

Before (dotnet new mvc - first build)
       68 ms  ResolvePackageFileConflicts                1 calls
       81 ms  ResolvePackageDependencies                 1 calls
      101 ms  GenerateDepsFile                           1 calls
      168 ms  MsBuild                                   10 calls
      724 ms  ResolveAssemblyReference                   1 calls
      795 ms  Csc                                        1 calls
     1125 ms  RestoreTask                                1 calls

     Time Elapsed 00:00:03.80

Before (dotnet new mvc - no changes):
       70 ms  ResolvePackageFileConflicts                1 calls
       95 ms  ResolvePackageDependencies                 1 calls
      167 ms  MsBuild                                   10 calls
      172 ms  RestoreTask                                1 calls
      729 ms  ResolveAssemblyReference                   1 calls

      Time Elapsed 00:00:01.88


After (dotnet new mvc - first build):
       75 ms  ResolvePackageFileConflicts                1 calls
       98 ms  GenerateDepsFile                           1 calls
       98 ms  ResolvePackageDependencies                 1 calls
      173 ms  MsBuild                                   10 calls
      260 ms  ResolveAssemblyReference                   1 calls
      810 ms  Csc                                        1 calls
     1092 ms  RestoreTask                                1 calls

     Time Elapsed 00:00:03.16

After (dotnet new mvc - no changes):
       76 ms  ResolvePackageFileConflicts                1 calls
      105 ms  ResolvePackageDependencies                 1 calls
      183 ms  MsBuild                                   10 calls
      187 ms  RestoreTask                                1 calls
      287 ms  ResolveAssemblyReference                   1 calls

      Time Elapsed 00:00:01.40

[nguerrera]

@livarcocc @davidfowl @davkean @cdmihai @dsplaisted @Pilchie

[davidfowl]

Do we want FindDependencies to mean skip everything (files and assembly deps) or do we want more granular flags? I think this is fine TBH.

[davidfowl]

There's one piece of code in RAR that I'm worried about with this change https://github.com/Microsoft/msbuild/blob/cb6c2643fe4a072f28e685bc46df3bace17a7108/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs#L2305-L2306

[nguerrera]

Ya, that's worrisome. :(

[cdmihai]

So, under what circumstance would System.Runtime or netstandard not be found? The search is being done on dependencyTable.Reference.Keys, which contains the flat list of all references. I assume the nuget graph includes those two, right? _ComputeLockFileReferences seems to add both for a netcoreapp.

[nguerrera]

It's definitely broken (and thankfully we have tests that catch it).

The problem is when netfx project pulls in refs from nuget that depend on System.Runtime, but do not pull in System.Runtime from nuget. The expectation is that RAR will still detect the reference to System.Runtime and then ImplicitlyExpandDesignTimeFacades will use that detection to add TargetFrameworkDir\Facades\*.dll to ReferencePath.

Here is a minimal repro that is broken with the current iteration:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net46</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="System.Collections.Immutable" Version="1.4.0" />
  </ItemGroup>
</Project>

class P {
    static void Main() {
        System.Collections.Immutable.ImmutableList<string>
            .Empty
            .Add("hello");
    }
}

Program.cs(5,14): error CS0012: The type 'Object' is defined in an assembly that is not referenced. 
You must add a reference to assembly 'System.Runtime, Version=4.0.0.0, Culture=neutral, 
PublicKeyToken=b03f5f7f11d50a3a'. [D:\Temp\repro3\repro3.csproj]

[cdmihai]

Mkay, so the assumption that Nuget is responsible with resolving the closure of dependencies is actually valid only when the nupkg dependency graph matches the underlying assembly dependency graph (for each assembly edge there is a nupkg edge, or something like that). When is this guaranteed? Is it always guaranteed for netcore and netstandard?

Either way, this means that metadata to skip dependencies should only be added when the nuget graph is guaranteed to match the assembly graph. No idea when this holds. Only on specific tfms? Maybe the tfm compatibility matrix should also store information whether the tfm may depend on facades.

Does this mean we can't do the optimization?

[davidfowl]

It's not quite that rigid @cdmihai I think we just need to also set the UsingSystemRuntime or UsingNetstandard as a result certain packages being referenced in the assets file. @nguerrera is that what you were thinking?

This case is actually not as bad as AutoGenerateBindingRedirects where I'm finding that we do need to do the entire dependency walk (but can skip search for .pdb, .xml, serialization assemblies).

Ugh, wuttttt. Why do we need to expand the graph for binding redirects? Are we missing assemblies?

[nguerrera]

Think of a unified assembly graph having edges labeled with the version of the assembly reference. We're not missing any nodes in that graph, but we're missing edges.

Binding redirects are needed when there are incoming edges at multiple versions.

We could consider brute force redirecting every reference or discovering the edges more cheaply than the current dependency walk: we only need to enumerate references but not probe for them. I'm inclined to tackle that in a second change. The approach for now is to leave the dependency walk enabled when generating binding redirects.

As for detecting UsingSystemRuntime/UsingNetStandard from packages in the graph, I'm not sure that's possible in the general case. Today, it would work to put an assembly referencing system.runtime in a net45 package folder and not have a package dependency on anything.

So what I'm doing instead is to use the same fallback as the design time build to still do a shallow search. We can maybe do something better but again I want to proceed in steps.

[cdmihai]

To summarize: RAR extracts data necessary for binding redirection from the closure. If nugets are skipped, then it does not have that data for nuget's closure.

• it would be nice if binding redirection were a separate concern, done by another task. Worst case it would be a lot slower, because it will have to do the closure walk again. Best case, for .netcore, it wouldn't be that much slower because much of the closure walk would be skipped in RAR. Only the walks from p2ps and non-nuget references would be redone. Or maybe msbuild could provide some shared memory model for tasks, so they can reuse in-memory caches and whatnot.
• could nuget provide this data as even yet more metadata? For each reference, the metadata could be a list of all incoming version edges. Then, RAR can unify that with whatever it finds from the non-nuget closures. But I guess this is impossible because the nuget graph is not the same as the assembly graph. At best they are the same only by convention / accident :(

It would have been really nice if nuget had a different architecture, where each language could implement its own nuget graph extension of what it means to be a valid graph, and what conflict resolution means. Then, the integration with RAR could have been more tight.

[davidfowl]

Binding redirect computation doesn't require a graph walk at all though. I get that RAR might use that information but it just requires the direct references (the edges) and all of the selected versions. The shallow check that design time builds do for netstandard and System.runtime might be enough to do this.

[nguerrera]

Current status:

1. If auto-generate-binding redirects: don't skip find dependencies, but do skip find .xml, .pdb, serialization/satellites assembly.
2. Borrow from design-time code to fix facade injection approach

I think we can do better than this, but I'd like to focus on testing from this point on for this change.

[nguerrera]

Do we want FindDependencies to mean skip everything (files and assembly deps) or do we want more granular flags? I think this is fine TBH

I thought about this a bit, and my thinking is we can add granular flags later if we end up needing them. Maybe FindDependencies isn't the best name for the uber flag. I am open to other names. Our reference items are already bloated with too much metadata and it costs allocations and log clutter so I don't want to set like five flags to false in the common case.

[nguerrera]

After getting derailed by another regression (#1730), I am now able to run all SDK tests with an updated msbuild containing the other half of this change. There are many failures around binding redirect generation and .NET Framework -> .NET Standard. I will be working through those.

[Pilchie]

I've never looked at the SDK targets before, so I'm curious - how does this change affect the .NET Standard facades that are injected into non-SDK framework projects that reference a .NET Standard library?

[cdmihai]

Shouldn't this also check for the PackageName metadata?

[nguerrera]

No, this is specifically for the netstandard libs that only include NuGetPackageId.

[nguerrera]

NETStandard.Library is special because it adds references in targets files rather than being raised from the lock file in the usual way:
https://github.com/dotnet/standard/blob/9a99f024343e6a36d15e7073890c07daa81606fc/netstandard/pkg/targets/netstandard/NETStandard.Library.targets#L16-L17

For the regular case, we set FindDependencies here: https://github.com/dotnet/sdk/pull/1725/files#diff-4a26943c6456c3afd827c7b87e69d390R455 and there is no coupling to other metadata.

[nguerrera]

I've never looked at the SDK targets before, so I'm curious - how does this change affect the .NET Standard facades that are injected into non-SDK framework projects that reference a .NET Standard library?

See the changes to Microsoft.NET.Build.Extensions.targets https://github.com/dotnet/sdk/pull/1725/files#diff-037688beff60348f7a74e33446cb6131

That gets packaged up as a separate component and inserted into VS and as a .msi for downlevel VS.

[nguerrera]

This is ready for review

[dasMulli]

I'm assuming this would break @jnm2's workaround for #1458 (comment) needing it to be extended to set MarkPackageReferencesAsExternallyResolved is set to false?

[jnm2]

Thanks for the heads up 😆

[nguerrera]

I'm assuming this would break @jnm2's workaround for #1458 (comment) needing it to be extended to set MarkPackageReferencesAsExternallyResolved is set to false?

That is correct. RAR is not actually the right place to find PDBs because it sees only compile-time assets and pdbs are runtime assets. This means the workaround would fail with a ref/lib split. My hope is that we can fix #1458 in the same release as this.

[davidfowl]

Why the new DependsOnTargets?

[nguerrera]

Separate commit for separate issue: 83912b8

[nguerrera]

@livarcocc PTAL

[davkean]

Thanks @nguerrera for pushing this through!

[mungojam]

I'm a bit concerned that this is showing against 2.1.3 which will break the workaround for #1458 (comment), while the fix for that is scheduled for 2.1.4.

Please could somebody with an understanding of the changes can provide an alternative workaround if there is going to be a period of time between the releases

[nguerrera]

@mungojam Yes, I will provide an alternative workaround shortly in the discussion on #1458.

[nguerrera]

See #1458 (comment) for the new workaround.