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

Enhancement: pass data created in threadpool back to main JS thread in zero-copy way #69

Closed springmeyer closed 6 years ago

springmeyer commented 7 years ago

The #67 talks about a critical optimization when passing data into the threadpool (to do work on it). The corresponding optimization that is equally important is that large data created in the threadpool needs to be efficiently passed back across the boundary from C++ to JS.

Otherwise if the data is large then this hurts performance in the After callback (called HandleOkCallback when using Nan::AsyncWorker), which runs on the main event loop. In particular, large copies at this point can trigger expensive allocation and further slow down the main event loop.

refs this code: https://github.com/mapbox/node-cpp-skel/blob/44f8d413f58fec46839ed7febb75d70ec1fe584e/src/standalone_async/hello_async.cpp#L104-L113.

Proposal

Modify node-cpp-skel to return a node::Buffer from the threadpool using node::Buffer::New.

We'll need to modify the example code to make this logical: we'll need to invent a scenario where a node::Buffer would be appropriate. A good example of this is when passing gzip compressed data back from the threadpool.

springmeyer commented 7 years ago

large data created in the threadpool needs to be efficiently passed back across the boundary from C++ to JS

Modify node-cpp-skel to return a node::Buffer from the threadpool using node::Buffer::New.

The way to achieve both of these things is to allocation data once and move it into the node.Buffer. Because the node.Buffer object does not support C++ move sematics we can accomplish this with the custom node::Buffer::New signature that accepts an existing char * to data and also a free_callback to use once ownership of that data is passed to node::Buffer::New. This can be accomplished like:

baton->string_ptr.release();
v8::Local<v8::Value> argv[2] = { Nan::Null(),
                                 Nan::NewBuffer((char*)baton->string_ptr->data(),
                                    baton->string_ptr->size(),
                                    [](char *, void * hint) {
                                        delete reinterpret_cast<std::string*>(hint);
                                    },
                                    baton->string_ptr.get()
                                 ).ToLocalChecked()
                               };
Nan::MakeCallback(Nan::GetCurrentContext()->Global(), Nan::New(baton->cb), 2, argv);

Described at https://github.com/mapnik/node-mapnik/issues/531#issuecomment-328545822

mapsam commented 6 years ago

Note that we ran into malloc errors related to passing buffers back to JS land in https://github.com/mapbox/vtcomposite/pull/19 - it'd be great to get this set up in node-cpp-skel to avoid rewriting the strings into std::unique_ptr!

springmeyer commented 6 years ago

Done in #147