-
Notifications
You must be signed in to change notification settings - Fork 170
Conversation
This is originally from the non-merging branch by David in rs-ipfs#126. Co-authored-by: David Craven <[email protected]>
This is originally from the non-merging branch by David in rs-ipfs#126. Co-authored-by: David Craven <[email protected]>
This is originally from the non-merging branch by David in #126. Co-authored-by: David Craven <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filling out a review so that my comments can be in a single place.
1. I think there are a lot of interesting ideas here
2. It would be much easier for me to review this if the PR was smaller and more singular
I originally commented privately that this, and the previous #119 were good "spike(s)" but the wideness makes it difficult to even start discussing whether or not parts are something we should merge. As I already mentioned it is quite difficult to find any single idea to discuss because there simply are too many to enumerate. I believe you've come to the same conclusion because the PR description is empty. For practical tips with PR's in general I'd like to link to ploeh's blog post on the topic.
To enumerate some ideas which were interesting I can remember from top of my head:
- making Bitswap behaviour bubble up events
- pushing more Bitswap functionality to ledger to produce fewer separate messages (I think though that this may have had a bug with how often the messages were created in time and how big the messages could had been, but very likely I missed something)
- switching away from
anyhow
tothiserror
- perhaps this could be done from bitswap first
- then followed with other leafs
- eventually phased out anyhow
- replacing and splitting of
SubscriptionRegistry
was surprising, perhapstokio::sync::watch
would work better than a custom version? I didn't understand the split fully either way and remember trying to ask privately the reason behind it without an answer.
I am quite curious if putting everything to work around a single stream wrapping a libp2p swarm, I'd like to see some pros and cons. Is this design used in any existing libp2p based library yet? I'd definitely see working away our ipfs::Ipfs<T: IpfsTypes>
as a plus but that'd of course need to have more significant long term pros as well related to concurrency. Limiting needless concurrency can be really good for correctness, especially if we can at the same time actually complete a bounded number of reads and writes. Benchmarks would probably help here as well.
it's not about limiting concurrency. it's that it's not really concurrent in the first place, we don't spawn any tasks other than in the strategy (and the tasks that libp2p spawns internally), and the tasks spawned in the strategy are simply overengineering as currently the strategy doesn't do anything interesting. any long running tasks should be spawned (the block store actually is spawning tasks now making it concurrent). it's essentially the same design as a warp or any other http server. their design is for every request a task is spawned. the ipfs handle is the client to the server, which spawns tasks when appropriate. |
Essentially what this pr tries to accomplish was the following:
with the better understanding I gained from trying to implement this these things slightly changed, but the ideas are the same |
the way I was planning to increase concurrency in the ipld-deamon is by using the fact that file systems are already concurrent. a client could read a block from the file system and cache it in memory directly. a request to to the ipld-deamon would only be needed to write a block, with the corresponding buffer to minimize writes to disk by only writing to disk on flush or when a block is not found locally request it to be found with bitswap. that design however was decided not to be pursued at the beginning of the grant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, glad this change was finally made
decouple the strategy from bitswap
No description provided.