Skip to content
This repository was archived by the owner on Jul 21, 2023. It is now read-only.

fix: performance improvements #107

Merged
merged 20 commits into from
May 8, 2019
Merged

fix: performance improvements #107

Show file tree
Hide file tree
Changes from 7 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 Apr 24, 2019
bf152e0
fix: per s/kademlia start with K peers, not Alpha
jacobheun Apr 24, 2019
81c26d9
fix: properly use kBucketSize option when provided
jacobheun Apr 24, 2019
57133a3
fix: make disjoint paths a function of kBucketSize/2
jacobheun Apr 24, 2019
6ce1e1b
refactor: split up query classes for easier testing
jacobheun Apr 25, 2019
a1f9e2b
test: add test to verify paths stop
jacobheun Apr 25, 2019
e49045d
chore: fix linting
jacobheun Apr 25, 2019
2d3bacd
fix: make alpha 3 again, reduce rpc message timeout
jacobheun Apr 25, 2019
81a558c
refactor: remove unneeded peerBook.put and update jsdocs
jacobheun Apr 25, 2019
75ec755
fix: stop the worker if it is done querying
jacobheun Apr 25, 2019
e21967f
test: add spy on continueQuering to its call count
jacobheun Apr 25, 2019
909f44c
docs: update jsdocs on utils
jacobheun Apr 25, 2019
a0eab2c
fix: make queries quicker but sloppier
jacobheun Apr 26, 2019
6e7ba6c
fix: dont error provide until all peers have been attempted
jacobheun Apr 26, 2019
d9831bb
fix: add timeout to query func
jacobheun Apr 26, 2019
61a8e26
test: add a simulation
jacobheun Apr 26, 2019
1e63a30
feat: allow configuration of alpha concurrency
jacobheun Apr 27, 2019
a1c9890
test(sim): add common peer check
jacobheun Apr 27, 2019
fa203c7
fix: apply suggestions from code review
dirkmc Apr 29, 2019
9c886f1
fix: remove ncp in favor of kBucketSize
jacobheun May 8, 2019
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
10 changes: 3 additions & 7 deletions src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ exports.PROVIDERS_VALIDITY = 24 * hour

exports.PROVIDERS_CLEANUP_INTERVAL = hour

exports.READ_MESSAGE_TIMEOUT = minute
exports.READ_MESSAGE_TIMEOUT = 30 * second

// The number of records that will be retrieved on a call to getMany()
exports.GET_MANY_RECORD_COUNT = 16
Expand All @@ -32,18 +32,14 @@ exports.GET_MANY_RECORD_COUNT = 16
exports.K = 20

// Alpha is the concurrency for asynchronous requests
exports.ALPHA = 3

// Number of disjoint query paths to use
// This is set to K/2 per the S/Kademlia paper
exports.DISJOINT_PATHS = 10
exports.ALPHA = 6

exports.maxMessageSize = 2 << 22 // 4MB

exports.defaultRandomWalk = {
enabled: true,
queriesPerPeriod: 1,
interval: 5 * minute,
timeout: 30 * second,
timeout: 10 * second,
delay: 10 * second
}
17 changes: 12 additions & 5 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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 ?

Copy link
Contributor Author

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 with dht.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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree 👍

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Contributor Author

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


/**
* The routing table.
Expand Down Expand Up @@ -321,7 +328,7 @@ class KadDHT extends EventEmitter {
waterfall([
(cb) => utils.convertBuffer(key, cb),
(id, cb) => {
const rtp = this.routingTable.closestPeers(id, c.ALPHA)
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
})
})
Expand Down Expand Up @@ -613,7 +620,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'))
Expand Down
4 changes: 2 additions & 2 deletions src/private.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ module.exports = (dht) => ({
* Try to fetch a given record by from the local datastore.
* Returns the record iff it is still valid, meaning
* - it was either authored by this node, or
* - it was receceived less than `MAX_RECORD_AGE` ago.
* - it was received less than `MAX_RECORD_AGE` ago.
*
* @param {Buffer} key
* @param {function(Error, Record)} callback
Expand Down Expand Up @@ -522,7 +522,7 @@ module.exports = (dht) => ({
}
})

const peers = dht.routingTable.closestPeers(key.buffer, c.ALPHA)
const peers = dht.routingTable.closestPeers(key.buffer, dht.kBucketSize)

timeout((cb) => query.run(peers, cb), providerTimeout)((err) => {
query.stop()
Expand Down
Loading