Extend peer task queue to work with want-have / want-block

[dirkmc]

The Bitswap proof-of-concept

• supports new message types
• pushes requests onto the request queue individually (instead of in groups)
• pulls requests off the request queue by size (instead of in pre-pushed groups)

This PR extends peer task queue to support Bitswap's requirements and

• orders peers by outstanding bytes (rather than number of blocks)

See ipfs/go-bitswap#197

[Stebalien]

LGTM. The only things that really need to be fixed are the documentation issues.

Otherwise, I'd like to give @hannahhoward a chance to review.

[hannahhoward]

Approve on the condition yall do the Graphsync update -- which I don't think will be too hard.

I am curious how this will affect allocations... I believe that part of the point of the old architecture was essentially to avoid every reallocating task blocks or changing the list order... hence the someone odd tactic of marking task blocks as garbage rather than removing them from the queue, until they got to the front. That always felt like possibly premature optimization but I don't know the history -- @Stebalien might.

[Stebalien]

hence the someone odd tactic of marking task blocks as garbage rather than removing them from the queue, until they got to the front.

So:

1. Queuing multiple tasks at once was actually added pretty recently (well after the task deletion logic). I believe we chose to group tasks on push instead of pop because it was convenient (I think?).
2. In terms of marking tasks as deleted versus removing them up-front, I'm not sure what the logic was. Any operation on the queue will be O(log(len(queue))). Really, the sooner we remove elements, the smaller the queue stays and the less work we do.

So yeah, probably premature (and possibly incorrect) optimization.

[dirkmc]

go-ipfs-pq has this cryptic comment:

	// TODO  explain why this interface should not be extended
	// It does not support Remove. This is because...

My assumption was that it doesn't support Remove() because removing from a heap is not as efficient as popping from the head of the heap (the heap is optimized to pop the head). Therefore it's more efficient to simply mark as removed and discard once it gets to the head. Just a guess though.

Perhaps we should add a Removed flag to the Task, and the heap sort function should move removed tasks to the front of the queue so that they are popped first (to be able to GC as soon as possible).

[Stebalien]

My assumption was that it doesn't support Remove() because removing from a heap is not as efficient as popping from the head of the heap (the heap is optimized to pop the head). Therefore it's more efficient to simply mark as removed and discard once it gets to the head. Just a guess though.

I seem to recall asking Why about this and he couldn't remember. As far as I know, Remove and Pop are both O(log(n)) so it shouldn't matter. (see point two in my previous comment)

[Stebalien]

Ah, I guess remove may have to do ~2log(n) operations (in practice)? It still seems worth it to remove items as fast as we can instead so n doesn't grow.

[dirkmc]

Looking at the source code that is correct, Remove may do twice as many operations, depending on where in the heap the removed item is.

In any case I agree that overall it's going to be either a negligible difference or maybe even net-positive to have Remove(). I'll add a PR against go-ipfs-pq.

[dirkmc]

ipfs/go-ipfs-pq#5

[Stebalien]

LGTM. Could you submit a PR against go-graphsync before we merge this?

[Stebalien]

Well, LGTM except the tests are failing.

[dirkmc]

Created PR against go-graphsync: ipfs/go-graphsync#44