Use ReadOnlyMemory when calculating the related file extensions #5943

nkolev92 · 2024-07-31T20:45:28Z

Bug

Description

This gets us about 4MB on OrchardCore.
We just use span instead of optimistically using substring.
Note that this calculation is cached on a per package level already.

This probably isn't perfect, but it's a start.

Before:

Name                                                                                                                                    	Inc %	          Inc
 nuget.packaging!ContentItemCollection.GetRelatedFileExtensionProperty                                                                  	  0.1	   11,851,736
+ mscorlib.ni!System.String.Substring(Int32, Int32)                                                                                     	  0.1	    7,679,344
+ nuget.packaging!NuGet.NoAllocEnumerateExtensions+OptimisticallyNonAllocatingEnumerable`1[System.__Canon].GetEnumerator()              	  0.0	    1,803,320
+ nuget.packaging!NuGet.NoAllocEnumerateExtensions+OptimisticallyNonAllocatingEnumerable`1+Enumerator[System.__Canon].MoveNext()        	  0.0	    1,100,856
+ mscorlib.ni!System.Collections.Concurrent.ConcurrentDictionary`2[System.__Canon,System.__Canon].TryAdd(System.__Canon, System.__Canon)	  0.0	    1,052,960
+ mscorlib.ni!String.Join                                                                                                               	  0.0	      108,760
+ clr!JIT_New                                                                                                                           	  0.0	      106,496

After:

Name                                                                                                                            	Inc %	         Inc
 nuget.packaging!ContentItemCollection.GetRelatedFileExtensionProperty                                                          	  0.1	   8,327,688
+ mscorlib!System.Collections.Generic.Dictionary`2[System.ReadOnlyMemory`1[System.Char],System.__Canon].set_Item(!0,!1)         	  0.0	   3,842,840
+ mscorlib!System.Collections.Generic.Dictionary`2[System.ReadOnlyMemory`1[System.Char],System.__Canon].TryGetValue(!0,!1&)     	  0.0	   1,668,760
+ nuget.packaging!NuGet.NoAllocEnumerateExtensions+OptimisticallyNonAllocatingEnumerable`1+Enumerator[System.__Canon].MoveNext()	  0.0	   1,243,928
+ nuget.packaging!NuGet.NoAllocEnumerateExtensions+OptimisticallyNonAllocatingEnumerable`1[System.__Canon].GetEnumerator()      	  0.0	   1,106,464
+ mscorlib.ni!String.Join                                                                                                       	  0.0	     345,640
+ clr!JIT_New                                                                                                                   	  0.0	     120,056

In microbenchmarks, it registers about the same, but we can also notice that it's faster.

Before:

| Method                | Mean          | Error      | StdDev     | Gen0       | Gen1      | Allocated    |
|---------------------- |--------------:|-----------:|-----------:|-----------:|----------:|-------------:|
| NJ                    |      44.10 us |   0.164 us |   0.153 us |     4.3335 |    0.1221 |     73.78 KB |
| MicrosoftBuildRuntime |      66.96 us |   0.285 us |   0.267 us |     8.9111 |    0.7324 |     151.6 KB |
| AllPackages           | 155,316.21 us | 563.189 us | 526.807 us | 15250.0000 | 1750.0000 | 259587.62 KB |

After:

| Method                | Mean          | Error      | StdDev     | Gen0       | Gen1      | Allocated    |
|---------------------- |--------------:|-----------:|-----------:|-----------:|----------:|-------------:|
| NJ                    |      44.74 us |   0.120 us |   0.112 us |     4.2114 |    0.1221 |     71.77 KB |
| MicrosoftBuildRuntime |      66.51 us |   0.223 us |   0.209 us |     8.9111 |    0.7324 |     151.6 KB |
| AllPackages           | 151,262.23 us | 354.896 us | 331.970 us | 15000.0000 | 2000.0000 | 254570.14 KB |

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

jeffkl · 2024-08-01T00:52:39Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

                return relatedFileExtensionsProperty;
            }
+
+            static bool ReadOnlyMemoryEquals(ReadOnlyMemory<char> x, ReadOnlyMemory<char> y)


Is it worth making this an extension method that the whole repo can use?

Sorry, I had merged before this.
I think we could eventually.
You were thinking a build/shared type of method?

akoeplinger · 2024-08-01T13:37:34Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

@@ -50,6 +52,7 @@ public void Load(IEnumerable<string> paths)
            }
        }

+        [Obsolete("Unused and will be removed in a future version.")]


this seems to be used in the dotnet/sdk: dotnet/sdk#42458 (comment)

2 options for you.

Supress the warning

Move to PopulateItemGroups

I'll revert the obsolete attribute for now.

Thanks. I think moving to PopulateItemGroups is the right approach going forward.

See NuGet/NuGet.Client#5943

akoeplinger · 2024-08-02T13:21:30Z

src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs

        private List<Asset> _assets;
-        private ConcurrentDictionary<string, string> _assemblyRelatedExtensions;
+        private Dictionary<ReadOnlyMemory<char>, string> _assemblyRelatedExtensions;


@nkolev92 the SDK PR is now failing with this error in multiple tests, I wonder if it is related to this change:

C:\h\w\AADF0A48\p\d\sdk\9.0.100-ci\NuGet.targets(180,5): error : Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.

Weird, this codepath isn't run concurrently.

nkolev92 added 5 commits July 31, 2024 12:57

Fun with benchmarks

d88a6a3

Cleanup

7311108

Cleanup

c4cafc2

Cleanup

0cd6d8a

bring the code up to date.

579cac7

nkolev92 changed the title ~~Fun with benchmarks~~ Use ReadOnlyMemory when calculating the related file extensions Jul 31, 2024

nkolev92 added 2 commits July 31, 2024 13:57

Cleanup

39b3833

remove benchmark code

de1360f

nkolev92 marked this pull request as ready for review July 31, 2024 21:01

nkolev92 requested a review from a team as a code owner July 31, 2024 21:01

martinrrm approved these changes Jul 31, 2024

View reviewed changes

dtivel approved these changes Jul 31, 2024

View reviewed changes

nkolev92 mentioned this pull request Jul 31, 2024

Reduce allocations in TokenSegment.TryMatch NuGet/Home#12728

Closed

Nigusu-Allehu approved these changes Aug 1, 2024

View reviewed changes

zivkan approved these changes Aug 1, 2024

View reviewed changes

nkolev92 merged commit 3274cb8 into dev Aug 1, 2024
28 checks passed

nkolev92 deleted the dev-nkolev92-TryMatchPlay branch August 1, 2024 00:51

jeffkl approved these changes Aug 1, 2024

View reviewed changes

akoeplinger reviewed Aug 1, 2024

View reviewed changes

akoeplinger added a commit to dotnet/sdk that referenced this pull request Aug 2, 2024

Replace obsolete nuget api in Microsoft.DotNet.PackageValidation

e9bd460

See NuGet/NuGet.Client#5943

akoeplinger mentioned this pull request Aug 2, 2024

Replace obsolete nuget api in Microsoft.DotNet.PackageValidation dotnet/sdk#42508

Merged

akoeplinger reviewed Aug 2, 2024

View reviewed changes

akoeplinger mentioned this pull request Aug 2, 2024

[main] Update dependencies from nuget/nuget.client dotnet/sdk#42458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ReadOnlyMemory when calculating the related file extensions #5943

Use ReadOnlyMemory when calculating the related file extensions #5943

nkolev92 commented Jul 31, 2024 •

edited

Loading

jeffkl Aug 1, 2024

nkolev92 Aug 1, 2024

akoeplinger Aug 1, 2024

nkolev92 Aug 1, 2024

nkolev92 Aug 1, 2024

akoeplinger Aug 1, 2024

akoeplinger Aug 2, 2024

nkolev92 Aug 2, 2024

Use ReadOnlyMemory when calculating the related file extensions #5943

Use ReadOnlyMemory when calculating the related file extensions #5943

Conversation

nkolev92 commented Jul 31, 2024 • edited Loading

Bug

Description

PR Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 commented Jul 31, 2024 •

edited

Loading