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

feat: use an expiry lru cache for the index backed blockstore #1167

Merged
merged 4 commits into from
Feb 9, 2023

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented Feb 8, 2023

Summary

This switches over the dagstore index backed blockstore to use an expiry based LRU cache instead of just an LRU. The existing LRU will never prune unless it fills up and evicts. With the new cache, expired entries will automatically be removed, and in the event the cache hits its max size, the shards closest to expiry will be pruned first.

  • Adds a config option, DealmakingConfig.BlockstoreCacheMaxShards to set the max shards in the cache. The previous value was hard coded at 100, which the default has been set to, but we may want to lower this as it can cause process memory to grow quite large. Too low can cause in progress retrievals to fail as shards in use can be pruned.
    • EDIT: reduced to 20 to match default simultaneous retrieval limits
  • Adds a config option, DealmakingConfig.BlockstoreCacheExpiry to set the duration a cached shard can be idle for until pruned, default is 10 minutes. Any time the cache is accessed it will be refreshed, so this could be set much lower if memory usage is a problem.
    • EDIT: reduced to 30 seconds

Memory Usage Before

Prior to the change with retrieval load on the system, followed by allowing the system to idle, heap space increases and is not reclaimed.

Looking at a diff of the heap space allocation we can see an increase in the piece reader memory pipeline despite the system being idle.

Memory Usage After

With these changes, allowing the system to idle after retrievals we can see in use heap decrease back to pre load levels.

Performing a diff of heap utilization we can see no significant markers of memory increase...

... and a clear reduction in piece reader heap between time in load and idle time

TODO

  • fix ci
  • bump dagstore version once released

Dependencies

Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

Looks good 👍
Feel free to merge once the dagstore release is ready.

Comment on lines 104 to 105
BlockstoreCacheMaxShards: 100,
BlockstoreCacheExpiry: Duration(10 * time.Minute),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we decrease these defaults to:

  • BlockstoreCacheMaxShards: 16
    16 seems like a reasonable default max for concurrent bitswap retrievals of a piece, and we don't want to go too much higher as each shard can potentially use GiBs of memory
  • BlockstoreCacheExpiry: 30s
    In the worst case we need to recreate the blockstore:
    • read the index from disk (about 5MB, should take no more than a few hundred milli-seconds)
    • get a reader over the piece (should take no more than a few hundred milli-seconds)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BlockstoreCacheExpiry: 30s should be a reasonable change. It gives plenty of buffer on an active retrieval and can be always be increased.

BlockstoreCacheMaxShards: 16 - Thinking more about this I think we should set this at 20, as it's currently our DefaultSimultaneousTransfers (yes, it only applies to Graphsync but does keep consistency in our defaults). I also think we should adjust the bitswap retrieval defaults to match this, 20.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, let's go for BlockstoreCacheMaxShards: 20 👍

@jacobheun jacobheun marked this pull request as ready for review February 9, 2023 15:48
@jacobheun jacobheun merged commit 51a632d into main Feb 9, 2023
@jacobheun jacobheun deleted the feat/expiry-cache branch February 9, 2023 15:52
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