Look for zero-copy optimizations using Nan::NewBuffer

[springmeyer]

Using Nan::NewBuffer allows the existing memory to be referenced rather than copied when creating a node::Buffer. Currently we are not using Nan::NewBuffer but there a likely a number of spots where we could safely benefit from this optimization.

[springmeyer]

refs mapbox/node-cpp-skel#69

[talaj]

A problem in many cases is that node::Buffer is created from std::string which cannot handover its internal buffer ownership to non-std::string object.

[springmeyer]

@talaj - one way I've found to handle this situation is to use std::make_unique<std::string>(); and then write data to the std::string inside the unique_ptr. When it comes to sending the data back to the threadpool I've done this like (where string_ptr is the unique_ptr on a baton passed to the threadpool):

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);

What do you think? Is that viable for node-mapnik / usecases you have in mind?

One downside of this approach is that it looks like the memory is leaked to leak checkers like https://clang.llvm.org/docs/LeakSanitizer.html

[talaj]

I think this is great solution. It can be a simple wrapper function around Nan::NewBuffer().

[springmeyer]

I think this is great solution. It can be a simple wrapper function around Nan::NewBuffer().

Good idea.

[springmeyer]

@talaj if you end of applying this in your work in node-mapnik, we should be fine without LeakSantizer issues since the LeakSantizer is currently disabled (refs #747).

But ideally we can enable it in the future. Anyway, wanted to follow through on what I see when applying this approach and testing with LeakSantizer:

=================================================================
==9980==ERROR: LeakSanitizer: detected memory leaks
Indirect leak of 432 byte(s) in 2 object(s) allocated from:
    #0 0x7f9e86cca00b in operator new(unsigned long) /home/travis/build/mapbox/mason/mason_packages/.build/llvm-4.0.1/projects/compiler-rt/lib/asan/asan_new_delete.cc:82:35
    #1 0x7f9e8657a628 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0xc4628)
    #2 0x7f9e8657b40a in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0xc540a)
    #3 0x7f9e8657b4b3 in std::string::reserve(unsigned long) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0xc54b3)
    #4 0x7f9e8657b732 in std::string::append(char const*, unsigned long) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0xc5732)
    #5 0x7f9e7cf59054 in protozero::pbf_writer::add_bytes(unsigned int, char const*, unsigned long) /home/travis/build/mapbox/vt-shaver-cpp/mason_packages/.link/include/protozero/pbf_writer.hpp:481:17
    #6 0x7f9e7cf43ae9 in AsyncShave(uv_work_s*) /home/travis/build/mapbox/vt-shaver-cpp/build/../src/shave.cpp:541:40
    #7 0xfa5f40 in worker /home/iojs/build/ws/out/../deps/uv/src/threadpool.c:95
    #8 0xfb5078 in uv__thread_start /home/iojs/build/ws/out/../deps/uv/src/unix/thread.c:52
    #9 0x7f9e85d8de99 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x7e99)
    #10 0x7f9e85abb2ec in clone (/lib/x86_64-linux-gnu/libc.so.6+0xf62ec)

I figure LeakSanitizer is correct that the memory is not being cleaned up right away, but that is by design: by transferring ownership of the string to the node::Buffer the delete of the std::string gets deferred to when v8 decides to garbage collect the node::Buffer. But I've tried forcing gc with node --expose-gc and calling gc() in the code after testing and the LeakSanitizer still complains. So I'm not quite sure how best to fix this indirect leak/suppress it yet. Curious if you have ideas.