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

Correctly count number of items in LRU #8742

Merged
merged 3 commits into from
Nov 29, 2023
Merged

Conversation

koenbeuk
Copy link
Contributor

@koenbeuk koenbeuk commented Nov 24, 2023

Fixes #8741

Microsoft Reviewers: Open in CodeFlow

Verified

This commit was signed with the committer’s verified signature.
scothis Scott Andrews

Verified

This commit was signed with the committer’s verified signature.
scothis Scott Andrews
@koenbeuk koenbeuk changed the title [WIP] Updated test to reflect counting issue Correctly count number of items in LRU Nov 24, 2023
@modabas
Copy link

modabas commented Nov 26, 2023

I think, moving increment count into static add function builders may be a problem, and may cause incrementing count way above real value at startup with many concurrent operations.
Also does LRU.GetOrAdd need a fix? It seems to work properly and does not increment count at update. For LRU.AddOrUpdate, if intent is not to increment count on update, perhaps following code may work (didn't test)

    public TValue AddOrUpdate<TState>(TKey key, Func<TState, TKey, TValue> addFunc, TState state)
    {
        var generation = GetNewGeneration();
        var storedValue = cache.AddOrUpdate(
            key,
            static (key, state) =>
            {
                var (_, outerState, addFunc, generation) = state;
                return new TimestampedValue(addFunc(outerState, key), generation);
            },
            static (key, existing, state) =>
            {
                var (self, outerState, addFunc, generation) = state;
                return new TimestampedValue(addFunc(outerState, key), self.GetNewGeneration());
            },
            (Self: this, State: state, AddFunc: addFunc, Generation: generation));

        var result = storedValue.Value;

        if (storedValue.Generation == generation)
        {
            Interlocked.Increment(ref count);
            AdjustSize();
        }

        return result;
    }

@modabas
Copy link

modabas commented Nov 26, 2023

I have put together a quick test app. Added a CacheCount property to LRU class that returns actual item count in cache to demonstrate what i meant.

Verified

This commit was signed with the committer’s verified signature.
scothis Scott Andrews
@koenbeuk
Copy link
Contributor Author

I think, moving increment count into static add function builders may be a problem, and may cause incrementing count way above real value at startup with many concurrent operations.

Thanks for catching that, this indeed seems to be an issue. I've updated this PR to use the logic as you proposed.

@ReubenBond ReubenBond merged commit e115a90 into dotnet:main Nov 29, 2023
@koenbeuk koenbeuk deleted the issue-8741 branch November 30, 2023 03:13
@mikescandy
Copy link

It seems this is the cause of an issue we're having in our production deployment that I haven't noticed until recently
image

Aug 23 is when we upgraded from 7 to 7.2.1, which includes the live grain migration and the new lru logic.
For context, I see the count exploding only for a stateless worker grain that we call for an health check
It would be awesome if this release could be release asap :)

ReubenBond pushed a commit to ReubenBond/orleans that referenced this pull request Dec 2, 2023
* Updated test to reflect counting issue

* Only increment and adjust size on Add

* Updated LRU count computation logic
ReubenBond added a commit that referenced this pull request Dec 2, 2023
* Updated test to reflect counting issue

* Only increment and adjust size on Add

* Updated LRU count computation logic

Co-authored-by: Koen <koen@bpk.ltd>
ReubenBond added a commit that referenced this pull request Dec 2, 2023
* Updated test to reflect counting issue

* Only increment and adjust size on Add

* Updated LRU count computation logic

Co-authored-by: Koen <koen@bpk.ltd>
@rwkarg
Copy link
Contributor

rwkarg commented Dec 2, 2023

@mikescandy Release is out with the updates:
https://github.com/dotnet/orleans/releases/tag/v7.2.4

It seems this is the cause of an issue we're having in our production deployment that I haven't noticed until recently image

Aug 23 is when we upgraded from 7 to 7.2.1, which includes the live grain migration and the new lru logic. For context, I see the count exploding only for a stateless worker grain that we call for an health check It would be awesome if this release could be release asap :)

@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LRU: AddOrUpdate incorrectly increments current count
5 participants