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

Make deno multithreaded #782

Merged
merged 6 commits into from Sep 25, 2018

Conversation

2 participants
@ry
Copy link
Collaborator

ry commented Sep 21, 2018

No description provided.

@ry ry force-pushed the ry:default_runtime branch 5 times, most recently from 2388d35 to 18266ba Sep 21, 2018

@ry ry force-pushed the ry:default_runtime branch 3 times, most recently from 9d95bee to 10448cd Sep 22, 2018

@ry ry referenced this pull request Sep 22, 2018

Closed

Use a custom Tokio runtime #778

@ry ry force-pushed the ry:default_runtime branch from 10448cd to 7a9d2ef Sep 22, 2018

@ry ry changed the title [WIP] Use default runtime in tokio Make deno multithreaded Sep 22, 2018

@ry ry force-pushed the ry:default_runtime branch from 7a9d2ef to 253c5bf Sep 22, 2018

pub fn event_loop(&self) {
// Main thread event loop.
loop {
match self.rx.try_recv() {

This comment has been minimized.

@piscisaureus

piscisaureus Sep 22, 2018

Collaborator

I think this can go.

This comment has been minimized.

@ry

ry Sep 22, 2018

Author Collaborator

I tried it.. but it fails on 001_hello.js - which is a program that doesn't send any messages. So it blocks forever trying to receive something.

    loop {
          let box_u8 = self.rx.recv().unwrap();
          if self.state.is_idle() {
            break;
          } else {
            // There are futures, so block main thread waiting for a new
            // message (a response from a future).
            self.send(box_u8);
          }
    }

This comment has been minimized.

@piscisaureus

piscisaureus Sep 22, 2018

Collaborator

did you try:

    while !self.state.is_idle() {
          let box_u8 = self.rx.recv().unwrap();
          self.send(box_u8);
    }

This comment has been minimized.

@ry

ry Sep 22, 2018

Author Collaborator

Thanks - that works.

@ry ry force-pushed the ry:default_runtime branch 2 times, most recently from 07848c7 to e036a68 Sep 22, 2018

@ry ry referenced this pull request Sep 24, 2018

Closed

Implemented deno.readDirSync #652

@ry ry force-pushed the ry:default_runtime branch from e036a68 to 82e9b62 Sep 24, 2018


pub extern "C" fn msg_from_js(i: *const isolate, buf: deno_buf) {
let bytes = unsafe { std::slice::from_raw_parts(buf.data_ptr, buf.data_len) };
// Hopefully Rust optimizes this away.

This comment has been minimized.

@piscisaureus

piscisaureus Sep 24, 2018

Collaborator

That seems a little unlikely. If you returned an &'static Buf it probably would be. But I don't think it's all that important.

This comment has been minimized.

@ry

ry Sep 24, 2018

Author Collaborator

Box is always 'static. I doubt this is anything that will ever show up in a profile, I'd rather not complicate the function now without doing benchmarks.

pub argv: Vec<String>,
pub flags: flags::DenoFlags,
ntasks: Mutex<i32>,

This comment has been minimized.

@piscisaureus

piscisaureus Sep 24, 2018

Collaborator

I don't think the tasks count should be modified from any thread other than the main thread.

If this PR does that, I object to it: the code that calls is_idle() cannot conceptually be made thread safe w.r.t. the ntasks counter (imagine that you call .recv() and then another thread decrements the tasks counter - you'd have a hanging application).

So this mutex is unnecessary.

This comment has been minimized.

@ry

ry Sep 24, 2018

Author Collaborator

Fixed in ab50c33

isolate.set_response(buf);
}
} else {
// Execute op asynchornously.

This comment has been minimized.

@piscisaureus

piscisaureus Sep 24, 2018

Collaborator

"asynchronously"

This comment has been minimized.

@ry

ry Sep 24, 2018

Author Collaborator

Fixed.

@piscisaureus
Copy link
Collaborator

piscisaureus left a comment

See comments. Other than that, LGTM.

@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Sep 24, 2018

You should demand a test that calls Isolate dispatch : )
I will update with that soon.

@ry ry force-pushed the ry:default_runtime branch 4 times, most recently from f4e2e28 to 4c5a9fd Sep 24, 2018

@ry ry referenced this pull request Sep 25, 2018

Closed

Some random clean ups #823

@ry ry force-pushed the ry:default_runtime branch from 2b5270f to 90a080f Sep 25, 2018

@ry ry force-pushed the ry:default_runtime branch from 90a080f to 98bf90f Sep 25, 2018

ry added a commit to ry/deno that referenced this pull request Sep 25, 2018

Run rust tests sequentially.
Working around a bug that I think has been here a while, but
is somehow made more promentent with denoland#782.

@ry ry force-pushed the ry:default_runtime branch from 255790c to f68f1c0 Sep 25, 2018

ry added a commit to ry/deno that referenced this pull request Sep 25, 2018

Run rust tests sequentially.
Working around a bug that I think has been here a while, but
is somehow made more promentent with denoland#782.

@ry ry force-pushed the ry:default_runtime branch from f68f1c0 to f40f247 Sep 25, 2018

ry added a commit to ry/deno that referenced this pull request Sep 25, 2018

Run rust tests sequentially.
Working around a bug that I think has been here a while, but
is somehow made more promentent with denoland#782.

@ry ry force-pushed the ry:default_runtime branch from f40f247 to 7462e4c Sep 25, 2018

ry added a commit to ry/deno that referenced this pull request Sep 25, 2018

Run rust tests sequentially.
Working around a bug that I think has been here a while, but
is somehow made more promentent with denoland#782.

@ry ry force-pushed the ry:default_runtime branch from 7462e4c to 4889070 Sep 25, 2018

ry added some commits Sep 18, 2018

Make Deno multithreaded.
By using the tokio default runtime.

This patch makes all of the ops thread safe.

Adds libdeno to JS globals to make for easier testing.

Preliminary work for #733.
Use lazy_static for HttpsConnector
And rename net.rs to http.rs

Share HTTP connection.
Add SetGlobalTimeout().
To be used for a timers implementation soon.

@ry ry force-pushed the ry:default_runtime branch from 4889070 to 15940b5 Sep 25, 2018

@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Sep 25, 2018

@piscisaureus PTAL

I had to disable a test in order to get this patch to pass CI. I'm not sure what exactly is going on, but it seems to be related to hyper's independent thread pool for DNS resolution. I will be replacing this thread pool and use non-blocking DNS soon - but it’s going to take a bit of development. I want to land this patch despite this regression because it will help us move forward with other development.

Also - the benchmark numbers are going to be painful to look at - there is a fairly large jump in the warm startup benchmark as a result of this patch:
screen shot 2018-09-25 at 11 23 27 am
And I would expect the syscall and thread benchmarks to jump too. The tokio default runtime is relatively wasteful compare to the "current thread" runtime - but I think this is worth it - I also think we’ll be able to optimize many things quickly and get back down.

@ry ry merged commit 591174a into denoland:master Sep 25, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@ry ry deleted the ry:default_runtime branch Sep 25, 2018

ry added a commit to ry/deno that referenced this pull request Sep 29, 2018

v0.1.6
- Adds deno.stdin, deno.stdout, deno.stderr, deno.open(), deno.write(),
  deno.read(), deno.Reader, deno.Writer, deno.copy() denoland#846
- Print 'Compiling' when compiling TS.
- Support zero-copy for writeFile() writeFileSync() denoland#838
- Fixes eval error bug denoland#837
- Make Deno multithreaded denoland#782
- console.warn() goes to stderr denoland#810
- Add deno.readlink()/readlinkSync() denoland#797
- Add --recompile flag denoland#801
- Use constructor.name to print out function type denoland#664
- Rename deno.argv to deno.args
- Add deno.trace() denoland#795
- Continuous benchmarks https://denoland.github.io/deno/

@ry ry referenced this pull request Sep 29, 2018

Merged

v0.1.6 #858

ry added a commit that referenced this pull request Sep 29, 2018

v0.1.6
- Adds deno.stdin, deno.stdout, deno.stderr, deno.open(), deno.write(),
  deno.read(), deno.Reader, deno.Writer, deno.copy() #846
- Print 'Compiling' when compiling TS.
- Support zero-copy for writeFile() writeFileSync() #838
- Fixes eval error bug #837
- Make Deno multithreaded #782
- console.warn() goes to stderr #810
- Add deno.readlink()/readlinkSync() #797
- Add --recompile flag #801
- Use constructor.name to print out function type #664
- Rename deno.argv to deno.args
- Add deno.trace() #795
- Continuous benchmarks https://denoland.github.io/deno/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment