search

ipfs / go-bitswap

The golang implementation of the bitswap protocol
MIT License
216 stars 112 forks source link

Receive swarm notifications synchronously #547

Closed guseggert closed 2 years ago

guseggert commented 2 years ago

We need to understand the impact of https://github.com/libp2p/go-libp2p-swarm/pull/295, which would switch to synchronous swarm notifications, which is theoretically better. We need to scope the effort of the code change, its impact on perf and resource consumption, and look for other risks.

marten-seemann commented 2 years ago

Bitswap should probably not use swarm notifications at all, but subscribe to the EvtPeerConnectednessChanged event.

BigLep commented 2 years ago

@guseggert @aschmahmann : do you have a sense of how much of this work is blocked on "what are we going to do about our go-bitswap" decisions, or can this be executed on safely independent of larger conversations.

BigLep commented 2 years ago

2022-04-01 conversation: it helps with memory but there were also synchronous operations that were getting locked from creating libp2p connections.

We'll tackle this when we get more into the "fix bitswap" lair.

Someone is going to need to study this for a ~day. We'd rather fix other bitswap bugs.

marten-seemann commented 2 years ago

it helps with memory but there were also synchronous operations that were getting locked from creating libp2p connections.

I don't understand this sentence. Can you please clarify?

https://github.com/libp2p/go-libp2p-swarm/pull/295 is schedule for the go-libp2p v0.19.0 release. go-libp2p is a library used by many projects, and we can't stop progress indefinitely because one downstream user (in an unmaintained repo!) is blocking us.

BigLep commented 2 years ago

Let me write that comment out again as I see my comment doesn't read out well. It was an attempt to quickly catch things that were happening in a verbal conversation, but I should have phrased it as such and made a note to come back to it and clean it up.

go-ipfs Steward perspective: Doing this issue is good as it helps with memory utilization (which has been a problem with go-bitswap) and we believe it may address the locking that we've seen occur which has prevented go-bitswap from making additional libp2p connections.

The unfortunate part is that it requires code changes in the hot-paths of the code that we've made previous attempts to debug/untangle/solve unsuccessfully so far. As a result, this will need to be done with care.

There is a lot of changes we need to do to bitswap this year to get out of the availability and performance issues we've seen and support new functionality being proposed this year. The team believes this work should be accounted for in light of the bigger changes rather than doing this one off in known riskier areas of the code.

I'll come back with the tracking issue for this work.

Let's also sync in terms of how this impacts libp2p.

BigLep commented 2 years ago

Tracking issue for overhaul: https://github.com/ipfs/go-libipfs/issues/73

Conversation about how this impacts go-libp2p is happening #ipfs-dev: https://discord.com/channels/806902334369824788/942673321852563456/959571857831514162