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

[FUTURE] Write a comparison benchmark using worker_threads module #143

Open
springmeyer opened this issue Jun 21, 2018 · 5 comments
Open

Comments

@springmeyer
Copy link
Contributor

springmeyer commented Jun 21, 2018

v10.5.0 has a new worker_threads module that allows pure JS functions to run on a separate thread. The node-cpp-skel demonstrates doing this type of thing using C++, but the worker_threads module allows for it in pure JS. (https://nodejs.org/en/blog/release/v10.5.0/)

It would be useful to try to write an 🍏 to 🍏 (or as close as we can get) comparison benchmark of doing the same work in C++ threads vs JS threads using worker_threads. We would try to answer questions like:

We could get at 🍒 by writing a benchmark that passes the same, large data, and does no compute in the thread. Instead that thread could just sleep for N seconds.

We could get at 🍊 by passing the same data and doing the same (as close as possible) compute, perhaps a Fibonacci (or some kind of classical compute intensive workload).


Depending on the results we will want to update https://github.com/mapbox/cpp/blob/master/node-cpp.md#why-create-a-node-addon which says:

Why create a node addon?
To port a C++ project to Node to expose a new interface for the tool (like Mapnik & Node Mapnik)
Improve performance at scale where Node becomes the bottleneck (i.e. concurrency)

If JS is just as fast as C++, then we'll want to update that doc to mention worker_threads as a more ideal solution than writing a node addon to access the thread pool. If JS/worker_threads is slower than C++ then we'll probably want to add details to that doc about what types of workloads or performance situations we'd want to use C++ and which we'd want to consider JS/worker_threads

/cc @mapbox/core-tech

@GretaCB
Copy link
Contributor

GretaCB commented Jul 5, 2018

@springmeyer thanks for writing this up! I was just skimming the Worker PR and FAQ, super interesting to learn how they have started to implement this.

Q: Does the node inspector work? Can I use the Chrome devtools to debug workers?
A: Not initially, but it's a todo and a high priorty. Help appreciated

coooooolll 🎉

@springmeyer
Copy link
Contributor Author

springmeyer commented Jul 5, 2018

super interesting to learn how they have started to implement this

👍 @GretaCB - yes its a real game changer - and full of interesting and hard questions. My take is that it is absolutely critical to vet this feature. One outcome could be that it is dangerous, slow, and something we want to ban from usage. Another outcome could be that it works so well we may want to push node-cpp-skel development in this direction. What I think should determine our approach is whether it is stable (not going to cause crashes or security problems) and is as high performance as using Nan::AsyncWorker/libuv to work in C++ threads.

Another interesting twist I had not considered when I opened this ticket is how you'd run a node C++ addon that uses the C++ threadpool inside a worker_thread. So a thread that runs another module that can multithread. This feels like a potentially horrible idea performance wise (if the threads contended - it would look to the eye like fast code but actually could be horrible slow). Or it could work great. We'd need to test. TryGhost/node-sqlite3#1007 was a first attempt that needs revisited (since it caused problems).

@icelord2
Copy link

@springmeyer I am messing with worker_thread and was trying to get sqlite3 to work before seeing the comments from a couple of the tickets. does sqlite3 currently work with worker threads? if not are there still plans to get it working?

@springmeyer
Copy link
Contributor Author

@icelord2 addaleax/node@1d8b74d indicates that node-sqlite3 would need to use NODE_MODULE_WORKER_ENABLED. I don't see any sign of that in the codebase of node-sqlite3, so I think this is not yet supported. If you need it I would recommend created a ticket at node-sqlite3 repo. Or better yet, a PR.

@springmeyer
Copy link
Contributor Author

/cc @mapbox/static-apis @mapbox/tilesets-api - this is the ticket I referred to today in our call.

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

No branches or pull requests

3 participants