mapbox / node-cpp-skel

Skeleton for bindings to C++ libraries for Node.js using node-addon-api
Creative Commons Zero v1.0 Universal
72 stars 10 forks source link

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

Open springmeyer opened 6 years ago

springmeyer commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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. https://github.com/mapbox/node-sqlite3/pull/1007 was a first attempt that needs revisited (since it caused problems).

icelord2 commented 5 years ago

@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 commented 5 years ago

@icelord2 https://github.com/addaleax/node/commit/1d8b74d790621cae2aa0045397f0eabe4a3f4d83 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 commented 2 years ago

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