Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

Receive swarm notifications synchronously #547

Closed
guseggert opened this issue Dec 14, 2021 · 6 comments · Fixed by #565
Closed

Receive swarm notifications synchronously #547

guseggert opened this issue Dec 14, 2021 · 6 comments · Fixed by #565
Labels
need/triage Needs initial labeling and prioritization
Milestone

Comments

@guseggert
Copy link
Contributor

We need to understand the impact of libp2p/go-libp2p-swarm#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.

@guseggert guseggert added the need/triage Needs initial labeling and prioritization label Dec 14, 2021
@ipfs ipfs deleted a comment from welcome bot Dec 14, 2021
@marten-seemann
Copy link
Member

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

@BigLep
Copy link

BigLep commented Apr 1, 2022

@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
Copy link

BigLep commented Apr 1, 2022

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
Copy link
Member

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?

libp2p/go-libp2p-swarm#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
Copy link

BigLep commented Apr 1, 2022

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
Copy link

BigLep commented Apr 1, 2022

Tracking issue for overhaul: ipfs/boxo#73

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

@BigLep BigLep linked a pull request Jun 1, 2022 that will close this issue
Repository owner moved this from 🥞 Todo to 🎉 Done in IPFS Shipyard Team Jun 13, 2022
@BigLep BigLep moved this from 🎉 Done to ☑️ Done (Archive) in IPFS Shipyard Team Jun 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
need/triage Needs initial labeling and prioritization
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants