Skip to content

Update serialization extension to support custom cache-size metric #52

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

Merged
merged 4 commits into from
Mar 18, 2025

Conversation

justinmimbs
Copy link
Contributor

The current Serialization extension assumes the cache size is always the number of items in the cache. This PR allows serializing LRUs with custom by functions, where lru.currentsize != length(lru).

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.97%. Comparing base (1cfbef2) to head (2185bb5).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   81.92%   81.97%   +0.05%     
==========================================
  Files           3        3              
  Lines         354      355       +1     
==========================================
+ Hits          290      291       +1     
  Misses         64       64              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -29,20 +29,21 @@ function Serialization.deserialize(s::AbstractSerializer, ::Type{LRU{K, V}}) whe
lock = Serialization.deserialize(s)
by = Serialization.deserialize(s)
finalizer = Serialization.deserialize(s)
n_items = Serialization.deserialize(s)
Copy link
Member

Choose a reason for hiding this comment

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

is there a way to detect there are no bytes left to deserialize and fallback to n_items = currentsize here? to continue supporting de-serializing old serialized files

Copy link
Member

Choose a reason for hiding this comment

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

(if not, I'm not sure this should be a patch release, it seems like it could unexpectedly break stuff. Not quite sure what folks use LRU serialization for though, is it for IPC only or for persistence?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point; this will break deserializing old serialized data. I think we can make this backward compatible, and that would be nicer in case there are some persisted LRUs out there.

Any existing file would have n_items in the currentsize position, so we can continue storing n_items there, and always recompute currentsize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR. Thanks for pointing this out!

Copy link
Member

Choose a reason for hiding this comment

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

good solution!

@Jutho
Copy link
Collaborator

Jutho commented Feb 19, 2025

Thanks; this looks like a great PR. I have no additional comments. I suggest to leave this open one more day for maybe @jarbus or @lkdvos to take a look, and then merge and tag tomorrow.

@jarbus
Copy link
Contributor

jarbus commented Feb 19, 2025

This seems good to me as well. Thanks for the great changes!

Copy link
Contributor

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

Also looks like a great improvement to me, I don't think I can spot any issues here.

@fingolfin
Copy link
Member

This was approved and all people who were meant to comment did so... Merging...

@fingolfin fingolfin merged commit 9abf419 into JuliaCollections:master Mar 18, 2025
12 checks passed
@Jutho
Copy link
Collaborator

Jutho commented Mar 18, 2025

Thanks; seems like I had completely forgotten about this.

@justinmimbs justinmimbs deleted the serialize branch March 18, 2025 19:28
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.

Serialization does not support non-default by functions
7 participants