JuliaCollections / LRUCache.jl

An implementation of an LRU Cache in Julia
Other
56 stars 23 forks source link

Add support for serialization of large caches #46

Closed jarbus closed 9 months ago

jarbus commented 9 months ago

Fixes #45, where serialization of large LRU might result in a stack overflow. Does not automatically load Serialization, but uses extensions to overwrite Serialization.{serialize,deserialize} if Serialization is loaded, available in julia 1.9+.

codecov-commenter commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (3125353) 78.45% compared to head (77d67a0) 81.97%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #46 +/- ## ========================================== + Coverage 78.45% 81.97% +3.52% ========================================== Files 2 3 +1 Lines 297 355 +58 ========================================== + Hits 233 291 +58 Misses 64 64 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jarbus commented 9 months ago

Thanks for the review @ericphanson, committed the changes you suggested!

Jutho commented 9 months ago

This looks great, thanks. I've also added some minor comments and questions.

jarbus commented 9 months ago

All great feedback so far, I've learned an unexpected amount about Julia from this simple PR. Thanks!

Jutho commented 9 months ago

Do you want another review? I can ask a person in our group that recently implemented some serialisation things for some other cache structures that we have in our code, but he is only available from Friday onwards. I am also fine with having this merged as is.

jarbus commented 9 months ago

I'm fine with either, doesn't hurt to get another set of eyes on it and I've already pointed all my code to my branch anyways

lkdvos commented 9 months ago

Woops, seems like I was too fast in suggesting the while loop to for loop change, the iterator indeed only returns the values, not the nodes themselves. My apologies!

jarbus commented 9 months ago

Thanks for the in-depth feedback @lkdvos! I'll look into implementing your suggested refactor. Also I suppose I need to undo that last commit now as well haha

jarbus commented 9 months ago

@lkdvos Finished working on your recommended changes, serialization code is much cleaner now. Thanks for your input!

Jutho commented 9 months ago

Looks good to me; maybe @lkdvos can take a final look and then we can merge.

jarbus commented 9 months ago

Good point, honestly not sure how to handle the multiple references to mutable values case, though.

ericphanson commented 9 months ago

The regular serializer handles it, so I was hoping it would just work

jarbus commented 9 months ago

Great feedback @ericphanson, worked as expected. I was able to clean everything up, code is much nicer now.

jarbus commented 9 months ago

@Jutho @ericphanson are there any other changes you recommend before merging?

ericphanson commented 9 months ago

Sorry, forgot about this. I think it’s good to go!