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

Now all together: Perf and CIDs #84

Merged
merged 3 commits into from
Dec 23, 2016
Merged

Now all together: Perf and CIDs #84

merged 3 commits into from
Dec 23, 2016

Conversation

dignifiedquire
Copy link
Member

@dignifiedquire dignifiedquire commented Dec 20, 2016

This is the perf improvement PR #83 and with the cid changes from #76 rebased onto them. All tests are passing and benchmarks are close to what I had before.

@dignifiedquire
Copy link
Member Author

This is ready for review, speed is now even a bit better than on #83 with some more optimizations.

@daviddias
Copy link
Member

daviddias commented Dec 21, 2016

For this to be merged all the way in, we want to have interop bitswap tests in js-ipfs API, which creates a tree of dependencies

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

LGTM, we just need to ship the release train 🚂

#### `putStream()`

Returns a duplex `pull-stream` that emits an object `{key: Multihash}` for every written block when it was stored.
Objects passed into here should be of the form `{data: Buffer, key: Multihash}`
Copy link
Member

Choose a reason for hiding this comment

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

It is no long data: Buffer, key Multihash, instead it is block: block, cid: cid

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


#### `put(blockAndCid, callback)`

- `blockAndKey: {data: Buffer, cid: CID}`
Copy link
Member

Choose a reason for hiding this comment

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

s/data: Buffer/block: Block

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

return _.map(_.range(n * blockFactor), () => {
return new Block(crypto.randomBytes(n * blockFactor))
})
}
Copy link
Member

Choose a reason for hiding this comment

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

❤️ benchmarks!

"lodash.pullallwith": "^4.7.0",
"lodash.uniqwith": "^4.5.0",
"lodash.values": "^4.3.0",
"multihashes": "^0.3.1",
Copy link
Member

Choose a reason for hiding this comment

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

Not used anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

.map((k) => l.wantlistContains(k))
.filter(Boolean)
.forEach((e) => {
// this.peerRequestQueue.push(e, l.partner)
Copy link
Member

Choose a reason for hiding this comment

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

I believe peerRequestQueue are no longer a thing, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep that's an artifact when I ripped it out :)

Copy link
Member

Choose a reason for hiding this comment

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

Did you ripped out PeerRequestQueue and MessageQueue, or just PeerRequestQueue? From your comment in #88 (comment), it sounds like you have removed both, but I believe only PeerRequestQueue is gone.

Copy link
Member Author

Choose a reason for hiding this comment

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

PeerRequestQueue is gone, but as mentioned in #88 if you look at the code for MsgQueue it's not a queue anymore, simply a buffer to coalesce messages. https://github.com/ipfs/js-ipfs-bitswap/blob/sq/src/components/want-manager/msg-queue.js

Copy link
Member Author

Choose a reason for hiding this comment

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

Which is important to keep on a per peer basis, as we send messages out on a per peer basis.

Copy link
Member

Choose a reason for hiding this comment

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

I need to update the graph :)

Copy link
Member Author

Choose a reason for hiding this comment

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

also feel free to suggest a better name :)

@@ -75,17 +91,27 @@ module.exports = class Network {
}

_onPeerMux (peerInfo) {
if (!this._running) {
return
}
Copy link
Member

Choose a reason for hiding this comment

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

good catch :)

@dignifiedquire
Copy link
Member Author

@diasdavid linking this together with the other repo PR, and the newly created interop tests and this adjustment in js-ipfs

diff --git a/src/core/components/go-online.js b/src/core/components/go-online.js
index 793a026..28894f7 100644
--- a/src/core/components/go-online.js
+++ b/src/core/components/go-online.js
@@ -14,7 +14,6 @@ module.exports = function goOnline (self) {
       }

       self._bitswap = new Bitswap(
-        self._libp2pNode.peerInfo,
         self._libp2pNode,
         self._repo.blockstore,
         self._libp2pNode.peerBook
(END)

I can confirm we can transfer data between go and js using bitswap 1.1.0 🎉

@daviddias
Copy link
Member

14:06 <@dignifiedquire> daviddias: #84 (comment) 🎉 bitswap is ready to rock
14:06 <@dignifiedquire> nice job! worked out of th box

AWESOME :D

Thank you :)

dignifiedquire and others added 3 commits December 23, 2016 08:22
- reduce base58 encoding
- use utf8 encoding instead of hex
- extract benchmarks from tests
- group multiple blocks into single messages
@dignifiedquire
Copy link
Member Author

🎉

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.

3 participants