Bitswap Refactor #4: Extract session peer manager from sessions #26

hannahhoward · 2018-11-16T00:43:08Z

Goals

Modularize Bitswap in preparation for attempts to optimize bitswap further

This also gets us quite a bit of the way toward the proposed split of bitswap into low level "just ask for blocks from peers and respond to requests" bitswap and high level bitswap -- sessions, and possibly other drivers as time goes on.

Implementation

Final PR includes:

Extracted out the managing of peers for a session into it's own backage -- this way, tracking optimal peers is seperate from improving algorithms for fetch blocks
Commented all exported functions to
Unit test new session manager, new session peermanager
Unit test sessions (they are currently really more integration tested in the main package)

For discussion

child of ipfs/kubo#5723

lanzafame

I understand a fair amount of this is just shuffling code around but I figure if it's being moved around it can get freshened up as well.

On top of the comments made, could all the exported types and functions/methods get a godoc comment, please?

sessionmanager/sessionmanager.go

messagequeue/messagequeue.go

lanzafame · 2018-11-16T03:37:14Z

messagequeue/messagequeue.go

+func (mq *msgQueue) openSender(ctx context.Context) error {
+	// allow ten minutes for connections this includes looking them up in the
+	// dht dialing them, and handshaking
+	conctx, cancel := context.WithTimeout(ctx, time.Minute*10)


The timeout value could potentially be a configuration field of the msgQueue struct? Not sure though

lanzafame · 2018-11-16T03:47:38Z

messagequeue/messagequeue.go

+	return mq.refcnt > 0
+}
+
+func (mq *msgQueue) AddMessage(entries []*bsmsg.Entry, ses uint64) {


session id. I don't make the variable names, I just copy-paste them. :)
But good point and I'll work on some variable names too -- just one at a time.

bitswap.go

messagequeue/messagequeue.go

wantmanager/wantmanager.go

messagequeue/messagequeue.go

sessionmanager/sessionmanager.go

hannahhoward · 2018-12-01T02:43:44Z

@lanzafame @whyrusleeping @michaelavila @eingenito @djdv I believe this PR to be done and would love any feedback you have. FYI, much has changed since the last round of feedback.

Seeking 2 solid LGTMs before merge.

parkan · 2018-12-03T18:23:47Z

we've been hearing a bunch of feedback about bitswap performance from folks using ipfs in production, would love to see this merged so performance work can happen on top of it!

whyrusleeping · 2018-12-03T19:13:42Z

This PR is exceedingly large, which makes review quite difficult. I would not feel comfortable pushing this through without proper review (especially since there are structural changes to core components)

Stebalien · 2018-12-03T19:19:54Z

@djdv I've assigned you to review this as you're also working on bitswap performance. However, if you're swamped, we can find someone else.

hannahhoward · 2018-12-04T02:09:24Z

@whyrusleeping

I appreciate the feedback about the size of the PR. That's fair

I will write up an explanation tomorrow of some design decisions and also do some line by line commenting.

I can also do a video screencast to go over it.

To be clear, while there are a number of changes, my belief is there is only one potential "breaking change" (SessionsForBlock was technically a public bitswap method, even though it's not actually used publicly as far as I know, certainly not in go-ipfs), and only a few changes that go beyond moving code around and restructuring.

whyrusleeping · 2018-12-04T02:22:36Z

@hannahhoward I think this would be much easier if it were many smaller PRs. Each of the individual changes made here could be PRed as they happen, giant sweeping 'refactor' PRs are a great way for bugs to sneak in, especially when the author insists they are 'just moving stuff around'.

For example, the previous Run method has been broken up into smaller pieces, and at the same time, moved to a different file. Reviewing this is quite a bit more time consuming now, as I have to pull up the old code in a separate window, and manually diff it against the newly changed stuff. It seems fine, but it would have been nice to see a simple file rename, and then the changes happen separately.

hannahhoward · 2018-12-04T18:25:21Z

@whyrusleeping it's actually not to late to seperate into separate PRs, and I think that's a reasonable idea. The commit history is fairly linear and most commits have passing tests, so I think that's a reasonable change.

whyrusleeping · 2018-12-04T18:52:26Z

@hannahhoward awesome!

hannahhoward · 2018-12-04T18:53:06Z

@djdv @whyrusleeping @Stebalien @lanzafame @eingenito @michaelavila

I get it -- 20+ fully modified files is a lot to digest, and hard to follow the progress and thinking of.

I've seperated this into 4 seperate PRs. The first I am hoping we can merge fairly quickly, as it only covers what several people have already commented on (plus some fixes for those comments, and the remaining issues are addressed in subsequent PRs):
#28

The next is the seperate of the want manager and peer manager, plus unit tests and further cleanup:
#29

The 3rd is just extracting sessions to a package:
#30

And finally there this PR, which is now just extracting the session peer manager from sessions itself, plus remaining unit tests.

I've set the base of the PR for each branch to the previous branch, and will change the base to master as we merge.

parkan · 2018-12-05T20:01:02Z

took the liberty to rename this PR for consistency and clarity

thank you for putting in the work to make these changes easier to review 💯

extract the job of finding and managing peers for a session from the job of requesting blocks

Add a unit test and do some additional decoupling

Add unit test for sessionpeermanger and comment exported methods

Add a unit test for session package

hannahhoward · 2018-12-13T19:19:00Z

@Stebalien @whyrusleeping @lanzafame @eingenito @michaelavila I know you all already took a look at this one when it was a giant blob, but it's ready to review on it's own as the final PR in this refactor sequence.

eingenito

Ignore my comments - they were existing code and no need to change.

eingenito · 2018-12-14T17:16:51Z

session/cidqueue.go

+		out := cq.elems[0]
+		cq.elems = cq.elems[1:]
+
+		if cq.eset.Has(out) {


This doesn't really matter - but I think Remove no-ops if the target doesn't exist - so might not need the Has call. Totally not something that has to be changed.

eingenito · 2018-12-14T17:18:13Z

session/cidqueue.go

+}
+
+func (cq *cidQueue) Len() int {
+	return cq.eset.Len()


This also totally doesn't matter but you'll Pop() if something is in the set or not - maybe you should take the length of elems?

hannahhoward · 2018-12-17T16:41:13Z

Merging on account of it's been open several days and @eingenito reviewing in depth

Bitswap Refactor ipfs#4: Extract session peer manager from sessions This commit was moved from ipfs/go-bitswap@4fc6272

ghost assigned hannahhoward Nov 16, 2018

ghost added the status/in-progress In progress label Nov 16, 2018

lanzafame reviewed Nov 16, 2018

View reviewed changes

whyrusleeping reviewed Nov 19, 2018

View reviewed changes

bitswap.go Outdated Show resolved Hide resolved

whyrusleeping reviewed Nov 19, 2018

View reviewed changes

messagequeue/messagequeue.go Outdated Show resolved Hide resolved

lanzafame reviewed Nov 19, 2018

View reviewed changes

wantmanager/wantmanager.go Outdated Show resolved Hide resolved

messagequeue/messagequeue.go Outdated Show resolved Hide resolved

sessionmanager/sessionmanager.go Outdated Show resolved Hide resolved

hannahhoward force-pushed the feat/refactor-sessions branch from 18e67bc to 4d5134c Compare November 20, 2018 22:10

hannahhoward force-pushed the feat/refactor-sessions branch 2 times, most recently from 90995eb to a616ddb Compare November 28, 2018 23:30

hannahhoward changed the title ~~WIP: Bitswap Refactor For Cleanliness~~ Bitswap Refactor For Cleanliness Nov 29, 2018

hannahhoward force-pushed the feat/refactor-sessions branch 7 times, most recently from 744f9e4 to b6787e8 Compare December 1, 2018 02:40

Stebalien requested a review from djdv December 3, 2018 19:18

This was referenced Dec 4, 2018

Bitswap Refactor #1: Session Manager & Extract Want Manager #28

Merged

Bitswap Refactor #2: Extract PeerManager From Want Manager + Unit Test #29

Merged

Bitswap Refactor #3: Extract sessions to package #30

Merged

hannahhoward changed the base branch from master to feat/extract-sessions December 4, 2018 18:46

hannahhoward force-pushed the feat/extract-sessions branch from 5eb506e to 604ae5e Compare December 4, 2018 19:48

hannahhoward force-pushed the feat/refactor-sessions branch from b6787e8 to 106b96f Compare December 4, 2018 19:48

parkan changed the title ~~Bitswap Refactor For Cleanliness~~ Bitswap Refactor #4: Extract session manager from sessions Dec 5, 2018

parkan changed the title ~~Bitswap Refactor #4: Extract session manager from sessions~~ Bitswap Refactor #4: Extract session peer manager from sessions Dec 5, 2018

hannahhoward force-pushed the feat/extract-sessions branch from 604ae5e to 40aa1fb Compare December 11, 2018 19:54

eingenito mentioned this pull request Dec 11, 2018

[WIP] Work towards improving the duplicate blocks issue #8

Closed

hannahhoward added 6 commits December 13, 2018 10:52

refactor(sessions): extract peer management

9e89126

extract the job of finding and managing peers for a session from the job of requesting blocks

refactor(session): cleanup sessions

d7a532d

test(sessionmanager): Add unit test

e1a2523

Add a unit test and do some additional decoupling

test(sessionpeermanager): Add unit test

ec47a3d

Add unit test for sessionpeermanger and comment exported methods

test(session): Add unit test

fa93c81

Add a unit test for session package

refactor(session): readability improvements

c5f9a91

hannahhoward force-pushed the feat/refactor-sessions branch from 106b96f to e43c4b8 Compare December 13, 2018 18:55

hannahhoward changed the base branch from feat/extract-sessions to master December 13, 2018 18:56

hannahhoward force-pushed the feat/refactor-sessions branch from e43c4b8 to 0666ee8 Compare December 13, 2018 19:01

test(session): make test more reliable

16f00de

hannahhoward force-pushed the feat/refactor-sessions branch from 0666ee8 to 16f00de Compare December 13, 2018 19:10

eingenito approved these changes Dec 14, 2018

View reviewed changes

hannahhoward merged commit 4fc6272 into master Dec 17, 2018

ghost removed the status/in-progress In progress label Dec 17, 2018

Jorropo pushed a commit to Jorropo/go-libipfs that referenced this pull request Jan 26, 2023

Merge pull request ipfs/go-bitswap#26 from ipfs/feat/refactor-sessions

b99c614

Bitswap Refactor ipfs#4: Extract session peer manager from sessions This commit was moved from ipfs/go-bitswap@4fc6272

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bitswap Refactor #4: Extract session peer manager from sessions #26

Bitswap Refactor #4: Extract session peer manager from sessions #26

hannahhoward commented Nov 16, 2018 •

edited

Loading

lanzafame left a comment

lanzafame Nov 16, 2018

lanzafame Nov 16, 2018

hannahhoward Nov 16, 2018

hannahhoward commented Dec 1, 2018

parkan commented Dec 3, 2018

whyrusleeping commented Dec 3, 2018

Stebalien commented Dec 3, 2018

hannahhoward commented Dec 4, 2018

whyrusleeping commented Dec 4, 2018

hannahhoward commented Dec 4, 2018

whyrusleeping commented Dec 4, 2018

hannahhoward commented Dec 4, 2018

parkan commented Dec 5, 2018

hannahhoward commented Dec 13, 2018

eingenito left a comment

eingenito Dec 14, 2018

eingenito Dec 14, 2018

hannahhoward commented Dec 17, 2018

Bitswap Refactor #4: Extract session peer manager from sessions #26

Bitswap Refactor #4: Extract session peer manager from sessions #26

Conversation

hannahhoward commented Nov 16, 2018 • edited Loading

Goals

Implementation

For discussion

lanzafame left a comment

Choose a reason for hiding this comment

lanzafame Nov 16, 2018

Choose a reason for hiding this comment

lanzafame Nov 16, 2018

Choose a reason for hiding this comment

hannahhoward Nov 16, 2018

Choose a reason for hiding this comment

hannahhoward commented Dec 1, 2018

parkan commented Dec 3, 2018

whyrusleeping commented Dec 3, 2018

Stebalien commented Dec 3, 2018

hannahhoward commented Dec 4, 2018

whyrusleeping commented Dec 4, 2018

hannahhoward commented Dec 4, 2018

whyrusleeping commented Dec 4, 2018

hannahhoward commented Dec 4, 2018

parkan commented Dec 5, 2018

hannahhoward commented Dec 13, 2018

eingenito left a comment

Choose a reason for hiding this comment

eingenito Dec 14, 2018

Choose a reason for hiding this comment

eingenito Dec 14, 2018

Choose a reason for hiding this comment

hannahhoward commented Dec 17, 2018

hannahhoward commented Nov 16, 2018 •

edited

Loading