WIP: PEX - Peer Exchange#210
Conversation
|
On a quick look from phone, seems like a reasonable thing to have. Code is mostly ok, minor comments so far not worth writing them down until this is finished. Thanks for the early feedback ask, so far so good |
| where | ||
| B: AsRef<[u8]>, | ||
| { | ||
| pub fn added_peers<'a>(&'a self) -> anyhow::Result<Box<dyn Iterator<Item = PexPeerInfo> + 'a>> { |
There was a problem hiding this comment.
I'd avoid allocation here as it's quite easy to do.
two options:
- either use itertools::Either (minimal changes to your existing code, good enough)
- Option is also an iterator, and you can flatten it.
Option 2 would be smth like this:
if self.added.as_ref().map(|added| added.len() % 6 != 0).unwrap_or(false) {
bail!(...)
}
let ips = self.added.as_ref().map(|addrs| addrs.chunk_exact(6)).flatten();
let flags = self.added_f.as_ref().map(|flags| flags.iter().copied()).flatten();
return ips.zip(flags).filter_map(|(ip, flags)| PexPeerInfo::from_bytes(ip, flags).ok())
There was a problem hiding this comment.
added flags - added_f - are optional - so we can have addresses but not flags, in that case zip will not work. But chunks might make things easier - will look into it.
There was a problem hiding this comment.
Look into the PR I put into your repo I've done it already
| { | ||
| // TODO: this is just first attempt at pex - will need more sophistication on adding peers - BEP 40, check number of live, seen peers ... | ||
| if let Ok(peers) = msg.added_peers() { | ||
| peers.for_each(|peer| { |
There was a problem hiding this comment.
Clearly adding eagerly is not good - it takes huge amount of CPU - will need to find something better
There was a problem hiding this comment.
What eats CPU exactly out of this?
There was a problem hiding this comment.
Adding eagerly should be the way to go, just need to fix our workaround the issues
|
I tried to look into it, and it seems totally ok to me. The peers get added, I don't see why this in particular would increase CPU usage. |
| /target | ||
| .DS_Store No newline at end of file | ||
| .DS_Store | ||
| /.vscode No newline at end of file |
There was a problem hiding this comment.
Please don't remove this. It's for consistent linter / prettifier for JS mostly
|
Also profiled in release mode, no CPU issues noticed, works like a charm. On my box, the absolute majority of the CPU is taken by:
The rest is rqbit itself + tokio overhead. |
|
Still see about 150% CPU - it's on big swarm - https://releases.ubuntu.com/24.04/ubuntu-24.04-desktop-amd64.iso.torrent?_gl=1*1bi12tz*_gcl_au*MTA4NjA0NjI5Ny4xNzI0NDkzMjYy&_ga=2.207753554.1199975540.1724493262-752598451.1714415561 Only when torrent is downloading, when stopped, CPU is minimal. Looks like this behaviour is only on this pex branch. |
|
Just checking, can that be that with that branch there's many more live peers that you observe? The CPU would be proportional to the number of live (i.e. connected and communicating) peers usually. Otherwise maybe you can profile with perf or whatever you have on your system to see where's the new hotspot? I didn't see any on the torrent I checked |
|
I'll check that Ubuntu one too |
|
Just tried the ubuntu one. System: macbook air m1 There are 0 hotspots in the new code, all of "on_received_message" is in "on_received_piece".
You can use the newly added "make devserver-profile" command to do all that. |
|
@izderadicka any updates on this? I think I can just go ahead and merge this + izderadicka#1, as it seems useful on its own already for discovering peers even if we don't send our peers yet |
|
Sorry I did not have time to look at - as I said I do have only limited time - I'm still concerned about increase on CPU. |
I addresed this already. Let's do this: I merge this as-is plus my changes on top. If we figure out perf issues we fix them, but I don't see any so far as I mentioned. The PR is useful already on its own and it doesn't need to wait until all PEX features are implemented. I'll merge this one instead #221 which includes your changes |
This is WORK IN PROGRESS (WIP) on adding PEX.
I wanted to share early - to get some feedback if I'm moving in wrong direction.
If this "early draft PR" does not work with project workflow, sorry for that, just close it.
WIP PRs did work for me to have place to discuss and share on particular feature, but if this does not fit this project sorry again.
I thought that PEX could be usefull, and should not be so difficult at least on receiving side. And I can try something bit more substantial .... But I do not have much time left - so it can take quite some time to finish.