Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to tokio, and update dependencies #589

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Vrtgs
Copy link

@Vrtgs Vrtgs commented May 14, 2024

implement the #588 suggestion

@bee-san
Copy link
Member

bee-san commented May 15, 2024

Thank you so much for this! We take performance seriously here. I'll have to read up more on Tokio vs STD:ASYNC but I think you're right, Tokio in my opinion would be faster now-a-days :)

@Vrtgs
Copy link
Author

Vrtgs commented May 15, 2024

opps forgot to make a new branch, sorry for the extra pushes

@bergabman
Copy link
Contributor

I'd like to add some things regarding the arguments in issue #588. It's the PR related to it, and I figured we could keep the discussion in one place.
First some context; Back in the day of creating rustscan the situation was slightly different. Tokio had a different api, structure, that was inconsistent with the rust ecosystem, and async-std promised a way more consistent, rust std feel for their async implementation. However this has all changed when async await got stabilized and tokio got completely rewritten. Looking at how async-std got no update in the past 2 years, it makes a whole lot more sense now to switch to tokio, so I agree on that point.

Considering performance, the scan engine was built on top of FuturesUnordered, which is a more generic futures implementation and not part of async-std, it's part of the futures crate. The reason for this decision was performance, and to not rely on async-std in performance critical parts of RustScan. As async await was just stabilized back then, we knew things would change, supporting the decision to stick with FuturesUnordered.
As @bee-san stated, performance is critical. I noticed that you replaced FuturesUnordered with JoinSet from tokio in the scan engine. L81 Based on some discussions in the tokio repo tokio-rs/tokio#5564, this decision would set us back with 5-18% in performance in general, because of a design decision in the implementation of JoinSet.
I haven't done a performance comparison myself, but it would be benficial to see if there is actually a performance difference between the 2 implementations in our case. Based on the benchmarks, and the discussion in that issue, I would prefer to stay with FuturesUnordered, but if the benchmark shows different, we can go with JoinSet, completely remove the futures dependency and use tokio everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants