Look for zero-copy optimizations using Nan::NewBuffer #531

Activity

1. springmeyer commented on Sep 9, 2017

   

   springmeyer

   on Sep 9, 2017

   MemberAuthor

   More actions

   refs mapbox/node-cpp-skel#69
2. talaj commented on Sep 11, 2017

   

   talaj

   on Sep 11, 2017

   Last edited by talaj

   Member

   More actions

   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.
3. springmeyer commented on Sep 11, 2017

   

   springmeyer

   on Sep 11, 2017

   MemberAuthor

   More actions

   @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
4. talaj commented on Sep 11, 2017

   

   talaj

   on Sep 11, 2017

   Member

   More actions

   I think this is great solution. It can be a simple wrapper function around Nan::NewBuffer().
5. springmeyer commented on Sep 11, 2017

   

   springmeyer

   on Sep 11, 2017

   MemberAuthor

   More actions

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

   Good idea.
6. springmeyer commented on Sep 11, 2017

   

   springmeyer

   on Sep 11, 2017

   MemberAuthor

   More actions

   @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.