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 collections resizing and yield allocations #5952

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Aug 5, 2024

Bug

Fixes: NuGet/Home#13677
Related: NuGet/Home#12728

Description

  • Reduce allocations caused by AddRange. This use to cause an extra dictionary + resizing allocations. Note that the refactoring create the target library is good practice since it ensures that we create the lock file target library there and not accidentally overwrite a collection with already existing values.
  • Stop using yield return, when the collection size is obvious.
  • Use IEnumerable where using a list is an option.
  • Mark unused methods as obsolete.

The improvements for an orchardcore restore total about ~50MB.

Micro benchmarks for part of the changes.

Method Mean Error StdDev Gen0 Gen1 Allocated
NJ 44.17 us 0.115 us 0.108 us 3.9673 0.1221 67.56 KB
MicrosoftBuildRuntime 66.33 us 0.168 us 0.149 us 8.4229 0.7324 141.95 KB
AllPackages 143,519.17 us 479.678 us 448.691 us 13500.0000 1000.0000 231438.42 KB

Default

|---------------------- |--------------:|-----------:|-----------:|-----------:|----------:|-------------:|
| NJ | 43.30 us | 0.078 us | 0.073 us | 4.0283 | 0.1221 | 68.77 KB |
| MicrosoftBuildRuntime | 66.20 us | 0.142 us | 0.125 us | 8.4229 | 0.7324 | 142.8 KB |
| AllPackages | 150,817.69 us | 668.471 us | 625.289 us | 14000.0000 | 1250.0000 | 238558.01 KB |

The gains from this are harder to prove since I refactored the methods quite a bit, but I estimated as ~3MB.

The gains from the yield changes are about 47MB.

Before:

Name                                                                                   	Inc %	           Inc
| + nuget.commands!NuGet.Commands.LockFileUtils+<ConvertToProjectPaths>d__15.MoveNext()	  1.1	   101,008,752
| |+ nuget.common!PathUtility.GetRelativePath                                          	  0.5	    49,087,496
| |+ nuget.commands!NuGet.Commands.LockFileUtils+<GetLockFileItems>d__16.MoveNext()    	  0.5	    46,649,376
| |+ nuget.common!PathUtility.GetPathWithForwardSlashes                                	  0.0	     4,341,616
| |+ clr!JIT_New                                                                       	  0.0	       930,264

After:

Name                                                                         	Inc %	          Inc
+ nuget.commands!LockFileUtils.ConvertToProjectPaths                         	  0.6	   56,099,112
|+ nuget.common!PathUtility.GetRelativePath                                  	  0.6	   49,771,504
|+ nuget.common!PathUtility.GetPathWithForwardSlashes                        	  0.0	    3,583,240
|+ clr!JIT_New                                                               	  0.0	    1,757,584
|+ mscorlib.ni!System.Collections.Generic.List`1[System.__Canon]..ctor(Int32)	  0.0	      986,784

Note that

| |+ nuget.commands!NuGet.Commands.LockFileUtils+d__16.MoveNext() 0.5 46,649,376
is pretty much gone.

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.

Copy link
Contributor

@dtivel dtivel left a comment

Choose a reason for hiding this comment

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

@nkolev92, consider adding a "Tips for writing performant code" doc to the repo with your learnings from these PR's. While there's no substitute for profiling, profiling is more demanding than an approachable list of best practices. For example, you could show common patterns with alternative patterns that minimize allocations.

@nkolev92 nkolev92 enabled auto-merge (squash) August 6, 2024 16:39
@nkolev92
Copy link
Member Author

nkolev92 commented Aug 6, 2024

@nkolev92, consider adding a "Tips for writing performant code" doc to the repo with your learnings from these PR's. While there's no substitute for profiling, profiling is more demanding than an approachable list of best practices. For example, you could show common patterns with alternative patterns that minimize allocations.

I can do that.
I think there's a lot better documents on performance best practices both internally and externally that I could ever write, so I'll try to reference those docs where possible.

@nkolev92 nkolev92 merged commit 92f78eb into dev Aug 6, 2024
28 checks passed
@nkolev92 nkolev92 deleted the dev-nkolev92-reduceCollectionsAndYieldAllocations branch August 6, 2024 17:26
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.

ConvertToProjectPaths causes extra allocations due to yield usage
4 participants