-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 Move event loop into JavaScript. #1003
Conversation
Before:
After:
So that's better - but when looking at the http benchmark it's worse:
Here's what the strace looks like on the http server:
|
Probably we need a benchmark dedicated to futex. Looks pretty bad |
Here is same issue. https://groups.google.com/forum/#!topic/comp.unix.programmer/5RAMxJJnRcE |
@kevinkassimo futex is most likely from the std::sync::mpsc we use in the main thread. It looks bad, and I'm not sure it's the best design, but I doubt this is the bottleneck. (I'd rather do epoll the main thread - but it will require making a custom Tokio runtime.) |
FYI Here's how I am profiling:
Here is are the outputs for before and after this patch: https://gist.github.com/ry/4b9d6f62b60ac8d7324ea624b7bd26e6 |
Counterintuitively this patch makes the program spend more time in C++. Look at the summary:
This might be because V8 is able to optimize better when the event loop is in JS. Examining where time is spent in C++
32% of the time is spent in __pthread_cond_wait after the patch. Surely this is the std::sync::mpsc in the main thread waiting for a response. A ridiculous place to spend time when we're performing non-blocking reads/writes. The read/writes should be performed on the main thread - there does not need to be cross-thread communication in this example. We need to move back to In both profiles |
Here is what an strace on tests/http_bench.ts looks like currently (on master) |
Here's a little script I'm using, in case this helps others look into this stuff. > cat profile_http.sh
#!/bin/bash
deno_exe=$1
if [[ ! -x "$deno_exe" ]]; then
echo $deno_exe is not executable.
exit 1
fi
$deno_exe tests/http_bench.ts --allow-net --prof &
sleep 1
third_party/wrk/linux/wrk http://localhost:4500/
kill `pgrep deno` |
libdeno.runMicrotasks(); | ||
} | ||
} | ||
return promiseErrorExaminer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, does this mean that we will only handle promise uncaught rejects when deno is about to exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just what passes the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think with this impl deno would not promptly die on uncaught errors after running to the end.
Which one was the original failing test?
(quite busy in the coming weeks but might as well take a look)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests/async_error.ts
} | ||
|
||
export function eventLoop(): boolean { | ||
for (;;) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always liked while (true)
better for infinite loops, feels more intentional to me, but...
Closing this one. It resulted in profiling and ongoing perf fixes. The ultimate goal is to move the loop to JS - but we're not ready to do it yet. |
No description provided.