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

Generic Improvements #169

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

jonhartnett
Copy link
Contributor

Disclaimer: This PR is kind of big. I recommend reviewing it patch-by-patch instead of all at once, which will make the individual atomic changes a lot easier to follow.

This PR is the result of a bunch of improvements that I made on my local fork that I thought I'd share upstream. Incidentally, they happen to solve many of the open issues. All changes have associated documentation and were validated against the existing tests + doc tests. New tests and doc tests were introduced where appropriate to validate new features.

Overview:

  1. Reduce memory usage by merging head + tail and switching internally from a HashMap to a HashSet.
  2. Eliminate eager allocation of the root node(s) on new
  3. Allow 0 as a capacity limit, which is a nice-to-have use case, but mainly simplifies much of the api/docs
  4. Add an Entry api for easier + more efficient transformations that don't fit into one of the main accessor functions. Also simplifies the internals, which makes it easier to reason about the safety of unsafe blocks.
  5. Add a Limiter api for limiting the cache size based on metrics other than len (e.g. memory usage)

I realize this is a lot to review on one go, so if you have issues with any of the individual commits, lmk and I can resubmit as individual PRs.

@jeromefroe
Copy link
Owner

@jonhartnett I'm extremely sorry for the delay following up here. I had meant to respond when the issue was first opened, but I forgot to do so promptly and ended up completely losing track of this 🤦. If it's possible to break these changes into multiple smaller PR's that would greatly help me review the changes 😅

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.

2 participants