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.

Sign up for GitHub

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

Jump to bottom

feat: allow usage of custom codecs #288

Merged
merged 1 commit into from
Jul 19, 2022
Merged

feat: allow usage of custom codecs #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { toString as uint8ArrayToString } from 'uint8arrays/to-string'
import { MessageCache } from './message-cache.js'
import { RPC } from './message/rpc.js'
import * as constants from './constants.js'
import { createGossipRpc, shuffle, hasGossipProtocol, messageIdToString } from './utils/index.js'
import { createGossipRpc, shuffle, messageIdToString } from './utils/index.js'
import {
PeerScore,
PeerScoreParams,
Expand Down Expand Up @@ -2411,7 +2411,12 @@ export class GossipSub extends EventEmitter<GossipsubEvents> implements Initiali
const backoff = this.backoff.get(topic)
for (const id of shuffledPeers) {
const peerStreams = this.streamsOutbound.get(id)
if (peerStreams && hasGossipProtocol(peerStreams.protocol) && !peers.has(id) && !this.direct.has(id)) {
if (
peerStreams &&
this.multicodecs.includes(peerStreams.protocol) &&
Copy link
Contributor

@dapplion dapplion Jun 23, 2022 •
edited
Loading

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

Copy link
Contributor Author

@D4nte D4nte Jun 23, 2022 •
edited
Loading

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:

  1. Modify the pubsub interface and then gossipsub to change multicodecs to a Set<string>.
  2. Add another field on GossipSub that is a Set and implement multicodecs as a getter to conform to the PubSub interface,
  3. 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.

Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

!peers.has(id) &&
!this.direct.has(id)
) {
const score = getScore(id)
if ((!backoff || !backoff.has(id)) && score >= 0) candidateMeshPeers.add(id)
// instead of having to find gossip peers after heartbeat which require another loop
Expand Down Expand Up @@ -2615,7 +2620,12 @@ export class GossipSub extends EventEmitter<GossipsubEvents> implements Initiali
const shuffledPeers = shuffle(Array.from(peersInTopic))
for (const id of shuffledPeers) {
const peerStreams = this.streamsOutbound.get(id)
if (peerStreams && hasGossipProtocol(peerStreams.protocol) && !fanoutPeers.has(id) && !this.direct.has(id)) {
if (
peerStreams &&
this.multicodecs.includes(peerStreams.protocol) &&
!fanoutPeers.has(id) &&
!this.direct.has(id)
) {
const score = getScore(id)
if (score >= this.opts.scoreThresholds.publishThreshold) candidateFanoutPeers.push(id)
// instead of having to find gossip peers after heartbeat which require another loop
Expand Down Expand Up @@ -2676,7 +2686,7 @@ export class GossipSub extends EventEmitter<GossipsubEvents> implements Initiali
if (!peerStreams) {
return
}
if (hasGossipProtocol(peerStreams.protocol) && filter(id)) {
if (this.multicodecs.includes(peerStreams.protocol) && filter(id)) {
peers.push(id)
}
})
Expand Down
5 changes: 0 additions & 5 deletions src/utils/has-gossip-protocol.ts
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion src/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
export * from './create-gossip-rpc.js'
export * from './shuffle.js'
export * from './has-gossip-protocol.js'
export * from './messageIdToString.js'
export { getPublishConfigFromPeerId } from './publishConfig.js'