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

Fast messsage id cache #179

Merged
merged 12 commits into from
Jan 14, 2022
Merged

Fast messsage id cache #179

merged 12 commits into from
Jan 14, 2022

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented Dec 22, 2021

Motivation

  • Gossipsub is designed to have message redundancy, right now we have to calculate 3.97 times per message data in average, in the worse case we calculate 10/11 times per message data

Description

  • Follow other clients to have a cheaper faster message id just for message de-duplication, so we have fast message id and canonical message id
  • Key as the sha256 of the message (fast message id), value as the canonical message id => enhance SimpleTimeCache for this
  • Instead of passing getMsgId() 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!)
  • Use the new getCanonicalMessageIdStr() function in all places instead of getMsgId() since we can't base on getMsgId() in lodestar, the message is removed very quickly from WeakMap
  • getCanonicalMessageIdStr(): get from the newly created getCachedMsgId() first, then the fast message id cache, then getMsgId

Test result

Screen Shot 2021-12-22 at 11 01 13

ts/index.ts Outdated
* @param {InMessage} message
* @returns
*/
getCachedMsgId (message: InMessage): Uint8Array | undefined {
Copy link
Contributor Author

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-commenter
Copy link

codecov-commenter commented Dec 22, 2021

Codecov Report

Merging #179 (7dd97e0) into master (1a442fe) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
test/utils/index.js 87.50% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a442fe...7dd97e0. Read the comment docs.

@twoeths twoeths requested a review from a team December 23, 2021 04:07
test/peer-score.spec.js Outdated Show resolved Hide resolved
dapplion
dapplion previously approved these changes Dec 23, 2021
Copy link
Contributor

@dapplion dapplion left a 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

ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
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",
Copy link
Member

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).

Copy link
Contributor

@dapplion dapplion 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! Agree with Cayman suggestion to pull sha256 dependency

dapplion
dapplion previously approved these changes Jan 7, 2022
ts/index.ts Outdated Show resolved Hide resolved
dapplion
dapplion previously approved these changes Jan 11, 2022
@wemeetagain
Copy link
Member

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.
We can switch some of this logic based on if the fast id function is set or not. eg: if (this.fastIdFnStr) { ... } else { ... }

dapplion
dapplion previously approved these changes Jan 13, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
test/heartbeat.spec.js Outdated Show resolved Hide resolved
test/heartbeat.spec.js Outdated Show resolved Hide resolved
test/compliance.spec.js Outdated Show resolved Hide resolved
test/compliance.spec.js Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
@wemeetagain wemeetagain merged commit 38ee6e1 into master Jan 14, 2022
@wemeetagain wemeetagain deleted the tuyen/fast_messsage_id_cache branch January 14, 2022 05:56
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.

None yet

4 participants