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

Reduce allocations in TokenSegment.TryMatch #5933

Merged
merged 3 commits into from
Jul 31, 2024

Conversation

nkolev92
Copy link
Member

Bug

Related: NuGet/Home#12728

Also internal workitem.

Regression? Last working version:

Description

This uses ReadOnlyMemory<char>.Slice() instead of string.Substring() to reduce allocations when pattern matching. This also needs to cache the matches with a Dictionary<ReadOnlyMemory<char>, object> in PatternTable so I needed to re-use an ordinal comparer that I found in another repo. The rest of the changes are fallout from changing the pattern we're looking for from a string to ReadOnlyMemory<char>. The reason I'm using ReadOnlyMemory<T> instead of ReadOnlySpan<T> is that ReadOnlySpan<T> is a readonly ref struct and can't be used as a type parameter.

I had to change the public API of NuGet.ContentModel.ContentPropertyDefinition and NuGet.ContentModel.PatternTable.TryLookup. I'm not sure how widely those are used by anyone so I marked the associated issue as a "breaking change" just to be safe.

This PR:

Name                                                                                                                                                                                                                     	Inc %	          Inc
 nuget.packaging!NuGet.ContentModel.Infrastructure.PatternExpression+TokenSegment.TryMatch(class NuGet.ContentModel.ContentItem&,class System.String,class System.Collections.Generic.IReadOnlyDictionary`2,int32,int32&)	  1.1	   99,485,616
+ mscorlib.ni!System.Collections.Generic.Dictionary`2[System.__Canon,System.__Canon].Insert(System.__Canon, System.__Canon, Boolean)                                                                                     	  0.4	   37,982,336
+ nuget.packaging!ContentPropertyDefinition.TryLookup                                                                                                                                                                    	  0.3	   22,924,920
+ nuget.packaging!NuGet.ContentModel.ContentItem.get_Properties()                                                                                                                                                        	  0.2	   16,821,344
+ system.memory.ni!?                                                                                                                                                                                                     	  0.1	   11,678,544
+ clr!JIT_New                                                                                                                                                                                                            	  0.1	   10,078,472

Name                                                                                                                                                                                                                     	Inc %	           Inc
 nuget.packaging!NuGet.ContentModel.Infrastructure.PatternExpression+TokenSegment.TryMatch(class NuGet.ContentModel.ContentItem&,class System.String,class System.Collections.Generic.IReadOnlyDictionary`2,int32,int32&)	  1.1	   100,373,768
+ mscorlib.ni!System.Collections.Generic.Dictionary`2[System.__Canon,System.__Canon].Insert(System.__Canon, System.__Canon, Boolean)                                                                                     	  0.4	    35,133,768
+ nuget.packaging!NuGet.ContentModel.ContentItem.get_Properties()                                                                                                                                                        	  0.3	    24,102,992
+ nuget.packaging!ContentPropertyDefinition.TryLookup                                                                                                                                                                    	  0.2	    21,077,912
+ system.memory.ni!?                                                                                                                                                                                                     	  0.1	    10,791,424
+ clr!JIT_New                                                                                                                                                                                                            	  0.1	     9,267,672

Name                                                                                                                                                                                                                     	Inc %	          Inc
 nuget.packaging!NuGet.ContentModel.Infrastructure.PatternExpression+TokenSegment.TryMatch(class NuGet.ContentModel.ContentItem&,class System.String,class System.Collections.Generic.IReadOnlyDictionary`2,int32,int32&)	  1.0	   93,417,816
+ mscorlib.ni!System.Collections.Generic.Dictionary`2[System.__Canon,System.__Canon].Insert(System.__Canon, System.__Canon, Boolean)                                                                                     	  0.4	   32,911,760
+ nuget.packaging!NuGet.ContentModel.ContentItem.get_Properties()                                                                                                                                                        	  0.2	   20,449,768
+ nuget.packaging!ContentPropertyDefinition.TryLookup                                                                                                                                                                    	  0.2	   19,134,128
+ system.memory.ni!?                                                                                                                                                                                                     	  0.1	   11,734,512
+ clr!JIT_New                                                                                                                                                                                                            	  0.1	    9,187,648

17.11 P3

Name                                                                                                                                                                                                                     	Inc %	           Inc
 nuget.packaging!NuGet.ContentModel.Infrastructure.PatternExpression+TokenSegment.TryMatch(class NuGet.ContentModel.ContentItem&,class System.String,class System.Collections.Generic.IReadOnlyDictionary`2,int32,int32&)	  1.3	   110,683,792
+ mscorlib.ni!System.String.Substring(Int32, Int32)                                                                                                                                                                      	  0.4	    37,942,624
+ mscorlib.ni!System.Collections.Generic.Dictionary`2[System.__Canon,System.__Canon].Insert(System.__Canon, System.__Canon, Boolean)                                                                                     	  0.4	    35,833,368
+ clr!JIT_New                                                                                                                                                                                                            	  0.4	    32,210,392
+ nuget.packaging!ContentPropertyDefinition.TryLookup                                                                                                                                                                    	  0.1	     4,697,408

Name                                                                                                                                                                                                                     	Inc %	           Inc
 nuget.packaging!NuGet.ContentModel.Infrastructure.PatternExpression+TokenSegment.TryMatch(class NuGet.ContentModel.ContentItem&,class System.String,class System.Collections.Generic.IReadOnlyDictionary`2,int32,int32&)	  1.1	   100,833,848
+ mscorlib.ni!System.String.Substring(Int32, Int32)                                                                                                                                                                      	  0.4	    35,158,928
+ mscorlib.ni!System.Collections.Generic.Dictionary`2[System.__Canon,System.__Canon].Insert(System.__Canon, System.__Canon, Boolean)                                                                                     	  0.4	    33,438,144
+ clr!JIT_New                                                                                                                                                                                                            	  0.3	    25,918,536
+ nuget.packaging!ContentPropertyDefinition.TryLookup                                                                                                                                                                    	  0.1	     6,318,240

Name                                                                                                                                                                                                                     	Inc %	           Inc
 nuget.packaging!NuGet.ContentModel.Infrastructure.PatternExpression+TokenSegment.TryMatch(class NuGet.ContentModel.ContentItem&,class System.String,class System.Collections.Generic.IReadOnlyDictionary`2,int32,int32&)	  1.3	   111,532,960
+ mscorlib.ni!System.String.Substring(Int32, Int32)                                                                                                                                                                      	  0.5	    40,336,008
+ mscorlib.ni!System.Collections.Generic.Dictionary`2[System.__Canon,System.__Canon].Insert(System.__Canon, System.__Canon, Boolean)                                                                                     	  0.4	    34,488,920
+ clr!JIT_New                                                                                                                                                                                                            	  0.3	    30,443,528
+ nuget.packaging!ContentPropertyDefinition.TryLookup                                                                                                                                                                    	  0.1	     6,264,504

3 runs each, on average, it's about 10MB for OrchardCore.
1/3 runs were a bit noisy and showing similar results.

Dictionary is the same amount of allocations, no substring allocations.
TryLookup is worse because of the ToString actualizations. A bit of a trade-off.

Note

This PR is not the last PR in this space.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception - Updated existing tests
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@nkolev92 nkolev92 marked this pull request as ready for review July 25, 2024 18:11
@nkolev92 nkolev92 requested a review from a team as a code owner July 25, 2024 18:11
@nkolev92 nkolev92 force-pushed the dev-jeffkl-tokensegment-trymatch branch 2 times, most recently from 7a782d3 to 6c5f8cc Compare July 25, 2024 18:16
@nkolev92 nkolev92 closed this Jul 25, 2024
@nkolev92 nkolev92 reopened this Jul 25, 2024
@nkolev92
Copy link
Member Author

I got a couple of changes that depend on this one, so if anyone's not focusing on somethig unrelating, this would be a great PR to review :)

jeffkl
jeffkl previously approved these changes Jul 30, 2024
@nkolev92
Copy link
Member Author

Thank you for the reviews @jeffkl, @dtivel, and @zivkan!

@nkolev92 nkolev92 merged commit 63b869a into dev Jul 31, 2024
28 of 29 checks passed
@nkolev92 nkolev92 deleted the dev-jeffkl-tokensegment-trymatch branch July 31, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants