[WIP] separate vm and backend threads #462
Conversation
There was a problem hiding this comment.
Excellent work! We will have to do several passes of reviews on this - initial comments below.
(For anyone watching who is confused about this - this is something Bert and I have been discussing a long time. Because all calls in Deno are serialized, there's the possibility of running JS and Rust in separate threads... potentially allowing Rust to do things like accept and decode incoming TLS connections while JS is processing application logic. It is not clear if this model will be slower or faster than the existing single threaded model, so this commit attempt to support both modes with the threads_enabled flag. The idea being that we can support both and then benchmark once we get things like the HTTP server working.)
| void deno_buf_delete_raw(deno_buf* buf) { memset(buf, 0, sizeof *buf); } | ||
|
|
||
| #ifdef __cplusplus | ||
| deno_buf::~deno_buf() { |
There was a problem hiding this comment.
Maybe add a comment that this is only for error checking.
|
|
||
| // This callback receives a deno_buf containing a message. It should return a | ||
| // message's cmd_id. | ||
| typedef uint32_t (*deno_cmd_id_cb)(const deno_buf* buf); |
There was a problem hiding this comment.
This doesn't seem like the most elegant design. If cmd_id is going to be required for the usage of libdeno, then it should probably be a parameter in deno_send and deno_recv_cb.
| @@ -0,0 +1,121 @@ | |||
|
|
|||
There was a problem hiding this comment.
copyright header (does this pass lint?)
src/mq.h
Outdated
|
|
||
| std::unique_lock<Mutex> lock(mutex_); | ||
|
|
||
| if (head_ == nullptr) { |
There was a problem hiding this comment.
Maybe comment that this is a standard link list append. Or maybe use std::list<deno_buf>?
| @@ -346,7 +378,7 @@ char** deno_argv() { return global_argv; } | |||
| int deno_argc() { return global_argc; } | |||
|
|
|||
| void deno_set_flags(int* argc, char** argv) { | |||
| v8::V8::SetFlagsFromCommandLine(argc, argv, true); | |||
| // v8::V8::SetFlagsFromCommandLine(argc, argv, true); | |||
There was a problem hiding this comment.
If you're doing this to get access to gtest command line flags, it's better we just remove the deno_set_flags in src/tests.cc.
| } | ||
|
|
||
| // Start backend worker thread. | ||
| // TODO: use multiple backend threads. |
There was a problem hiding this comment.
I'm not yet in agreement with the TODO. Let's keep it at two threads for now.
| // TODO: use multiple backend threads. | ||
| std::thread backend_thread(backend_main, d); | ||
| auto r = execute_js(d, js_filename, js_source); | ||
| // TODO: join the backend thread. |
There was a problem hiding this comment.
The backend thread is started and joined on every deno_execute? I think it would be better if the backend thread was created and joined in main().
| d->data = data; | ||
|
|
||
| auto env_value = getenv("DENO_THREADS"); | ||
| d->threads_enabled = env_value != nullptr && env_value == std::string("1"); |
There was a problem hiding this comment.
Since d is opaque to rust, threads_enabled should be an argument to deno_new... or maybe something like deno_enable_theads(bool)...
| try: | ||
| run([test_cc]) | ||
| except: | ||
| pass |
src/binding.cc
Outdated
| auto res_buf = DENO_BUF_INIT; | ||
| auto r = d->res_queue.RecvFilter(&res_buf, [&](const deno_buf& buf) { | ||
| return cmd_id == d->cmd_id_cb(&buf); | ||
| }); |
|
This needs to be measured after Tokio lands and we can get i/o benchmarks - I don't know if Tokio uses threads (or how many) under the hood so this might seem to be a win until then. Generally interesting work! |
|
FYI #434 starts using async messages (deno_send) from the event loop. |
No description provided.