-
Notifications
You must be signed in to change notification settings - Fork 47
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: allow usage of custom codecs #288
Conversation
The CI errors I am encountering are not related to the changes introduced by this PR:
|
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
if (peerStreams && hasGossipProtocol(peerStreams.protocol) && !peers.has(id) && !this.direct.has(id)) { | ||
if ( | ||
peerStreams && | ||
this.multicodecs.includes(peerStreams.protocol) && |
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.
includes loops over an array, would it be better to use a Set? Otherwise it's a quadratic loop inside shuffledPeers
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.
The multicodecs
field comes from the PubSub
interface in @libp2p/interface-pubsub
: https://github.com/libp2p/js-libp2p-interfaces/blob/9f8d5f45647e3bd8943acfbfb77424b0f83c27cf/packages/interface-pubsub/src/index.ts#L74
I see 3 solutions:
- Modify the pubsub interface and then gossipsub to change
multicodecs
to aSet<string>
. - Add another field on
GossipSub
that is aSet
and implementmulticodecs
as a getter to conform to thePubSub
interface, - Leave it as it is.
Happy to proceed with either.
In this instance, I do not expect a library consumer to set a high number of value in the array so I am not sure 1 & 2 are worth it
I can see that the check is done inside a loop of a loop (this.mesh.forEach
and then shuffledPeers
) so indeed, it may be worth doing 1 or 2.
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.
Lets try to upstream the change in the interface (1)
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.
Lets try to upstream the change in the interface (1)
Done: libp2p/js-libp2p-interfaces#260
Waiting for it to be reviewed/merged/released to update this PR.
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.
To move forward with this, I need to do a run of the test suite with a Set
and an Array
to check if there is any performance gain: libp2p/js-libp2p-interfaces#260 (comment)
Unfortunately, the CI is currently broken on this repo due to the lack of lockfile and discrepancy in the dependencies being used. I am looking if I can fix that myself.
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.
Getting there, I hit a road block trying to bump several libp2p libraries, some help would be appreciated :) #289
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.
Running benchmarks on a droplet with multicodecs
as Array
and Set
.
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.
Note that the Set
does not seem to help with performance: libp2p/js-libp2p-interfaces#260 (comment)
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.
Would it be acceptable to merge the PR with just 4b873a3 ?
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.
yeah
The `multicodecs` values are the protocol codecs for the given pubsub implementation. They are likely to be set once (at instantiation) but accessed multiple time: everytime it needs to check whether a remote peer supports the same pubsub protocol. Hence, the usage of `Set` over `Array` is more adapted as it will improve access performance by using `Set.prototype.has` O(1) instead of `Array.prototype.includes` O(N). Refs: ChainSafe/js-libp2p-gossipsub#288
Blocked on CI failure:
|
The gossip codecs are now available on `GossipSub.multicodecs`. Hence, the check can use this field instead of a function that uses the constants. This enables library consumers to set their own codecs by assigning a different value to `GossipSub.multicodecs`.
Codecov Report
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
- Coverage 81.48% 80.44% -1.04%
==========================================
Files 44 41 -3
Lines 9447 9104 -343
Branches 917 826 -91
==========================================
- Hits 7698 7324 -374
- Misses 1749 1780 +31
Continue to review full report at Codecov.
|
Can you force push the right version here? looks like it still has the commit to change to a Set |
Done! Ready. |
The gossip codecs are now available on
GossipSub.multicodecs
.Hence, the check can use this field instead of a function that uses
the constants.
This enables library consumers to set their own codecs by assigning a
different value to
GossipSub.multicodecs
.