-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fast messsage id cache #179
Conversation
ts/index.ts
Outdated
* @param {InMessage} message | ||
* @returns | ||
*/ | ||
getCachedMsgId (message: InMessage): Uint8Array | undefined { |
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.
Lodestar getMsgId()
should divide its implementation into getCachedMsgId()
and computeMsgId()
Codecov Report
@@ Coverage Diff @@
## master #179 +/- ##
=======================================
Coverage 76.58% 76.58%
=======================================
Files 2 2
Lines 1905 1905
Branches 140 141 +1
=======================================
Hits 1459 1459
Misses 446 446
Continue to review full report at Codecov.
|
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 looks really nice! Thanks for looking into the memory costs 👍
Can you please open an issue to raise the timeout to 33 slots? Memory should be analyzed for that case too
1b77fe1
to
82e8b44
Compare
package.json
Outdated
@@ -41,6 +41,7 @@ | |||
"homepage": "https://github.com/ChainSafe/js-libp2p-gossipsub#readme", | |||
"dependencies": { | |||
"@types/debug": "^4.1.7", | |||
"@chainsafe/as-sha256": "^0.2.4", |
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.
Maybe the fastMsgId should be passed into the constructor, like the msgId in the pubsub base class, so we don't need to add this dependency (maybe just dev dependency for testing).
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.
Looks good! Agree with Cayman suggestion to pull sha256 dependency
I think the fast id feature should be optional. Likely many applications (including ipfs and existing apps) will not need this feature and shouldn't pay for the cost. |
dfc2628
to
cad1127
Compare
Motivation
Description
SimpleTimeCache
for thisgetMsgId()
function to other classes (PeerScore, GossipTracer, MessageCache) in the constructor, just pass message id through method as we always have them in the consumer code which make it simpler (and Lighthouse does this!)getCanonicalMessageIdStr()
function in all places instead ofgetMsgId()
since we can't base ongetMsgId()
in lodestar, the message is removed very quickly from WeakMapgetCanonicalMessageIdStr()
: get from the newly createdgetCachedMsgId()
first, then the fast message id cache, thengetMsgId
Test result
seenTTL
option anywaysubscribeAllSubnets
flag in lodestar