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.

Sign up for GitHub

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

Jump to bottom

Bitswap Rounds CR suggestions #2 #459

Merged
merged 41 commits into from
Dec 17, 2014
Merged

Bitswap Rounds CR suggestions #2 #459

merged 41 commits into from
Dec 17, 2014

Conversation

btc
Copy link
Contributor

@btc btc commented Dec 17, 2014

@whyrusleeping another round of suggestions. Let's keep this one open for the while.

Brian Tiger Chow added 2 commits December 16, 2014 20:28
fix(test): nil Blockstore
7281f66 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
style: line wrapping
260629e 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@btc btc added the status/in-progress In progress label Dec 17, 2014
@btc
Copy link
Contributor Author

btc commented Dec 17, 2014

@whyrusleeping each commit is an atomic change/suggestion.

Brian Tiger Chow added 25 commits December 17, 2014 02:09
fix: move to callsite so public callers don't experience the internal…
32ae1f9 
… timeout rule

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
style constify variables
b188a2c 
good to const until it's required for them to be variable.

TODO pass them in as configuration options
refactor: change Tasks to Outbox
0056361 
notice that moving the blockstore fetch into the manager removes the
weird error handling case.

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor: avoid loop reuse
bb92988 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix: move the check into the function.
4e1818d 
function should be a no-op when passed an empty slice

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor: context first in argument list
0d77e02 
(merely by convention)

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
doc: comment
be87976 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor: taskKey := p.Key() + block.Key()
fda0617 
for clarity and to avoid errors, define a function

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
unexport task and taskList
68ae09f 
the less bitswap has to know about, the easier it'll be for readers.
(This now returns Messages.)

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor: remove peerKey type
9d2c54c 
we've been using maps with peers long enough now that this probably is
no longer necessary

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
add comment to fix race
9259f66 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
perf: avoid lots of communication by signaling once at end of method
3d1d19c 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
it's not a queue yet but it's okay to name it as such
f7310a4 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
tq.Cancel -> tq.Remove
9a6bb16 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
privatize Task
72584ec 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
doc: add comment to Envelope
09ab411 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor: re-use wantlist.Entry type wherever it makes sense
39ee5eb 
it seems to make sense since, in each place, the Key and Priority
represent the same information

b/c you know the saying...

"It is better to have 100 functions operate on one data structure than
10 functions on 10 data structures."

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor: separate responsibilties
37b1f50 
Before, priority carried two pieces of information.

One: priority as defined by remote peer
Two: whether task is trashed

This assumes the protocol is defined for natural numbers instead of
integers. That may not always be the case. Better to leave that
assumption outside so this package isn't coupled to the whims of the
protocol.

The protocol may be changed to allow any integer value to be used.
Hopefully by that time, new responsibilties weren't added to the
Priority variable.

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
mv comment
b23869f 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor: remove ledgerMap type
b0a2ee6 
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>
refactor: put mutex next to the things it protects
87e94f9 
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>
refactor: wantlist splits into WL and ThreadSafe WL
81c0979 
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>
rename to strategy.LedgerManager to decision.Engine
aac777d 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
rm empty file
1a2e9ee 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
rename to peerRequestQueue
d5bb556 
this opens up the possibility of having multiple queues. And for all
outgoing messages to be managed by the decision engine

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
Brian Tiger Chow added 10 commits December 17, 2014 02:11
fix: don't sort the output of Entries()
666ac0b 
only sort SortedEntries()

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
rm unused method
72965e6 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
add comment
167fbac 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
unexport functions
7b93d74 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix: check blockstore before adding task
f5133db 
addresses #438 (comment)

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
log unusual event
61ab0df 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix: set peerset size
6edef72 
addresses #438 (comment)

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
doc: peerset fixme
76889a8 
not changing this because i don't want to write a test for it now

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
refactor: *Entry -> Entry
2c834af 
in many places, entries are assigned from one slice to another and in
different goroutines. In one place, entries were modified (in the
queue). To avoid shared mutable state, probably best to handle entries
by value.

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
extract constants
fd1da4c 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
Brian Tiger Chow added 3 commits December 17, 2014 03:09
refactor(bs/decision.Engine): pass in Entry
07c6a8b 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
fix: add lock to taskQueue
d36c480 
@whyrusleeping may wanna have a look and make sure i didn't screw
anything up here

BenchmarkInstantaneousAddCat1MB-4            200          10763761 ns/op
97.42 MB/s
BenchmarkInstantaneousAddCat2MB-4       panic: runtime error: invalid
memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0xbedd]

goroutine 14297 [running]:
github.com/jbenet/go-ipfs/exchange/bitswap/decision.(*taskQueue).Remove(0xc2087553a0,
        0xc2085ef200, 0x22, 0x56f570, 0xc208367a40)
    /Users/btc/go/src/github.com/jbenet/go-ipfs/exchange/bitswap/decision/taskqueue.go:66
    +0x82
github.com/jbenet/go-ipfs/exchange/bitswap/decision.(*Engine).MessageSent(0xc20871b5c0,
        0x56f570, 0xc208367a40, 0x570040, 0xc208753d40, 0x0, 0x0)
    /Users/btc/go/src/github.com/jbenet/go-ipfs/exchange/bitswap/decision/engine.go:177
    +0x29e
github.com/jbenet/go-ipfs/exchange/bitswap.(*bitswap).send(0xc20871b7a0,
        0x56f4d8, 0xc208379800, 0x56f570, 0xc208367a40,
        0x570040, 0xc208753d40, 0x0, 0x0)
    /Users/btc/go/src/github.com/jbenet/go-ipfs/exchange/bitswap/bitswap.go:352
    +0x11c
github.com/jbenet/go-ipfs/exchange/bitswap.(*bitswap).taskWorker(0xc20871b7a0,
        0x56f4d8, 0xc208379800)
    /Users/btc/go/src/github.com/jbenet/go-ipfs/exchange/bitswap/bitswap.go:238
    +0x165
    created by
    github.com/jbenet/go-ipfs/exchange/bitswap.New
    /Users/btc/go/src/github.com/jbenet/go-ipfs/exchange/bitswap/bitswap.go:66
    +0x49e
doc: some comments about the future of the decision engine
fa6e63c 
License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@btc
Copy link
Contributor Author

btc commented Dec 17, 2014

@whyrusleeping RFCR

Overall, I really love the new design. In the batch job "send wantlist to providers", the de-duplication is well-executed. The priority queue stuff inside the decision engine is awesome too.

Given the above commits, I'd say #438 is LGTM and RFM.

The above commits represent the sum total of my feedback. I'm eager to get yours. I'm especially curious to get your feedback on this commit (discussing the potential future of the priority queue model):

fa6e63c

@btc
Copy link
Contributor Author

btc commented Dec 17, 2014

cc @jbenet

fix: batches of blocks have equal priority
aaa46cc 
addresses...

https://github.com/jbenet/go-ipfs/pull/438/files#r21878994

License: MIT
Signed-off-by: Brian Tiger Chow <brian@perfmode.com>
@whyrusleeping
Copy link
Member

All of this looks good to me!

  • when sending out the wantlists, include cancel requests

My only concern with that, is that cancel requests should be send out as soon as possible to avoid duplicate block send/receives.

  • when handling blockrequests, include sendwantlist and cancel as appropriate

I want to do something along these lines 👍

  • when handling cancel, if we recently received a wanted block from a
    peer, include a partial wantlist that contains a few other high priority
    blocks

👍 to this too.

@whyrusleeping
Copy link
Member

And adding onto that, I was up last night thinking when I should have been sleeping, and I want to give the ledgermanager the ability to request blocks by itself to try and gain good karma on the network. So, if a certain block is being requested frequently, we should request it too so we can rehost it. I think we can put this logic into the LedgerManager at some point.

whyrusleeping added a commit that referenced this pull request Dec 17, 2014
@whyrusleeping
Merge pull request #459 from jbenet/bitswap-rounds-CR-by-btc
00f746b 
Bitswap Rounds CR suggestions #2
@whyrusleeping whyrusleeping merged commit 00f746b into bitswap-rounds Dec 17, 2014
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Dec 17, 2014
@whyrusleeping whyrusleeping deleted the bitswap-rounds-CR-by-btc branch December 17, 2014 19:07
@btc
Copy link
Contributor Author

btc commented Dec 18, 2014

My only concern with that, is that cancel requests should be send out as soon as possible to avoid duplicate block send/receives.

SGTM.

So, if a certain block is being requested frequently, we should request it too so we can rehost it. I think we can put this logic into the LedgerManager at some point.

SGTM

@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
Closed
80 tasks
hacdias added a commit that referenced this pull request Nov 29, 2023
@hacdias
feat!: namesys refactor, ipns TTL bubbled up to gateway (#459)
edfc5ef 
This commit was moved from ipfs/boxo@a50f784
hacdias added a commit that referenced this pull request Nov 29, 2023
@hacdias
feat!: namesys refactor, ipns TTL bubbled up to gateway (#459)
faac7c1 
This commit was moved from ipfs/boxo@a50f784
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@btc @whyrusleeping