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

Use an LRU cache for standalone funs #82

Closed
wants to merge 3 commits into from

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Apr 11, 2022

I tried out micro-benchmarking some LRU styles: the-mikedavis/lru_bench and I found one that I think works pretty well, especially for concurrent read/write throughput. I need to plug this in to khepri-benchmark to see how it fares with concurrency but the micro-benchmarks say that this cache is ~1.4x slower for get/2s and ~1.8x slower for put/2s compared to persistent_term which I think is not too bad.

Mostly I had fun beating lru which is linear runtime (this implementation is logarithmic) on the cache capacity for both put/2 and get/2. Look how it crawls in that XX-Large case! 😄

I'd like to improve the documentation and maybe rename some variables but I thought I'd open this up to discussion earlier rather than later. Plus I have some questions:

  • Does this match expectations for interfaces/behaviours? I based it on rabbit_auth_cache
  • Should I be tuning any of the read/write concurrency settings in ets?

And mostly: is the lack of atomicity in put/2 and get/2 ok? A tla+ checker might point out some nasty scenarios from concurrent put/2s and get/2s, but given that this cache is purely for speed, I think the fidelity of the data doesn't matter much: if keys are dropped or something lives in the cache for longer than expected, it's no big deal. What do you think?

connects #75

@coveralls
Copy link

coveralls commented Apr 11, 2022

Pull Request Test Coverage Report for Build 530

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 47 of 47 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 87.775%

Totals Coverage Status
Change from base Build 521: 0.3%
Covered Lines: 1874
Relevant Lines: 2135

💛 - Coveralls

@michaelklishin
Copy link
Member

michaelklishin commented Apr 11, 2022

Khepri already offers a preference for Ra query types (local, leader). In many cases, getting stale data can be acceptable. In others, a way to bypass the cache would
give the user full control.

Cache misses are OK as long as they are relatively rare (assuming there are significantly more reads than writes).

@the-mikedavis
Copy link
Member Author

I should clarify that this is the cache interface for transaction functions (#75) rather than the query cache (#33). For this cache, a key living too long would mean extra memory overhead and a key living too shortly would mean it would need to be re-compiled - it shouldn't affect data staleness. This implementation scales well with capacity so we should be able to tune it for the trade-off between memory consumption and cache misses without too much trouble.

@the-mikedavis
Copy link
Member Author

I wonder if the cache should allow passing in an eviction callback that gets called when a key is dropped from the cache? Even if a key is dropped from the cache, the standalone function's module isn't purged so memory could continuously increase unless we purge evicted modules.

@the-mikedavis
Copy link
Member Author

Also I updated the micro-benchmark (https://github.com/the-mikedavis/lru_bench/tree/0263ed83bc35b58d2e50a95302c849d666492238) to run in parallel and this implementation seems to scale even better under concurrent access. It's still ~1.5x slower than persistent_term for get/2s but actually ~2.5x faster for put/2s 🚀

@michaelklishin
Copy link
Member

Ah, I see. Then perfect read consistency is not a goal for this cache since it does not store user data. I trust your judgement @the-mikedavis 👍

@dumbbell dumbbell added the enhancement New feature or request label Apr 20, 2022
@dumbbell dumbbell self-assigned this Apr 20, 2022
@dumbbell dumbbell changed the title use an LRU cache for standalone funs Use an LRU cache for standalone funs Jun 8, 2022
@the-mikedavis
Copy link
Member Author

Closing for now - we can revisit this later if the memory consumption of standalone funs becomes more pressing. We might end up re-using or modifying the LRU cache from this branch in the query cache.

This needs a bit of work: the cache should take a callback that it executes when evicting an entry so we can unload the standalone func's module. That way we also purge the memory taken up by the code as well.

@the-mikedavis the-mikedavis deleted the md-cache-interface branch November 14, 2022 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants