mapnik
Look for zero-copy optimizations using `Nan::NewBuffer`
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.
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.
@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
I think this is great solution. It can be a simple wrapper function around Nan::NewBuffer().
I think this is great solution. It can be a simple wrapper function around Nan::NewBuffer().
Good idea.
@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 https://github.com/mapnik/node-mapnik/issues/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.