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

Remove the need to allocate a Func on each compatibility cache lookup #4095

Merged
merged 2 commits into from
Jun 11, 2021

Conversation

jebriede
Copy link
Contributor

@jebriede jebriede commented Jun 4, 2021

Bug

Fixes: NuGet/Home#10919

Regression? Last working version: N/A

Description

Instead of allocating a func to pass to the GetOrAdd method of the concurrent dictionary on each lookup, check the dictionary first to see if the entry already exists and evaluate its compatibility and add it to the cache only if it does not. That way, a Func does not need to be allocated for each lookup and reduces the overall GC pressure.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
      Code is covered by existing tests
    • Test exception
    • OR
    • N/A
  • Documentation

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

@jebriede jebriede requested a review from a team as a code owner June 4, 2021 22:03
@jebriede
Copy link
Contributor Author

jebriede commented Jun 4, 2021

Using OrchardCore as the solution, this fix eliminates the following allocations:

Name                                                                      	Inc %	          Inc
 Type System.Func`2[NuGet.Frameworks.CompatibilityCacheKey,System.Boolean]	  0.6	   42,664,364
+ clr!?                                                                   	  0.6	   42,664,364
 + nuget.frameworks!CompatibilityProvider.IsCompatible                    	  0.6	   42,664,364

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

🔪 those allocations.

src/NuGet.Core/NuGet.Frameworks/CompatibilityProvider.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@erdembayar erdembayar left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

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

Could you please summarize the allocation impact of this fix for Solution Load in OrchardCore?

@jebriede
Copy link
Contributor Author

jebriede commented Jun 5, 2021

Could you please summarize the allocation impact of this fix for Solution Load in OrchardCore?

@aortiz-msft: The allocations in this table quantify the impact this had when doing a restore for OrchardCore. Based on this profile, it's about 42MB of allocations that are no longer being made with this fix! 😄
#4095 (comment)

I plan on pulling data on the impact of each of my fixes on Solution Load with OrchardCore once this series of PRs have been merged.

Copy link
Contributor

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@jebriede jebriede force-pushed the dev-jebriede-RemoveAnonymousFunc branch from 6d2df22 to 3d7b143 Compare June 11, 2021 00:12
@jebriede jebriede merged commit 47975b9 into dev Jun 11, 2021
@jebriede jebriede deleted the dev-jebriede-RemoveAnonymousFunc branch June 11, 2021 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants