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

revert ordering change to address PR feedback #45964

Merged
merged 1 commit into from
Jan 16, 2021

Conversation

adamsitnik
Copy link
Member

In #45563 I've removed some allocations related to storing CacheEntry scopes but also changed the order of exiting the scope.

This PR reverts the ordering change based on feedback from @davidfowl

So we are back to the "old order" but we still avoid a call to the expensive CacheEntryHelper.Current by taking advantage of having this value stored in _previous field.

#45563 (comment)

@ghost
Copy link

ghost commented Dec 11, 2020

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

Issue Details

In #45563 I've removed some allocations related to storing CacheEntry scopes but also changed the order of exiting the scope.

This PR reverts the ordering change based on feedback from @davidfowl

So we are back to the "old order" but we still avoid a call to the expensive CacheEntryHelper.Current by taking advantage of having this value stored in _previous field.

#45563 (comment)

Author: adamsitnik
Assignees: -
Labels:

area-Extensions-Caching

Milestone: 6.0.0

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Assuming David's cool with it, LGTM.

@eerhardt eerhardt merged commit 3f23874 into dotnet:master Jan 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 15, 2021
@adamsitnik adamsitnik deleted the revertOderingChange branch July 2, 2021 11:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants