-
Notifications
You must be signed in to change notification settings - Fork 961
Delete the block_cache
#8404
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
base: unstable
Are you sure you want to change the base?
Delete the block_cache
#8404
Conversation
| /// Fetch custody info from the cache. | ||
| /// If custody info doesn't exist in the cache, | ||
| /// try to fetch from the DB and prime the cache. | ||
| // FIXME(sproul): re-add cache for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eserilev Did you remember if caching the DataColumnCustodyInfo had a noticeable performance benefit? I think it probably helps a bit, but we also don't load it very often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelsproul we don't load it very often. The only places it's used is when responding to Status and DataColumnByRange RPC requests and during custody backfill. During custody backfill and DataColumnByRange RPC requests any performance benefits from caching this value seems negligible to me, since we're already reading and/or writing from/to disk.
I don't know how often we respond to Status requests. But without caching we'd now have to read from disk when responding to this request. Doesn't seem like a big deal to me, but we could cache it in a similar way that we cache AnchorInfo, BlobInfo, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try measuring it. I suspect LevelDB would cache it in memory for us anyway if we are reading frequently for status messages. Also wouldn't be too hard to cache as you said
Proposed Changes
The
block_cachewas disabled by default for hindering performance. We have no plans to re-enable it, and likely a performant replacement would look somewhat different from what we have now.This PR deletes the
block_cacheimplementation entirely, achieving a slight simplification of our DB code, and also takes aim at thedo_atomically_with_block_and_blobs_cachemethod which IMO was overly complicated for little gain.Additional Info
TODO:
DataColumnCustodyInfo.do_atomically_with_block_and_blobs_cacheto something more appropriate.