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

feat(bitswap.decision.Engine) use PriorityQueue for Engine.Outbox #568

Merged
merged 8 commits into from
Jan 18, 2015

Conversation

btc
Copy link
Contributor

@btc btc commented Jan 15, 2015

This PR makes bitswap's decision engine more intelligent. Now, incoming block requests are pushed into a priority queue and sorted using a pluggable comparator.

The default comparator (provisionally-titled V1) is defined as follows:

// V1 respects the target peer's wantlist priority. For tasks involving
// different peers, the oldest task is prioritized.
var V1 = func(a, b *peerRequestTask) bool {
    if a.Target == b.Target {
        return a.Entry.Priority > b.Entry.Priority
    }
    return FIFO(a, b)
}

// FIFO is a basic task comparator that returns tasks in the order created.
var FIFO = func(a, b *peerRequestTask) bool {
    return a.created.Before(b.created)
}

The engine interface remains the same:

func NewEngine(ctx context.Context, bs bstore.Blockstore) *Engine

// MessageReceived performs book-keeping. Returns error if passed invalid
// arguments.
func (e *Engine) MessageReceived(p peer.ID, m bsmsg.BitSwapMessage) error

func (e *Engine) MessageSent(p peer.ID, m bsmsg.BitSwapMessage) error

func (e *Engine) Outbox() <-chan Envelope

func (e *Engine) Peers() []peer.ID
    Returns a slice of Peers with whom the local node has active sessions

type Envelope struct {
    // Peer is the intended recipient
    Peer peer.ID
    // Message is the payload
    Message bsmsg.BitSwapMessage
}
    Envelope contains a message for a Peer

It's built ontop of a generic pq that could probably be used elsewhere:

package pq
    import "."


TYPES

type Elem interface {
    // SetIndex stores the int index.
    SetIndex(int)
    // Index returns the last given by SetIndex(int).
    Index() int
}
    Elem describes elements that can be added to the PQ. Clients must
    implement this interface.

type ElemComparator func(a, b Elem) bool
    ElemComparator returns true if pri(a) > pri(b)

type PQ interface {
    // Push adds the ele
    Push(Elem)
    // Pop returns the highest priority Elem in PQ.
    Pop() Elem
    // Len returns the number of elements in the PQ.
    Len() int
    // Update `fixes` the PQ.
    Update(index int)
}
    PQ is a basic priority queue.

func New(cmp ElemComparator) PQ
    New creates a PQ with a client-supplied comparator.

@btc btc added the status/in-progress In progress label Jan 15, 2015
@whyrusleeping
Copy link
Member

I was planning a refactor in bitswap that would hold a queue for each partner, and then hold a priority queue of the partners to decide who we service next. The idea behind it is that we need to try and avoid 'slow' peers holding up the entire system.

Thats not to say that this interferes with that in any way, just letting you know my plans.

@btc
Copy link
Contributor Author

btc commented Jan 15, 2015

RFCR @jbenet @whyrusleeping

@jbenet jbenet modified the milestone: α Jan 15, 2015
e.peerRequestQueue.Push(entry.Entry, p)
newWorkExists = true
Copy link
Member

Choose a reason for hiding this comment

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

is there a strong reason to order these? if so maybe comment it? at first glance i wouldn't know and maybe screw up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't. I switched the order because I mistakenly believed that assigning to the variable would magically signal to the other goroutine immediately. In the moment, I had forgotten how to write software.

Copy link
Member

Choose a reason for hiding this comment

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

❤️ 👍

@jbenet
Copy link
Member

jbenet commented Jan 16, 2015

@briantigerchow RFM (though see comment above)

@btc btc force-pushed the feat/bitswap-decision-pq branch 2 times, most recently from 6d5891b to 54bd592 Compare January 18, 2015 22:16
Brian Tiger Chow added 8 commits January 18, 2015 15:09
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>

Conflicts:
	exchange/bitswap/decision/taskqueue.go
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
	refactor: peerRequestQueue

	it's a mistake to make one queue to fit all. Go's lack of algebraic
	types turns a generalized queue into a monstrosity of type
	checking/casting. Better to have individual queues for individual
	purposes.

	Conflicts:
		exchange/bitswap/decision/bench_test.go
		exchange/bitswap/decision/tasks/task_queue.go

	fix(bitswap.decision.PRQ): if peers match, always return result of pri comparison

	fix(bitswap.decision.Engine): push to the queue before notifying

	TOCTOU bug

	1. client notifies
	2. worker checks (finds nil)
	3. worker sleeps
	3. client pushes (worker missed the update)

	test(PQ): improve documentation and add test

	test(bitswap.decision.Engine): handling received messages

	License: MIT
	Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@btc
Copy link
Contributor Author

btc commented Jan 18, 2015

Since RFM, exchange/bitswap/decision/pq package has been extracted to thirdparty/pq

Merging.

cc @jbenet @whyrusleeping

btc pushed a commit that referenced this pull request Jan 18, 2015
feat(bitswap.decision.Engine) use PriorityQueue for Engine.Outbox
@btc btc merged commit d0e4cdf into master Jan 18, 2015
@btc btc removed the status/in-progress In progress label Jan 18, 2015
@btc btc deleted the feat/bitswap-decision-pq branch January 18, 2015 23:15
@btc btc removed their assignment Mar 30, 2015
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