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

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

Closed
springmeyer opened this issue Aug 25, 2017 · 3 comments

Comments

@springmeyer
Copy link
Contributor

springmeyer commented Aug 25, 2017

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:

void HandleOKCallback() override
{
Nan::HandleScope scope;
const auto argc = 2u;
v8::Local<v8::Value> argv[argc] = {
Nan::Null(), Nan::New<v8::String>(result_).ToLocalChecked()};
callback->Call(argc, argv);
}
.

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
Copy link
Contributor Author

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:

  • Using a std::make_unique<std::string>(); to allocate data and work with it (append data, for example) in the threadpool
  • The unique_ptr will own the data while working on it
  • Then exchange ownership with the node::Buffer after returning from the threadpool like so:
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 mapnik/node-mapnik#531 (comment)

@mapsam
Copy link
Contributor

mapsam commented May 16, 2018

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

@springmeyer
Copy link
Contributor Author

Done in #147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants