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

[#86] Adds policy caching #111

Merged
merged 10 commits into from
Jan 31, 2020
Merged

[#86] Adds policy caching #111

merged 10 commits into from
Jan 31, 2020

Conversation

clintfred
Copy link
Contributor

see #86

Adds simple policy caching for straightforward use cases where some number of policies are being evaluated repeatedly. Caching will greatly improve the performance of these usecases as it will save a roundtrip to the webservice per encryption.

@clintfred clintfred changed the title [#86] Add policy caching [#86] Adds policy caching Jan 30, 2020
src/internal/document_api/mod.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Copy link
Member

@coltfred coltfred left a comment

Choose a reason for hiding this comment

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

Some suggestions about making cache invalidation better, but all in all it seems pretty reasonable.

tests/document_ops.rs Show resolved Hide resolved
if errs.is_empty() {
//if the cache has grown too large, clear it prior to adding new entries
if policy_cache.len() >= config.max_entries {
policy_cache.clear()
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit sad that this is the invalidation strat we're using. Even removing some arbitrary entry would be better, wouldn't it? Also, you didn't check to see if the if the insert would be actually replacing an entry (concurrent requests).

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 agree that it's pretty simple minded. All we're really trying to guarantee here is that the cache can't grow without bound for a long running process. I feel like anything smarter probably should include time or usage stats.

I did consider doing what you suggest and just throwing out a single, random entry, which seems reasonable, but then we need to introduce some machinery to decide which (random) entry to throw out and it just didn't seem worth it.

src/lib.rs Show resolved Hide resolved
src/internal/document_api/mod.rs Show resolved Hide resolved
@clintfred clintfred merged commit 5bd8398 into master Jan 31, 2020
@clintfred clintfred deleted the 86-caching branch January 31, 2020 23:31
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.

3 participants