Bitswap rounds #438

whyrusleeping · 2014-12-10T02:09:18Z

This PR implements the first revision of 'Bitswap Rounds' as defined by @jbenet in issue #417

jbenet · 2014-12-10T02:12:06Z

exchange/bitswap/bitswap.go

@@ -29,6 +30,8 @@ const maxProvidersPerRequest = 3
 const providerRequestTimeout = time.Second * 10
 const hasBlockTimeout = time.Second * 15

+const roundTime = time.Second / 2


maybe all these consts should be related to each other:

const roundTime = time.Second / 2 const providerRequestTimeout = roundTime * 20 const hasBlockTimeout = roundTime * 30

Also, note you can probably set these down 10 or 100x for tests. (though... timeouts in other subsystems may affect this. it may be worth exploring the instrumented Signals idea, and maybe configuring timeouts in one place.

btc · 2014-12-10T02:15:06Z

https://build.protocol-dev.com/job/gotest/124/console

jbenet · 2014-12-10T02:15:21Z

exchange/bitswap/bitswap.go

 	}
 	wg.Wait()
 }

+func (bs *bitswap) roundWorker(ctx context.Context) {
+	roundTicker := time.NewTicker(roundTime)
+	bandwidthPerRound := 500000


bandwidthPerRound should be a const/var up at top.

slipped my mind, thanks

and include unit eg bytesPerRound

if all blocks are larger than 500K, will anything ever be sent?

whyrusleeping · 2014-12-10T02:18:33Z

jenkins is angry because i forgot to make vendor...

btc · 2014-12-10T02:19:18Z

exchange/bitswap/message/message.go

-	// implies Priority(A) > Priority(B)
-	AddWanted(u.Key)
+	// AddEntry adds an entry to the Wantlist.
+	AddEntry(u.Key, int, bool)


it's not obvious what bools mean in parameter lists

type Type int const ( Normal = iota Revoke/Cancel )

or named params:

AddEntry(key u.Key, priority int, cancel bool)

(a thought that occurred to me is we could use priority = 0 but this type of overloading can lead to problems later)

jbenet · 2014-12-10T03:07:41Z

Large problem: right now, with massive bandwidth allocation, this means that actually sending out all the data for each round will take longer that the round timeout.

On one hand we can let round duration be determined by send time (just keep looping, and sleep while there is nothing to send (empty wantlists, we can use a condvar)).

On the other hand, if we do, one slow peer may slow everything down, because if we wait for the network, transport to be done sending, we could be waiting a long time. (waiting for the transport is not something we do currently (we buffer inside channels) but we may want to go that route (by shrinking the buffers) because otherwise rounds will be allocated one after another really fast and everything will just hang on the pipes.

Solution is non trivial.

btc · 2014-12-10T07:24:06Z

may want to rebase this out before the branch gets big and it becomes more difficult to do
https://build.protocol-dev.com/job/all-the-tests/74/

btc · 2014-12-11T00:59:46Z

How is the add/cat use case? Any issues with files > 100 MB?

btc · 2014-12-11T01:04:20Z

(squashed the vendor fix so all commits build)

it's only used in two places, but i think we've been using maps on IPFS types so much now that the specificity is no longer necessary License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

@whyrusleeping

If we put the lock next to the fields it protects, it can sometimes make it easier to reason about threadsafety. In this case, it reveals that the task queue (not threadsafe) isn't protected by the mutex, yet shared between the worker and callers. @whyrusleeping License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

bitswap keeps the threadsafe version. observing the ledger shows that it doesn't need it anymore (ledgermanager is protected and safe). License: MIT Signed-off-by: Brian Tiger Chow <brian@perfmode.com>