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

[WIP] Refactor to use new futures api #2612

Closed
wants to merge 1 commit into from

Conversation

afinch7
Copy link
Contributor

@afinch7 afinch7 commented Jul 4, 2019

Currently depends on a alpha version of futures-preview, but with std::future now on stable we should get a stable release of futures 0.3 soon. Normally I would say wait on something like this, but #2385 is going to need this change to properly support async ops.

@afinch7 afinch7 force-pushed the rust_futures_api branch 3 times, most recently from f0e3f4e to 9286854 Compare July 12, 2019 22:34
@afinch7 afinch7 marked this pull request as ready for review July 12, 2019 22:34
@afinch7 afinch7 force-pushed the rust_futures_api branch from 9286854 to 3aab4b1 Compare July 12, 2019 22:39
cli/http_util.rs Show resolved Hide resolved
@afinch7 afinch7 force-pushed the rust_futures_api branch from 3aab4b1 to f980a97 Compare July 16, 2019 16:16
cli/resources.rs Outdated Show resolved Hide resolved
@95th
Copy link
Contributor

95th commented Jul 18, 2019

Can we temporarily switch to nightly for some time and move the code to async await?

cc @ry

@ry
Copy link
Member

ry commented Jul 18, 2019

Can we temporarily switch to nightly for some time and move the code to async await?

You can do that in a branch :)

Not in master. I don't want to open that can of worms.

@95th
Copy link
Contributor

95th commented Jul 18, 2019

In that case, given the scale of refactoring we're doing at the moment, I think it is not worth it to maintain another branch. We'll do it when async-await is in stable

@afinch7
Copy link
Contributor Author

afinch7 commented Jul 18, 2019

Here is a list of dependencies, and their respective statuses.

Dependent on tokio:

  • tokio-process(getting merged into tokio) - waiting on stable release
  • hyper - waiting on alpha release (using git for now)
  • hyper-rustls - waiting on patch to be started(dependent on
    others)

I think that is everything let me know if there is something I missed. Also thoughts on runtime?

@afinch7
Copy link
Contributor Author

afinch7 commented Jul 18, 2019

I manged to get deno_tcp working, so here are some results so far. I picked the median of 3 runs for both. Don't mind the terrible tail latency. I think that has more to do with my hardware than anything.

This branch:

Running 10s test @ http://127.0.0.1:4500/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     0.91ms    5.76ms 102.88ms   98.33%
    Req/Sec    14.77k     1.69k   16.43k    96.00%
  293777 requests in 10.00s, 14.29MB read
Requests/sec:  29369.49
Transfer/sec:      1.43MB

Master:

Running 10s test @ http://127.0.0.1:4500/
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     0.91ms    5.81ms 104.34ms   98.42%
    Req/Sec    14.55k     1.67k   16.07k    96.00%
  289246 requests in 10.00s, 14.07MB read
Requests/sec:  28920.91
Transfer/sec:      1.41MB

Within margin of error, but still about ~2-3% in favor of this branch.

@ry
Copy link
Member

ry commented Jul 18, 2019

@afinch7 That's good to hear!

@95th
Copy link
Contributor

95th commented Aug 22, 2019

Here is a list of dependencies, and their respective statuses.

Think most of the stuff can be checked except hyper-rustls (Perhaps use hyper-tls)?

@afinch7
Copy link
Contributor Author

afinch7 commented Aug 22, 2019

Here is a list of dependencies, and their respective statuses.

Think most of the stuff can be checked except hyper-rustls (Perhaps use hyper-tls)?

Yes I think most of the dependencies are ready, but async/await has been delayed to 1.39.0 that's really the only thing I'm still waiting on here.

@ry
Copy link
Member

ry commented Oct 2, 2019

Old PR, closing.

We will move to the new futures API, but ideally there would be a stable release of Tokio using this new future API first. We need Tokio to make a stable release using the new Futures API first.

@ry ry closed this Oct 2, 2019
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.

3 participants