large refactor of bitswap, implement wantmanager to manage wantlist #1218

whyrusleeping · 2015-05-09T07:14:48Z

This PR moves all wantlist update logic into the wantmanager, this ensures that outgoing messages are synchronized and coalesced as needed. This greatly improves bitswap performance and correctness.

whyrusleeping · 2015-05-09T07:52:17Z

exchange/bitswap/bitswap_test.go

@@ -35,30 +34,6 @@ func TestClose(t *testing.T) {
 	bitswap.Exchange.GetBlock(context.Background(), block.Key())
 }

-func TestProviderForKeyButNetworkCannotFind(t *testing.T) { // TODO revisit this


this test made my head hurt. Pretty sure it wasnt testing anything useful.

well it's testing what error pops out of bitswap when: (a) we have a provider, but (b) it is not reachable. we need to keep these tests. if the name is too complicated, maybe:

TestUnreachableProvider

what you describe is not what is being tested here. all it appears to be testing is that a context expires

the test does appear to be testing it fine, see below. But the timeout may be way too small. maybe 1us not ns.

// create an identity and provide a block. pinfo := p2ptestutil.RandTestBogusIdentityOrFatal(t) rs.Client(pinfo).Provide(context.Background(), block.Key()) // but not on network solo := g.Next() defer solo.Exchange.Close() ctx, _ := context.WithTimeout(context.Background(), time.Nanosecond) // getting the block should fail, because the provider isn't there. _, err := solo.Exchange.GetBlock(ctx, block.Key()) // the way it fails is that thectx expires. if err != context.DeadlineExceeded {

whyrusleeping · 2015-05-09T08:21:12Z

The big problem here, is that we want backpressure everywhere but in bitswap...

jbenet · 2015-05-09T09:36:36Z

So, doing some 'real world' testing on this, it doesnt appear that the net notifee thing actually works, i get messages going to and from peers that ive never received a 'Connected' notification from.

that's concerning. can you repro in an isolated case? i can sink into it and check it out

jbenet · 2015-05-09T09:38:42Z

exchange/bitswap/peermanager.go

+	// TODO move this to peerstore
+)
+
+type PeerManager struct {


may want to give an overview of what PeerManager is for here

jbenet · 2015-05-09T09:56:44Z

without yet understanding it fully, this seems to me like a great step forward.

whyrusleeping · 2015-05-09T17:36:34Z

without yet understanding it fully, this seems to me like a great step forward.

I hope so! 😄

whyrusleeping · 2015-05-13T00:17:45Z

@jbenet this is RFCR, i'm going to rebase in a little bit, but the big stuff is there. This is MUCH faster for transfer speeds on the open internet, theres an occasional pause of ten seconds that i'm still tracking down, but it never truly hangs or degrades. It will always continue after bitswaps 10 second 're-request' timer ticks. In practice, ive only seen the pause once every three or four runs.

jbenet · 2015-05-13T05:47:52Z

exchange/bitswap/peermanager.go

+			if err != nil {
+				log.Error(err)
+				// TODO: cant connect, what now?
+			}


it's ok to check if you really wil send to mq.p. if there's a connection, this call returns early.

okay, thoughts on what to do when it fails?

if ConnectTo fails-- connection failed. So it's like a disconnection, i'd say?

jbenet · 2015-05-13T05:58:22Z

@whyrusleeping one thing i'm missing is: how do partial updates happen?

whyrusleeping · 2015-05-13T06:14:50Z

partial wantlist updates happen the same way as they normally do. you just send your message through the peermanager instead of the network, and if multiple messages pile up while youre either dialing or sending a previous message, they get combined

jbenet · 2015-05-13T06:16:11Z

you just send your message through the peermanager instead of the network

could you point me to funcs + flow? i'm failing to see it :(

whyrusleeping · 2015-05-13T06:19:18Z

a partial update happen here: https://github.com/ipfs/go-ipfs/pull/1218/files#diff-9d5aff71f348a6f1cdcd472cd7bd4415R391

via a broadcast (which just means, send to everyone)

jbenet · 2015-05-13T06:25:06Z

exchange/bitswap/peermanager.go

+	}()
+
+	if mq.wlmsg == nil || msg.Full() {
+		mq.wlmsg = msg


if partial updates come this way too, not sure this should be getting set? maybe only if Full() ? not sure.

or is mq.wlmsg a message "staged" to be sent out?

yes, the wlmsg is the one we are staging to be sent out, and in runQueue we grab it, and send it out upon receiving the work signal

jbenet · 2015-05-13T06:29:14Z

@whyrusleeping i don't grasp this changeset well-- the datastructs aren't obviously clear, for example:

type msgQueue struct {
 p peer.ID

 lk    sync.Mutex
 wlmsg bsmsg.BitSwapMessage

 work chan struct{}
 done chan struct{}
}

turns out this msgQueue is not as generic as the name implies-- no blocks go through it, only wantlist messages. and wlmsg is mostly a full wantlist message, unless it's nil. in which case it can be a partial one. it seems that this is just a message staged to be sent out. perhaps something like this is clearer:

type wantlistMsgQueue struct {
 p peer.ID

 lk   sync.Mutex  // guards  ... only next? or what else?
 next bsmsg.BitSwapMessage

 work chan struct{}
 done chan struct{}
}

the other datastructures could use some clarifying too. the control flow is hard.

maybe try drawing it? it's possible that drawing it will either reveal the complexity OR the simplicity -- i'm maybe not seeing it right.

whyrusleeping · 2015-05-13T06:32:27Z

its only a full wantlist if the caller makes it a full wantlist. im not sure what youre seeing here

whyrusleeping · 2015-05-13T06:34:46Z

    if mq.wlmsg == nil || msg.Full() {
        mq.wlmsg = msg
        return
    }

here, if we arent holding a message, we take the one being added.
OR
if the message being added is full, we overwrite whatever else we had.

Otherwise, down here, we add the entries from the passed in wantlist to the one we are holding, to combine them into one, saving us a send call

    // TODO: add a msg.Combine(...) method
    for _, e := range msg.Wantlist() {
        if e.Cancel {
            mq.wlmsg.Cancel(e.Key)
        } else {
            mq.wlmsg.AddEntry(e.Key, e.Priority)
        }
    }

wking · 2015-05-13T17:49:15Z

exchange/bitswap/bitswap.go

@@ -326,7 +316,7 @@ func (bs *Bitswap) sendWantlistToProviders(ctx context.Context, entries []wantli

 // TODO(brian): handle errors
 func (bs *Bitswap) ReceiveMessage(ctx context.Context, p peer.ID, incoming bsmsg.BitSwapMessage) error {
-	defer log.EventBegin(ctx, "receiveMessage", p, incoming).Done()
+	//defer log.EventBegin(ctx, "receiveMessage", p, incoming).Done()


This is version controlled, so we should delete instead of commenting-out code we want to kill.

jbenet · 2015-05-21T02:54:16Z

@whyrusleeping b0eb0d2 breaks. all others pass.

whyrusleeping · 2015-05-21T03:02:02Z

noted, ill rebase that out

Also more refactoring of bitswap in general, including some perf improvements and eventlog removal. clean up, and buffer channels move some things around correctly buffer work messages more cleanup, and improve test perf remove unneccessary test revert changes to bitswap message, they werent necessary

large refactor of bitswap, implement wantmanager to manage wantlist

jbenet · 2015-05-21T19:47:50Z

After speaking with @whyrusleeping we decided to merge this in now, as it is already better than master.

whyrusleeping · 2015-05-21T21:32:14Z

yeap, its better, not perfect. but we can address the new issues as they come up

whyrusleeping added the status/in-progress In progress label May 9, 2015

whyrusleeping reviewed May 9, 2015
View reviewed changes

jbenet reviewed May 9, 2015
View reviewed changes

whyrusleeping mentioned this pull request May 11, 2015

Sprint G ipfs/team-mgmt#8

Closed

30 tasks

whyrusleeping force-pushed the refactor/bitswap branch from cece493 to 0e20492 Compare May 13, 2015 05:02

jbenet reviewed May 13, 2015
View reviewed changes

wking reviewed May 13, 2015
View reviewed changes

whyrusleeping force-pushed the refactor/bitswap branch from 479cefa to f9f85ff Compare May 20, 2015 00:24

jbenet force-pushed the refactor/bitswap branch 2 times, most recently from 3c1e74f to b827e4e Compare May 20, 2015 17:21

whyrusleeping force-pushed the refactor/bitswap branch from b827e4e to 7f3bb79 Compare May 21, 2015 03:07

jbenet force-pushed the refactor/bitswap branch from 7f3bb79 to 2b80fa2 Compare May 21, 2015 05:14

whyrusleeping and others added 15 commits May 21, 2015 03:08

update comments and reintroduce test

8443b99

contextify peermanager

ef967ce

WIP: super awesome bitswap cleanup fixtime

6bf33ad

fix race bugs

32da687

move taskdone inside lock boundaries

65f815a

turn tests down a bit and better context passing

6ab4bfe

turn rebroadcast back on

594c778

explicitly set bitswap message fullness

829b884

fixup the bitswap readme

2eac921

add a distribution test with the rebroadcast delay disabled

2882c79

better bitswap logging

c273a3b

clarify synhronization constructs

b71a0ac

warning -> notice

2f934e8

defer tock.Stop()

ce0d2f4

jbenet force-pushed the refactor/bitswap branch from 2b80fa2 to ce0d2f4 Compare May 21, 2015 07:08

jbenet added a commit that referenced this pull request May 21, 2015

Merge pull request #1218 from ipfs/refactor/bitswap

5853eac

large refactor of bitswap, implement wantmanager to manage wantlist

jbenet merged commit 5853eac into master May 21, 2015

jbenet removed the status/in-progress In progress label May 21, 2015

jbenet deleted the refactor/bitswap branch May 21, 2015 19:47

wking mentioned this pull request Jun 12, 2015

changelog updates #1360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

large refactor of bitswap, implement wantmanager to manage wantlist #1218

large refactor of bitswap, implement wantmanager to manage wantlist #1218

whyrusleeping commented May 9, 2015

whyrusleeping May 9, 2015

jbenet May 9, 2015

whyrusleeping May 10, 2015

jbenet May 13, 2015

whyrusleeping commented May 9, 2015

jbenet commented May 9, 2015

jbenet May 9, 2015

jbenet commented May 9, 2015

whyrusleeping commented May 9, 2015

whyrusleeping commented May 13, 2015

jbenet May 13, 2015

whyrusleeping May 13, 2015

jbenet May 13, 2015

jbenet commented May 13, 2015

whyrusleeping commented May 13, 2015

jbenet commented May 13, 2015

whyrusleeping commented May 13, 2015

jbenet May 13, 2015

jbenet May 13, 2015

whyrusleeping May 13, 2015

jbenet commented May 13, 2015

whyrusleeping commented May 13, 2015

whyrusleeping commented May 13, 2015

wking May 13, 2015

jbenet commented May 21, 2015

whyrusleeping commented May 21, 2015

jbenet commented May 21, 2015

whyrusleeping commented May 21, 2015

large refactor of bitswap, implement wantmanager to manage wantlist #1218

large refactor of bitswap, implement wantmanager to manage wantlist #1218

Conversation

whyrusleeping commented May 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented May 9, 2015

jbenet commented May 9, 2015

Choose a reason for hiding this comment

jbenet commented May 9, 2015

whyrusleeping commented May 9, 2015

whyrusleeping commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented May 13, 2015

whyrusleeping commented May 13, 2015

jbenet commented May 13, 2015

whyrusleeping commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented May 13, 2015

whyrusleeping commented May 13, 2015

whyrusleeping commented May 13, 2015

Choose a reason for hiding this comment

jbenet commented May 21, 2015

whyrusleeping commented May 21, 2015

jbenet commented May 21, 2015

whyrusleeping commented May 21, 2015