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

[refactor] introduce an LRU cache inside certificate store #9760

Merged
merged 7 commits into from
Mar 24, 2023

Conversation

akichidis
Copy link
Contributor

@akichidis akichidis commented Mar 23, 2023

Description

This PR is introducing an LRU cache in the certificate_store in order to minimise the impact from the read operations (only on the primary index at the moment). That will help:

  • reduce the number of reads against rocksdb
  • avoid deserialisation costs of Certificate

relevant metrics for cache hit/miss have been added.

The cache has been set to hold 100 * 60 items (100 nodes X 60 rounds) - but we can tune further. Also, we only populate the cache when we write a certificate - especially for the cases we are interested I don't consider we need to re-populate on read.

Test Plan

Added unit tests


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

@vercel
Copy link

vercel bot commented Mar 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Ignored Deployments
Name Status Preview Comments Updated
explorer ⬜️ Ignored (Inspect) Mar 24, 2023 at 9:42PM (UTC)
explorer-storybook ⬜️ Ignored (Inspect) Mar 24, 2023 at 9:42PM (UTC)
wallet-adapter ⬜️ Ignored (Inspect) Mar 24, 2023 at 9:42PM (UTC)

@akichidis akichidis marked this pull request as ready for review March 24, 2023 00:26
Copy link
Collaborator

@kchalkias kchalkias left a comment

Choose a reason for hiding this comment

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

In some other uses of LRU we had a wrapper over the lru dependency. Shall we reuse that logic here as a template? ie check this PR
#9056

*maybe @mystenmark and @benr-ml who did that before can provide more intel on this (ie we use this for metrics etc)

Copy link
Collaborator

@kchalkias kchalkias left a comment

Choose a reason for hiding this comment

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

Check this LRU pattern from @benr-ml as well
#9712

Copy link
Contributor

@arun-koshy arun-koshy left a comment

Choose a reason for hiding this comment

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

Nice addition Tasos! 🚀

Copy link
Member

@mwtian mwtian left a comment

Choose a reason for hiding this comment

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

I think adding a data structures that keeps certificates above GC round consistently should not be much more complex. If we add that in future, do you think we still need the cache?

@akichidis
Copy link
Contributor Author

I think adding a data structures that keeps certificates above GC round consistently should not be much more complex. If we add that in future, do you think we still need the cache?

@mwtian let's talk more about what you have in mind because for sure I can see other alternatives than this cache. I would still prefer adding this for the time being so we get some gains and we can always remove it later as it's not too invasive. What do you think?

@akichidis akichidis merged commit 27ca30d into main Mar 24, 2023
@akichidis akichidis deleted the exp-synchronizer-read-all branch March 24, 2023 22:23
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.

4 participants