-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: Add tipset -> Message lookup cache #10528
Conversation
This commit adds a cache for looking up block messages in a tipset. This should improve performance for quite a few lotus APIs, including: - The Eth API block accesses. - Fee history. - Tipset message counts - StateListMessages - (and likely more) Fixes: #10519
chain/store/store.go
Outdated
@@ -47,6 +47,7 @@ var ( | |||
|
|||
var DefaultTipSetCacheSize = 8192 | |||
var DefaultMsgMetaCacheSize = 2048 | |||
var DefaultTipSetMessageCacheSize = 8192 |
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.
This seems too aggressive. How did we arrive to this number? Assuming an average of 150 messages per tipset, this could be holding a total of 1228800 messages. And if we store this, do we even need the above message meta cache any longer?
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 just put some number there when I was working on the PR, it was still in draft. I am not sure what number to put here, how large are each message on average? Anyway, I lowered it to 1024 but that may still be too high?
@@ -105,6 +105,10 @@ type BlockMessages struct { | |||
|
|||
func (cs *ChainStore) BlockMsgsForTipset(ctx context.Context, ts *types.TipSet) ([]BlockMessages, error) { | |||
// returned BlockMessages match block order in tipset | |||
tsKey := ts.Key() | |||
if entry, ok := cs.tsMsgCache.Get(tsKey); ok { | |||
return entry, nil |
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 grepped the code and I don't see any caller changing the returned slice from this function. However, nothing prevents a caller from overwriting the array represented by the returned slice so a correct implementation would be to return a copy here like we do in #10517. Thoughts?
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'm not very convinced this is necessary, to be honest -- see #10519 (comment).
Closing for now, we can review again if we think this is needed once #10552 lands |
Related Issues
See #10519
Proposed Changes
This commit adds a cache for looking up block messages in a tipset. This should improve performance for quite a few lotus APIs, including:
Additional Info
I made the cache size configurable through an env variable LOTUS_CHAIN_TIPSET_MESSAGE, so we need to update the Lotus doc accortingly (https://lotus.filecoin.io/lotus/configure/defaults/#environment-variables)
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps