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 caches instead for session cache #1442

Conversation

stevefan1999-personal
Copy link
Contributor

@stevefan1999-personal stevefan1999-personal commented Sep 3, 2023

This PR almost removes the entirety of limited_cache.rs, because I spotted that it is just a LRU in a nutshell. This let us use more cache replacement policies, for example, the Adaptive Replacement Cache as an experiment.  

The caches crate offers no_std mode and it is taken care of. But there are few problems:

  1. Hidden KeyRef type complicates key lookup · Issue #2 · al8n/caches-rs (github.com)
  2. Buggy behavior on ARC · Issue #9 · al8n/caches-rs (github.com)

I have worked around the issue and made subsequent PRs for this. It is now await feedback and will replace the current git crate repo it to point to the main crates.io once the PRs are accepted and published.

Also it seems like the initial cache size is off by one, so I "fixed" it in the tests. It turns out the original LRU cache implementation seems to have an open interval.

@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Merging #1442 (b93e4af) into main (150b692) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1442      +/-   ##
==========================================
- Coverage   96.49%   96.47%   -0.02%     
==========================================
  Files          71       71              
  Lines       15171    15110      -61     
==========================================
- Hits        14639    14578      -61     
  Misses        532      532              
Files Changed Coverage Δ
rustls/src/client/handy.rs 98.80% <100.00%> (ø)
rustls/src/limited_cache.rs 100.00% <100.00%> (ø)
rustls/src/server/handy.rs 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ctz
Copy link
Member

ctz commented Sep 3, 2023

Here's a comment from last time we decided against taking a crate dependency for this: #469 (comment)

I think that still stands -- and I'm not feeling the caches crate is meeting that bar.

@stevefan1999-personal
Copy link
Contributor Author

@al8n wonder if you would like to fix those so that we could include it in rustls?

@al8n
Copy link

al8n commented Sep 4, 2023

@al8n wonder if you would like to fix those so that we could include it in rustls?

I would like to fix those, I will take a look this weekend.

@cpu
Copy link
Member

cpu commented Oct 16, 2023

It sounds like at a minimum this work is blocked on upstream adjustments to the caches dependency. I'm a little bit hesitant to leave a PR open in the tracker that has that sort of outstanding work blocking it. It's already collected up a number of conflicts with main.

I propose we close this PR until it seems more viable to land in days as opposed to weeks+. WDYT?

@djc
Copy link
Member

djc commented Oct 17, 2023

I agree we should close this, I don't think the caches dependency is a good fit for rustls.

@al8n
Copy link

al8n commented Oct 17, 2023

caches was developed when I was starting to learn Rust, but at the moment, I do not have enough time to improve the code quality. So I agree with closing this for now.

@cpu cpu closed this Oct 17, 2023
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.

None yet

5 participants