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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary closure / delegate allocation from CallSiteFactory.GetCallSite #44755

Merged
merged 1 commit into from
Nov 17, 2020
Merged

Remove unnecessary closure / delegate allocation from CallSiteFactory.GetCallSite #44755

merged 1 commit into from
Nov 17, 2020

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Nov 16, 2020 •
edited
Loading

Nothing is ever removed from the cache, and CreateCallSite is already storing the newly created ServiceCallSite into the dictionary, so GetOrAdd isn't providing any meaningful value: we can simply TryGetValue and then call CreateCallSite if it fails.

From startup of a dotnet new mvc app:
image

Related to #44598

@stephentoub
Remove unnecessary closure / delegate allocation from CallSiteFactory…
caf9698 
….GetCallSite

Nothing is ever removed from the cache, and CreateCallSite is already storing the newly created ServiceCallSite into the dictionary, so GetOrAdd isn't providing any meaningful value: we can simply TryGetValue and then call CreateCallSite if it fails.
@stephentoub stephentoub added this to the 6.0.0 milestone Nov 16, 2020
@ghost
Copy link

ghost commented Nov 16, 2020

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details
Description:

Nothing is ever removed from the cache, and CreateCallSite is already storing the newly created ServiceCallSite into the dictionary, so GetOrAdd isn't providing any meaningful value: we can simply TryGetValue and then call CreateCallSite if it fails.

From startup of a dotnet new mvc app:
image
Author: stephentoub
Assignees: -
Labels:

area-Extensions-DependencyInjection, tenet-performance
Milestone: [object Object]

eerhardt
eerhardt approved these changes Nov 16, 2020
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM

cc @davidfowl

@davidfowl
Copy link
Member

Nothing is ever removed from the cache,

Yet, this one an optimization we've discussed a number of times in the past if we ever need to reduce the memory profile of DI (which comes up)

@stephentoub
Copy link
Member Author

stephentoub commented Nov 16, 2020 •
edited
Loading

this one an optimization we've discussed a number of times in the past if we ever need to reduce the memory profile of DI

Even if it changes, shouldn't affect this PR.

@jkotas jkotas merged commit 886a014 into dotnet:master Nov 17, 2020
@stephentoub stephentoub deleted the callsitecache branch November 21, 2020 01:10
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidfowl davidfowl davidfowl approved these changes

@maryamariyan maryamariyan maryamariyan approved these changes

@eerhardt eerhardt eerhardt approved these changes

Assignees
No one assigned
Labels
area-Extensions-DependencyInjection tenet-performance Performance related issue
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@stephentoub @davidfowl @maryamariyan @eerhardt @jkotas