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

bitswap: Bitswap now sends multiple blocks per message #5

Merged
merged 2 commits into from Oct 4, 2018

Conversation

Projects
None yet
4 participants
@taylormike
Copy link
Contributor

taylormike commented Aug 13, 2018

Updated PeerRequestTask to hold multiple wantlist.Entry(s). This allows Bitswap to send multiple blocks in bulk per a Peer's request. Also, added a metric for how many blocks to put in a given message. Currently: 512 * 1024 bytes.
Fixes #4378
@whyrusleeping @Stebalien

@whyrusleeping whyrusleeping requested review from Stebalien and schomatis Aug 30, 2018

@whyrusleeping

This comment has been minimized.

Copy link
Member

whyrusleeping commented Aug 30, 2018

Hey @taylormike, sorry for the wait. New repos mean i wasnt watching for notifications.

@whyrusleeping whyrusleeping self-requested a review Aug 30, 2018

newWorkExists = true
if blockSize, _ = e.bs.GetSize(entry.Cid); blockSize == -1 {
// Force Update Size Cache
e.bs.Get(entry.Cid)

This comment has been minimized.

@Stebalien

Stebalien Aug 30, 2018

Contributor

This really shouldn't be necessary. GetSize should always work (even if it isn't cached).

This comment has been minimized.

@taylormike

taylormike Sep 6, 2018

Contributor

@Stebalien I agree, I removed the 'GetSize, then Get, then GetSize sequence'
This is done. I pushed out these changes and squashed into a single commit.

activeEntries = []*wl.Entry{}
msgSize = 0
}
activeEntries = append(activeEntries, entry.Entry)

This comment has been minimized.

@Stebalien

Stebalien Aug 30, 2018

Contributor

Shouldn't this be in an else condition?

This comment has been minimized.

@taylormike

taylormike Sep 6, 2018

Contributor

This is intentional.
That being said, I can rewrite it as such:

  if msgSize + blockSize < maxMessageSize {
       activeEntries = append(activeEntries, entry.Entry)
       msgSize += blockSize 
       } else {
	 e.peerRequestQueue.Push(p, activeEntries...)
	 activeEntries = []*wl.Entry{}
	 msgSize = 0
       }
@@ -46,7 +46,7 @@ type prq struct {
}

// Push currently adds a new peerRequestTask to the end of the list
func (tl *prq) Push(entry *wantlist.Entry, to peer.ID) {
func (tl *prq) Push(entries []*wantlist.Entry, to peer.ID) {

This comment has been minimized.

@Stebalien

Stebalien Aug 30, 2018

Contributor

API nit: While we're changing this, I'd change it to Push(to peer.ID, entries ...*wantlist.Entry).

This comment has been minimized.

@taylormike

taylormike Sep 6, 2018

Contributor

@Stebalien I agree.
This is done. I pushed out these changes and squashed into a single commit.

@schomatis
Copy link
Member

schomatis left a comment

Very clean, nice. The message size logic seems OK. (I'm not familiarized with the rest of the code.)

@@ -231,6 +240,8 @@ func (e *Engine) MessageReceived(p peer.ID, m bsmsg.BitSwapMessage) error {
l.wantList = wl.New()
}

blockSize, msgSize := 0, 0

This comment has been minimized.

@whyrusleeping

whyrusleeping Aug 31, 2018

Member

cleaner to write:

var blockSize, msgSize int

This comment has been minimized.

@taylormike

taylormike Sep 6, 2018

Contributor

@whyrusleeping I agree.
This is done. I pushed out these changes and squashed into a single commit.

newWorkExists = true
if blockSize, _ = e.bs.GetSize(entry.Cid); blockSize == -1 {

This comment has been minimized.

@whyrusleeping

whyrusleeping Aug 31, 2018

Member

I wouldnt throw away an error. If this actually errors, something seems pretty wrong. It's worthy of logging IMO

This comment has been minimized.

@schomatis

schomatis Aug 31, 2018

Member

There was a previous mention from @Stebalien that GetSize wouldn't fail, but maybe we could add a comment there explaining it, since yes, that silent error seems suspicious.

This really shouldn't be necessary. GetSize should always work (even if it isn't cached).

This comment has been minimized.

@Stebalien

Stebalien Aug 31, 2018

Contributor

Sorry, that statement was a bit confusing. I meant that if the Get worked, the GetSize should have worked. That is, we can just call:

size, err := e.bs.GetSize(entry.Cid)
if err != nil {
    /* we don't have it, *may* be an error, check if it's bs.ErrNotFound */
} else {
    // we have the block
}

We don't need to call GetSize, then Get, then GetSize.

This comment has been minimized.

@taylormike

taylormike Sep 6, 2018

Contributor

@whyrusleeping, @schomatis, @schomatis I agree.
I removed the 'GetSize, then Get, then GetSize sequence' and I'm now logging the error.

This is done. I pushed out these changes and squashed into a single commit.

@taylormike taylormike force-pushed the taylormike:feat/bitswap/sendmultiple branch from d501754 to 433f52e Sep 6, 2018

@taylormike

This comment has been minimized.

Copy link
Contributor

taylormike commented Sep 6, 2018

I pushed out the most recent code review feedback changes and squashed into a single commit.
I also updated bench_test, peer_request_queue_test, and engine_test accordingly.

@Stebalien
Copy link
Contributor

Stebalien left a comment

One small performance nit and one slightly larger concern.

When testing this, it didn't appear to have much impact until I upped the fetch concurrency quite a bit. However, I've been wanting to do that for a while anyways. Once I did that, I noticed a ~20% speedup (not counting the speedup from the increased parallelism).

Also, I wasn't able to test in a real system. I expect this'll have quite a bit more impact on a network with many peers.

newWorkExists = true
if blockSize, err = e.bs.GetSize(entry.Cid); err != nil {

This comment has been minimized.

@Stebalien

Stebalien Sep 6, 2018

Contributor

Actually, we should probably just use GetSize instead of Has. That way, we only make one request.

This comment has been minimized.

@Stebalien

Stebalien Sep 6, 2018

Contributor

(that is, we can replace the Has check in the condition)

This comment has been minimized.

@taylormike

taylormike Oct 4, 2018

Contributor

@Stebalien
I agree, I pushed out this change and squashed into a single commit.

func taskKey(p peer.ID, k *cid.Cid) string {
func taskKey(p peer.ID, entries []*wantlist.Entry) string {
key := string(p)
for _, entry := range entries {

This comment has been minimized.

@Stebalien

Stebalien Sep 6, 2018

Contributor

This is a bit unfortunate as it could grow quite large (very large). I wonder if we can switch to a different method for computing task keys.

This comment has been minimized.

@Stebalien

Stebalien Sep 6, 2018

Contributor

Ah, I see. This isn't actually used. Mind removing it along with the Key() function?

This comment has been minimized.

@taylormike

taylormike Oct 4, 2018

Contributor

@Stebalien Sorry for the delayed response.
This is done. I pushed out this change and squashed into a single commit.

@Stebalien

This comment has been minimized.

Copy link
Contributor

Stebalien commented Sep 6, 2018

I've tested this against go-ipfs and it doesn't appear to break anything.

@Stebalien

This comment has been minimized.

Copy link
Contributor

Stebalien commented Sep 6, 2018

It looks like the checkHandledInOrder test is broken.

@taylormike

This comment has been minimized.

Copy link
Contributor

taylormike commented Sep 7, 2018

@Stebalien I'm looking into the checkHandledInOrder test failure.

@taylormike taylormike force-pushed the taylormike:feat/bitswap/sendmultiple branch from 433f52e to bd8c0f4 Oct 4, 2018

@taylormike

This comment has been minimized.

Copy link
Contributor

taylormike commented Oct 4, 2018

@Stebalien Sorry for the delayed response.
The checkHandledInOrder test failure is now resolved. I pushed out this change and squashed into a single commit.

bitswap: Bitswap now sends multiple blocks per message
Updated PeerRequestTask to hold multiple wantlist.Entry(s). This allows Bitswap to send multiple blocks in bulk per a Peer's request. Also, added a metric for how many blocks to put in a given message. Currently: 512 * 1024 bytes. 

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>

@Stebalien Stebalien force-pushed the taylormike:feat/bitswap/sendmultiple branch from bd8c0f4 to eb0d1ff Oct 4, 2018

@wafflebot wafflebot bot added the in progress label Oct 4, 2018

gx: update go-ipfs-bitswap
fixes bs.GetSize bug
@Stebalien

This comment has been minimized.

Copy link
Contributor

Stebalien commented Oct 4, 2018

Running the go-ipfs sharness tests now.

@Stebalien

This comment has been minimized.

Copy link
Contributor

Stebalien commented Oct 4, 2018

Looks good!

@Stebalien Stebalien merged commit cbd7eb7 into ipfs:master Oct 4, 2018

@wafflebot wafflebot bot removed the in progress label Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment