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

Change the way raw values are preserved in TryParse #5960

Merged
merged 4 commits into from
Aug 9, 2024

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Aug 7, 2024

Bug

Related: NuGet/Home#12728

Description

Say we have the lib/{tfm}/{assembly} pattern
During the TokenSegment.TryParse method if we encounter a tfm, we store the parsed tfm value in the ContentItem to pick the best group later. We also store the tfm_raw in some cases, since we need it in pack. Now since this is only needed during pack, we don't need to store and create string allocations during restore.

The implementation isn't a design I love, but something that we're constrained to due to our public APIs. Open to other suggestions if people have any.
In general server team and SDK team would use the ManagedCodeConventions specified by default and used during restore. In order to not break that, I removed tfm_raw allocations completely in the general codepath.
A PatternDefintion carries the knowledge that the parser needs to preserve the raw values. I added it as an init property to avoid breaking the constructor or a lot of code duplication. The fact that it's an init property keeps it immutable.
As part of this work, I also null annotated ContentQueryDefinition and improved the allocations in the constructor. This saves only few hundred KBs so it wasn't worth calling out.

Total, this shaves off about 10MB during an OrchardCore restore.

Before:

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&)	  0.9	   79,744,304
+ mscorlib.ni!System.Collections.Generic.Dictionary`2[System.__Canon,System.__Canon].Insert(System.__Canon, System.__Canon, Boolean)                                                                                     	  0.4	   33,491,856
+ nuget.packaging!NuGet.ContentModel.ContentItem.get_Properties()                                                                                                                                                        	  0.2	   17,589,200
+ nuget.packaging!ContentPropertyDefinition.TryLookup                                                                                                                                                                    	  0.1	   12,173,224
+ system.memory.ni!?                                                                                                                                                                                                     	  0.1	    9,595,200
+ clr!JIT_New                                                                                                                                                                                                            	  0.1	    6,894,824

After:

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&)	  0.8	   69,501,008
+ mscorlib.ni!System.Collections.Generic.Dictionary`2[System.__Canon,System.__Canon].Insert(System.__Canon, System.__Canon, Boolean)                                                                                     	  0.4	   32,375,832
+ nuget.packaging!NuGet.ContentModel.ContentItem.get_Properties()                                                                                                                                                        	  0.2	   19,186,544
+ nuget.packaging!ContentPropertyDefinition.TryLookup                                                                                                                                                                    	  0.1	   10,983,648
+ clr!JIT_New                                                                                                                                                                                                            	  0.1	    6,954,984

Note that this removes: + system.memory.ni? which is the string actualization from ReadOnlyMemory in the specific TryMatch method.
After I think the only realistic allocation reductions that don't completely involve refactoring all of the assets selection are the dictionary usages.

Some micro benchmarks. The improvements are more significant there.

Method Mean Error StdDev Gen0 Gen1 Allocated
NJ 44.65 us 0.257 us 0.228 us 3.9673 0.1221 67.56 KB
MicrosoftBuildRuntime 66.93 us 0.243 us 0.228 us 8.4229 0.7324 141.95 KB
AllPackages 165,097.44 us 1,003.022 us 889.153 us 16000.0000 2250.0000 272144.95 KB
Method Mean Error StdDev Gen0 Gen1 Allocated
NJ 40.24 us 0.353 us 0.331 us 3.2959 0.1221 56.5 KB
MicrosoftBuildRuntime 62.62 us 0.822 us 0.686 us 8.1787 0.6104 138.23 KB
AllPackages 152,979.21 us 804.331 us 713.019 us 14250.0000 2000.0000 240733.08 KB

PR Checklist

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

@nkolev92 nkolev92 marked this pull request as ready for review August 7, 2024 21:54
@nkolev92 nkolev92 requested a review from a team as a code owner August 7, 2024 21:54
@nkolev92 nkolev92 requested a review from martinrrm August 9, 2024 00:55
@nkolev92
Copy link
Member Author

nkolev92 commented Aug 9, 2024

@NuGet/nuget-client Can I please get a review here?

I have 2 follow-up PRs and they're both dependent on this one.

Comment on lines +24 to +25
var groupPatternsArray = groupPatterns as PatternDefinition[] ?? groupPatterns.ToArray();
var pathPatternsArray = pathPatterns as PatternDefinition[] ?? pathPatterns.ToArray();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public APIs 🤦

@nkolev92 nkolev92 merged commit 4b906a9 into dev Aug 9, 2024
28 checks passed
@nkolev92 nkolev92 deleted the dev-nkolev92-rawValues branch August 9, 2024 22:47
@nkolev92
Copy link
Member Author

nkolev92 commented Aug 9, 2024

@martinrrm If you have more feedback, I'd be happy to address it.
I'm just trying to unblock my other 2 PRs.

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.

3 participants