-
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 small cache to execution traces #10517
Conversation
bc83c4f
to
3738433
Compare
Updated the PR, waiting on my lotus to sync so I can test 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.
LGTM
This PR adds a small cache to calls to ExecutionTrace which helps improve performance for node operators like exchanges and block explorers. If items is in cache calls to this function will be 2-3x faster. Fixes: #10504
877074a
to
2e45f6f
Compare
@raulk could you take another look at this PR so I can get it landed? |
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.
LGTM but would add an env variable to adjust the cache size. (Probably not a config value, unless we want to expose with a deep knob so openly)
@@ -39,6 +40,8 @@ import ( | |||
const LookbackNoLimit = api.LookbackNoLimit | |||
const ReceiptAmtBitwidth = 3 | |||
|
|||
const execTraceCacheSize = 16 |
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.
We probably want to make this configurable because SPs may want to disable it while exchanges may want to crank it up? We also are going with intuition for this value, so having ability to change it without a new build would help.
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.
Will make a followup PR which makes this tuneable via env variable so we can get this landed
Related Issues
#10504
Proposed Changes
This PR adds a small cache to calls to
ExecutionTrace
which helps improve performance for node operators like exchanges and block explorers.If items is in cache calls to this function will be 2-3x faster.
Additional Info
I ran a local test for 30minutes where I called
lotus state compute-state --json
in a loop from two threads.NOTE: Stress testing this over an hour resulted in Lotus process residual memory taking almost 30GB, I am kind concerned that it grew this large and GC didn't kick in, so will continue to stress test this for longer.
EDIT: Ran longer stress test, memory stayed flat so should be good
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