Allow references to specify metadata to skip finding dependencies in RAR #2716

nguerrera · 2017-11-11T05:15:03Z

SDK half: dotnet/sdk#1725

See there for perf numbers

nguerrera · 2017-11-11T05:31:23Z

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

livarcocc

Test coming?

cdmihai · 2017-11-14T01:22:42Z

src/Tasks/AssemblyDependency/ReferenceTable.cs

+        // Targets that raise references from NuGet packages set FindDependencies=false to skip looking
+        // for dependencies that they have already resolved and related files that they are responsible
+        // for deploying.
+        private static bool ShouldFindDependencies(Reference reference)


Should this be in Reference itself?

I was wondering if a more general name is suited. I want to experiment with this to turn off other RAR behaviour.

Which other behaviours that are per-reference?

When resolving a reference, RAR first looks near all the parents of that reference. If it finds nothing, then it starts asking the resolvers that were actually specified to RAR. I think this could be skipped when the reference is a nuget reference. For example, ProjA -> ProjB ->Nupkg. It's not worth looking for the Nupkg alongside ProjA or ProjB, and skip directly to the actual RAR resolvers. Though even in this case, the FindDependencies name is still valid.

@cdmihai I like that optimization

cdmihai · 2017-11-28T01:53:13Z

src/Tasks/AssemblyDependency/Reference.cs

+            get
+            {
+                string findDependencies = _primarySourceItem?.GetMetadata("FindDependencies");
+                return string.IsNullOrEmpty(findDependencies) ? true : findDependencies.Equals("true", StringComparison.OrdinalIgnoreCase);


Can you please cache this value? It traverses the metadata name, may new up a StringBuilder and other small things. Probably not a big gain though caching it.

nguerrera · 2017-11-28T18:57:34Z

Code is ready for review, but still working on unit test

cdmihai · 2017-11-28T22:13:54Z

src/Tasks/AssemblyDependency/ReferenceTable.cs

                            {
-                                FindDependenciesAndScatterFiles(reference, newEntries);
+                                SkippedFindingExternallyResolvedDependencies = true;


ComputeClosure clears out _references. This flag probably needs to be reset as well right?

cdmihai · 2017-11-28T22:17:41Z

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

@@ -2146,6 +2160,8 @@ ReadMachineTypeFromPEHeader readMachineTypeFromPEHeader
                        assemblyMetadataCache
                        );

+                    dependencyTable.FindDependenciesOfExternallyResolvedReferences = FindDependenciesOfExternallyResolvedReferences;


Quite the large constructor parameter list right? :)

Yeah. Are you OK with this deviation?

Yes, just for the sake of less churn. The large number of arguments seems bad enough and shadows this deviation :)

nguerrera · 2017-11-29T19:11:30Z

@cdmihai Tests are added

AndyGerlicher · 2017-11-29T19:43:34Z

src/Tasks/AssemblyDependency/Reference.cs

@@ -1107,6 +1117,9 @@ string executableExtension
            // This is an assembly file, so we'll need to find dependencies later.
            DependenciesFound = false;

+            string externallyResolved = sourceItem.GetMetadata("ExternallyResolved");
+            ExternallyResolved = string.Equals(externallyResolved, "true", StringComparison.OrdinalIgnoreCase);
+


Nit: Can you use:
ExternallyResolved = MetadataConversionUtilities.TryConvertItemMetadataToBool(sourceItem, "ExternallyResolved")

Not used everywhere, but it already is elsewhere in the file so we should try to be consistent.

That looks like it would be an order of magnitude more expensive, but once per reference is probably ok. I'll do it for consistency.

AndyGerlicher

Looks good!

cdmihai · 2017-11-29T22:34:53Z

src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs

+        /// <summary>
+        /// Externally resolved references do not get their related files identified by RAR. In the common
+        /// nuget assets case, RAR cannot be the one to identify what to copy because RAR sees only the
+        /// compile-time assets and not the 


Sentence ends in cliffhanger :)

cdmihai · 2017-11-30T02:00:00Z

Perf looks good.

Build: Time (ms)

Test	Overall	Significant δ	Value
DotnetConsoleProject	✅	yes	23.1327 -> 22.8537 (-1.206%)
DotnetWebProject	👌	no	28.3711 -> 28.4794 (0.382%)
DotnetMvcProject	👌	no	30.8593 -> 30.951 (0.297%)
Picasso	✅	yes	4520.3862 -> 4483.849 (-0.808%)
SmallP2POldCsproj	🔴	yes	196.7781 -> 198.5418 (0.896%)
SmallP2PNewCsproj	👌	no	174.3653 -> 174.5162 (0.087%)

Build: Memory (bytes)

Test	Overall	Significant δ	Value
DotnetConsoleProject	::ok_hand:	no	1166874 -> 1166873 (0%)
DotnetWebProject	✅	yes	1312718 -> 1312137 (-0.044%)
DotnetMvcProject	::ok_hand:	no	1485719 -> 1485589 (-0.009%)
Picasso	👌	no	193911965 -> 195004016 (0.563%)
SmallP2POldCsproj	🔴	yes	8179031 -> 8196331 (0.212%)
SmallP2PNewCsproj	::ok_hand:	no	7268575 -> 7264851 (-0.051%)

Includes dotnet/msbuild#2716

dnfclas added the cla-already-signed label Nov 11, 2017

nguerrera mentioned this pull request Nov 11, 2017

Add reference metadata to indicate that RAR can skip dependency searching dotnet/sdk#1725

Merged

livarcocc approved these changes Nov 11, 2017

View reviewed changes

cdmihai reviewed Nov 14, 2017

View reviewed changes

cdmihai reviewed Nov 28, 2017

View reviewed changes

nguerrera force-pushed the speed-up-rar branch 2 times, most recently from 0321193 to 90feae2 Compare November 28, 2017 02:25

Allow references to specify metadata to skip finding dependencies in RAR

9da1390

nguerrera force-pushed the speed-up-rar branch from 90feae2 to 9da1390 Compare November 28, 2017 02:40

nguerrera requested review from cdmihai and AndyGerlicher November 28, 2017 02:41

nguerrera changed the title ~~WIP Allow references to specify metadata to skip finding dependencies in RAR~~ Allow references to specify metadata to skip finding dependencies in RAR Nov 28, 2017

cdmihai reviewed Nov 28, 2017

View reviewed changes

cdmihai mentioned this pull request Nov 29, 2017

Skip dependee RAR resolvers for externally resolved references #2759

Merged

nguerrera added 2 commits November 29, 2017 11:05

Clear flag when clearing references

ae71364

Add tests for externally resolved reference handling

7330f52

AndyGerlicher reviewed Nov 29, 2017

View reviewed changes

AndyGerlicher approved these changes Nov 29, 2017

View reviewed changes

Use MetadataConversionUtilities.TryConvertItemMetadataToBool

e1ee474

cdmihai reviewed Nov 29, 2017

View reviewed changes

cdmihai approved these changes Nov 29, 2017

View reviewed changes

Complete sentence

479a5d2

cdmihai merged commit 516bf1f into dotnet:master Nov 30, 2017

cdmihai added a commit to dotnet/cli that referenced this pull request Dec 4, 2017

Bump MSbuild to 15.6.0-preview-000012-1193158

49a1942

Includes dotnet/msbuild#2716

cdmihai mentioned this pull request Dec 4, 2017

Bump MSbuild to 15.6.0-preview-000012-1193158 dotnet/cli#8161

Merged

nguerrera pushed a commit to dotnet/cli that referenced this pull request Dec 4, 2017

Bump MSbuild to 15.6.0-preview-000012-1193158 (#8161)

ea74bf1

Includes dotnet/msbuild#2716

nguerrera deleted the speed-up-rar branch March 7, 2018 19:24

nguerrera mentioned this pull request Nov 30, 2018

ResolveAssemblyReference is slow on .NET Core with many references #2015

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow references to specify metadata to skip finding dependencies in RAR #2716

Allow references to specify metadata to skip finding dependencies in RAR #2716

nguerrera commented Nov 11, 2017 •

edited

Loading

nguerrera commented Nov 11, 2017

livarcocc left a comment

cdmihai Nov 14, 2017

nguerrera Nov 14, 2017 •

edited

Loading

cdmihai Nov 15, 2017

davidfowl Nov 15, 2017

cdmihai Nov 28, 2017 •

edited

Loading

nguerrera commented Nov 28, 2017

cdmihai Nov 28, 2017

nguerrera Nov 29, 2017

cdmihai Nov 28, 2017

nguerrera Nov 28, 2017

cdmihai Nov 29, 2017

nguerrera commented Nov 29, 2017

AndyGerlicher Nov 29, 2017

nguerrera Nov 29, 2017

AndyGerlicher left a comment

cdmihai Nov 29, 2017 •

edited

Loading

nguerrera Nov 29, 2017

cdmihai commented Nov 30, 2017

Allow references to specify metadata to skip finding dependencies in RAR #2716

Allow references to specify metadata to skip finding dependencies in RAR #2716

Conversation

nguerrera commented Nov 11, 2017 • edited Loading

nguerrera commented Nov 11, 2017

livarcocc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

nguerrera commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyGerlicher left a comment

Choose a reason for hiding this comment

cdmihai Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai commented Nov 30, 2017

Build: Time (ms)

Build: Memory (bytes)

nguerrera commented Nov 11, 2017 •

edited

Loading

nguerrera Nov 14, 2017 •

edited

Loading

cdmihai Nov 28, 2017 •

edited

Loading

cdmihai Nov 29, 2017 •

edited

Loading