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

LFU after access fails to update expiry when read buffer is not full #603

Merged
merged 4 commits into from
Jun 8, 2024

Conversation

bitfaster
Copy link
Owner

@bitfaster bitfaster commented Jun 1, 2024

From the discussion here #516, this is the repro:

[Fact]
public void Repro()
{ 
    var cache = new ConcurrentLfuBuilder<int, int>()
        .WithCapacity(10)
        .WithExpireAfterAccess(TimeSpan.FromSeconds(5))
        .Build();

    cache.AddOrUpdate(1, 1);

    Thread.Sleep(4000);

    cache.TryGet(1, out var value).Should().BeTrue(); // maintenance doesn't run here

    Thread.Sleep(2000);

    cache.TryGet(1, out value).Should().BeTrue();
    cache.TryGet(1, out value).Should().BeTrue();
}

Since maintenance only runs when the read buffer is full, expiry time is never updated.

Solution:

  • Policy computes new expiry time at the call site (either on read or update), instead of during maintenance.
  • For read, IsExpired has just been called, so we can piggyback on the current time to avoid two calls
  • We can no longer avoid re-scheduling the same node as part of maintenance (previously we checked if expiry time had changed). Likely the removal of calling expiry calculator is equal cost.

There are a couple of further optimizations:

  1. For expire after access, we could make a policy which has empty OnRead and AfterRead methods, enabling the JIT to elide the method calls to the policy and the expiry calculator, which are anyway a no-op.
  2. During maintenance, it may be worth testing the node we are now scheduling against the last node we scheduled. For cases where the same node is read many times, this may avoid duplicate scheduling. Need to measure whether adding this is actually beneficial.

@coveralls
Copy link

coveralls commented Jun 1, 2024

Coverage Status

coverage: 99.209% (+0.07%) from 99.141%
when pulling ac459e7 on users/alexpeck/lfuafteraccess
into 07f8a1c on main.

@bitfaster
Copy link
Owner Author

bitfaster commented Jun 2, 2024

For 4ee9397 the JIT does not completely elide the additional branch, there is an extra and (.NET 6 shown)

image
image

.NET Fwk has 4 extra instructions:
image

@bitfaster
Copy link
Owner Author

bitfaster commented Jun 8, 2024

.NET 6 generates identical assembly:

image

.NET Fwk, 4 bytes extra code within TryGetImpl. It looks like this is really only 1 extra instruction because the disassembly has a bunch of dupes (bug in benchmark dot net):

image

@bitfaster bitfaster marked this pull request as ready for review June 8, 2024 20:26
@bitfaster bitfaster merged commit 85a14e4 into main Jun 8, 2024
13 checks passed
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.

LFU expire after access policy fails to update expiry until read buffer is full
2 participants