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

Use FuturesUnordered to track ops, fix latency issues #2131

Merged
merged 3 commits into from Apr 16, 2019

Conversation

3 participants
@piscisaureus
Copy link
Collaborator

commented Apr 16, 2019

Additionally, instead of polling ops in a loop until none of them are
ready, the isolate will now yield to the task system after delivering
the first batch of completed ops to the javascript side.

Although this makes performance a bit worse (about 15% fewer
requests/second on the 'deno_core_http_bench' benchmark), we feel that
the advantages are worth it:

  • It resolves the extremely high worst-case latency that we were seeing
    on deno_core_http_bench, in particular when using the multi-threaded
    Tokio runtime, which would sometimes exceed a full second.

  • Before this patch, the implementation of Isolate::poll() had to loop
    through all sub-futures and poll each one of them, which doesn't scale
    well as the number of futures managed by the isolate goes up. This
    could lead to poor performance when e.g. a server is servicing
    thousands of connected clients.

piscisaureus added some commits Apr 8, 2019

core: run isolate tests within a task
This change is made in preparation for using FuturesUnordered to track
futures that are spawned by the isolate. FuturesUnordered sets up
notififications for every future that it finds to be not ready when
polled, which causes a crash if attempted outside of a task context.
core: make Isolate use FuturesUnordered to track ops
Additionally, instead of polling ops in a loop until none of them are
ready, the isolate will now yield to the task system after delivering
the first batch of completed ops to the javascript side.

Although this makes performance a bit worse (about 15% fewer
requests/second on the 'deno_core_http_bench' benchmark), we feel that
the advantages are worth it:

* It resolves the extremely high worst-case latency that we were seeing
  on deno_core_http_bench, in particular when using the multi-threaded
  Tokio runtime, which would sometimes exceed a full second.

* Before this patch, the implementation of Isolate::poll() had to loop
  through all sub-futures and poll each one of them, which doesn't scale
  well as the number of futures managed by the isolate goes up. This
  could lead to poor performance when e.g. a server is servicing
  thousands of connected clients.

@piscisaureus piscisaureus force-pushed the piscisaureus:core_latency branch from 5bbec76 to 7807afa Apr 16, 2019

use futures::future::lazy;
use futures::future::ok;
use futures::Async;
use std::ops::FnOnce;

This comment has been minimized.

Copy link
@ry

ry Apr 16, 2019

Collaborator

props for not importing tokio

@ry

ry approved these changes Apr 16, 2019

Copy link
Collaborator

left a comment

LGTM

It would be cool to have all the text your wrote in the description of this PR in the commit message.

@piscisaureus

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 16, 2019

It would be cool to have all the text your wrote in the description of this PR in the commit message.

I did already

@piscisaureus piscisaureus merged commit 7807afa into denoland:master Apr 16, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@piscisaureus piscisaureus deleted the piscisaureus:core_latency branch Apr 16, 2019

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Benchmarks impacted by this patch:

Screen Shot 2019-04-16 at 3 35 29 PM

Screen Shot 2019-04-16 at 3 35 38 PM

@ry

This comment has been minimized.

Copy link
Collaborator

commented Apr 17, 2019

@kevinkassimo the latency wins from this patch should be shown too

Screen Shot 2019-04-17 at 11 33 41 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.