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

Implement eviction callback. #23

Merged
merged 2 commits into from
Jun 3, 2021

Conversation

GunnarFarneback
Copy link
Contributor

This PR adds an eviction callback, which if set is called for each element that leaves the cache. This is useful if your cached values contain some resource that you want to recover. PR #1 had the same idea but is very much out of date with respect to the code.

Unfortunately this functionality doesn't come entirely for free, even if no callback is set. In terms of test/benchmark.jl the timing differences seem to be within the measurement noise but what is definitely noticeable is that all functions that may cause eviction (setindex!, get!, pop!, delete!, resize!, empty!) get one extra allocation of 80 bytes to hold the empty list of pending evictions.

To avoid this allocation will either require:

  1. Notify the callback while the lock is held. This seems inadvisable.
  2. Increase the implementation complexity in some way to avoid the allocation, possibly with function barriers to induce code specialization via some type trick or constant propagation.
  3. Some brilliant insight that avoids the allocation in a simple way. Haven't been that lucky so far.

@GunnarFarneback
Copy link
Contributor Author

I suspect the failures of the 4 thread tests on Julia 1.0 is because the combination of @threads and Channel is too difficult there. It should be fine just to disable the test under those circumstances.

@codecov-io
Copy link

codecov-io commented Feb 1, 2021

Codecov Report

Merging #23 (a3145c7) into master (1fadc28) will increase coverage by 3.70%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   72.81%   76.51%   +3.70%     
==========================================
  Files           2        2              
  Lines         217      247      +30     
==========================================
+ Hits          158      189      +31     
+ Misses         59       58       -1     
Impacted Files Coverage Δ
src/LRUCache.jl 96.62% <100.00%> (+0.89%) ⬆️
src/cyclicorderedset.jl 46.46% <0.00%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fadc28...a3145c7. Read the comment docs.

@Krastanov
Copy link

I was wondering what the status of this is? I have a use case where I my cache includes image files saved to disk, and this pull request would be incredibly helpful.

@Jutho
Copy link
Collaborator

Jutho commented Apr 18, 2021

My apologies; I completely forgot about this. I am occupied at least until Tuesday, but will try to look at later this week.

@Jutho
Copy link
Collaborator

Jutho commented Jun 3, 2021

My apologies for the late response; I finally have some time for this. Is it ok if I try to come up with some other implementation fo this which is free of cost without callback. Also, eviction_callback is a bit long and obscure. How do you think about finalizer as a keyword argument, in analogy to how you can register a finalizer to a mutable julia object.

@Jutho
Copy link
Collaborator

Jutho commented Jun 3, 2021

Actually, I will merge this and only then make a few changes. Are you fine with renaming eviction_callback to finalizer as keyword argument?

@Jutho Jutho merged commit 9c806d4 into JuliaCollections:master Jun 3, 2021
@GunnarFarneback GunnarFarneback deleted the eviction_callback branch June 4, 2021 05:50
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.

4 participants