This repository has been archived by the owner on Jul 21, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 60
fix: performance improvements #107
Merged
Merged
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
12fb0d7
fix: add basic timeout to individual query func calls
jacobheun bf152e0
fix: per s/kademlia start with K peers, not Alpha
jacobheun 81c26d9
fix: properly use kBucketSize option when provided
jacobheun 57133a3
fix: make disjoint paths a function of kBucketSize/2
jacobheun 6ce1e1b
refactor: split up query classes for easier testing
jacobheun a1f9e2b
test: add test to verify paths stop
jacobheun e49045d
chore: fix linting
jacobheun 2d3bacd
fix: make alpha 3 again, reduce rpc message timeout
jacobheun 81a558c
refactor: remove unneeded peerBook.put and update jsdocs
jacobheun 75ec755
fix: stop the worker if it is done querying
jacobheun e21967f
test: add spy on continueQuering to its call count
jacobheun 909f44c
docs: update jsdocs on utils
jacobheun a0eab2c
fix: make queries quicker but sloppier
jacobheun 6e7ba6c
fix: dont error provide until all peers have been attempted
jacobheun d9831bb
fix: add timeout to query func
jacobheun 61a8e26
test: add a simulation
jacobheun 1e63a30
feat: allow configuration of alpha concurrency
jacobheun a1c9890
test(sim): add common peer check
jacobheun fa203c7
fix: apply suggestions from code review
dirkmc 9c886f1
fix: remove ncp in favor of kBucketSize
jacobheun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,12 +74,19 @@ class KadDHT extends EventEmitter { | |
*/ | ||
this.kBucketSize = options.kBucketSize || c.K | ||
|
||
/** | ||
* Number of disjoint query paths to use | ||
* This is set to `kBucketSize`/2 per the S/Kademlia paper | ||
* @type {number} | ||
*/ | ||
this.disjointPaths = Math.ceil(this.kBucketSize / 2) | ||
|
||
/** | ||
* Number of closest peers to return on kBucket search, default 20 | ||
* | ||
* @type {number} | ||
*/ | ||
this.ncp = options.ncp || c.K | ||
this.ncp = options.ncp || this.kBucketSize | ||
|
||
/** | ||
* The routing table. | ||
|
@@ -321,7 +328,7 @@ class KadDHT extends EventEmitter { | |
waterfall([ | ||
(cb) => utils.convertBuffer(key, cb), | ||
(id, cb) => { | ||
const rtp = this.routingTable.closestPeers(id, c.ALPHA) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing as this code is the same for every query, maybe it should be part of the Query itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed |
||
const rtp = this.routingTable.closestPeers(id, this.kBucketSize) | ||
|
||
this._log('peers in rt: %d', rtp.length) | ||
if (rtp.length === 0) { | ||
|
@@ -412,7 +419,7 @@ class KadDHT extends EventEmitter { | |
return callback(err) | ||
} | ||
|
||
const tablePeers = this.routingTable.closestPeers(id, c.ALPHA) | ||
const tablePeers = this.routingTable.closestPeers(id, this.kBucketSize) | ||
|
||
const q = new Query(this, key, () => { | ||
// There is no distinction between the disjoint paths, | ||
|
@@ -442,7 +449,7 @@ class KadDHT extends EventEmitter { | |
|
||
waterfall([ | ||
(cb) => utils.sortClosestPeers(Array.from(res.finalSet), id, cb), | ||
(sorted, cb) => cb(null, sorted.slice(0, c.K)) | ||
(sorted, cb) => cb(null, sorted.slice(0, this.kBucketSize)) | ||
], callback) | ||
}) | ||
}) | ||
|
@@ -527,6 +534,7 @@ class KadDHT extends EventEmitter { | |
provide (key, callback) { | ||
this._log('provide: %s', key.toBaseEncodedString()) | ||
|
||
const errors = [] | ||
waterfall([ | ||
(cb) => this.providers.addProvider(key, this.peerInfo.id, cb), | ||
(cb) => this.getClosestPeers(key.buffer, cb), | ||
|
@@ -536,10 +544,21 @@ class KadDHT extends EventEmitter { | |
|
||
each(peers, (peer, cb) => { | ||
this._log('putProvider %s to %s', key.toBaseEncodedString(), peer.toB58String()) | ||
this.network.sendMessage(peer, msg, cb) | ||
this.network.sendMessage(peer, msg, (err) => { | ||
if (err) errors.push(err) | ||
cb() | ||
}) | ||
}, cb) | ||
} | ||
], (err) => callback(err)) | ||
], (err) => { | ||
if (errors.length) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should help mitigate #105. While the provide will still technically fail, we'll at least have attempted to provide to all peers. |
||
// This should be infrequent. This means a peer we previously connected | ||
// to failed to exchange the provide message. If getClosestPeers was an | ||
// iterator, we could continue to pull until we announce to kBucketSize peers. | ||
err = errcode(`Failed to provide to ${errors.length} of ${this.kBucketSize} peers`, 'ERR_SOME_PROVIDES_FAILED', { errors }) | ||
} | ||
callback(err) | ||
}) | ||
} | ||
|
||
/** | ||
|
@@ -613,7 +632,7 @@ class KadDHT extends EventEmitter { | |
waterfall([ | ||
(cb) => utils.convertPeerId(id, cb), | ||
(key, cb) => { | ||
const peers = this.routingTable.closestPeers(key, c.ALPHA) | ||
const peers = this.routingTable.closestPeers(key, this.kBucketSize) | ||
|
||
if (peers.length === 0) { | ||
return cb(errcode(new Error('Peer lookup failed'), 'ERR_LOOKUP_FAILED')) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can you add docs to
options.ncp
?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.
So, looking through the code we actually only use
dht.ncp
in 1 spot. I kind of think we should just get rid of it altogether and replace that occurrence withdht.kBucketSize
. While it could potentially provide some finer grained control of the number of results we're returning, it's not doing that and I think it adds more confusion than it's worth right now. Thoughts?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.
Agree 👍
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.
Sounds good!
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.
ncp is now gone in favor of
kBucketSize