src: introduce PullAll method to speed up blob.text/arrayBuffer

[debadree25]

Made a small attempt at addressing nodejs/performance#118 essentially array buffer repeatedly calls in to the Pull method in c++ gathers all the buffers and then calls again to concatenate the buffers hence moved this entire process to the C++ side. There seems to be good performance improvment of ~3x but could not run the full comparison benchmark because it takes tooo long. My C++ skills are also pretty weak so would need some guidance 😅! Opening this PR to gather enough feedback to improve 😃

Refs: nodejs/performance#118

Thank You!

[anonrig]

If you pass resolve and reject methods to pullAll function, we don't even need to return anything and just handle it on C++ side. It might provide a performance boost. This would also reduce calling and mutating the callback function.

[debadree25]

Good idea!

[anonrig]

Can we use a little more descriptive struct names?

[debadree25]

Okay so now the main problem how to replicate something like

reader.pull((status, buffer) => {
// ....
queueMicrotask(() => readNext());
}

in the C++ land
i tried something like this

std::shared_ptr<Impl> impl = std::make_shared<Impl>();
impl->reader = BaseObjectPtr<Blob::Reader>(reader);
impl->callback.Reset(env->isolate(), fn);
impl->env = env;
auto next = [impl_ptr = impl](int status, const DataQueue::Vec* vecs, size_t count, bob::Done doneCb) mutable {
  // ... initialising stuff and collecting the buffer
  if (status > 0) {
      impl_ptr->enqueue_cb();
  } else {
      // .. concatenate the buffers
      Local<Value> argv[2] = {Int32::New(env->isolate(), status), ArrayBuffer::New(env->isolate(), store)};
      impl_ptr->reader->MakeCallback(fn, arraysize(argv), argv);
  }
};

impl->enqueue_cb = [&]() {
    reader->inner_->Pull(next, node::bob::OPTIONS_END, nullptr, 0);
};

impl->enqueue_cb();

but the above gives a segmentation fault ☹️
i tried using unique_ptr too but that gives

note: copy constructor of '(lambda at ../../src/node_blob.cc:420:15)' is implicitly deleted because field '' has a deleted copy constructor

so any other ideas? I know wouldnt be doing a recursive call instead would call the Pull wrapped in enqueue microtask ref: nodejs/performance#118 (comment) but need to get the calling order correct first

[anonrig]

Can you also update typescript typings for blob?

[debadree25]

Updating

[debadree25]

Okay now this causes a stack overflow for large bufs we need to use microtask queue i tried something like this

env->RequestInterrupt([impl_ptr](Environment* env) {
        env->context()->GetMicrotaskQueue()->EnqueueMicrotask(
            env->isolate(),
            [](void* arg) {
              printf("Calling here?\n");
              auto impl_ptr = static_cast<std::shared_ptr<Impl>*>(arg);
              (*impl_ptr)->enqueue_cb();
            },
            impl_ptr.get());
      });

but this gives a very friendly segfault probably because i mangling the pointer somewhere ☹️ there arent much examples of usage with the microtask queue

[debadree25]

any ideas @anonrig ?😅

[debadree25]

@nodejs/cpp-reviewers if anyone could guide on this PR would be great help!

Thank you!

[lemire]

I think that NewBackingStore returns std::unique_ptr<BackingStore>, doesn't it? If I am right, and you are changing it to a shared_ptr, then I recommend you justify this choice with a comment.

[lemire]

same question here. If NewBackingStore returns a unique_ptr, changing the type might require an explanation.

[lemire]

I personnally do not understand why there is a std::move... If it is correct, I recommend adding a comment to explain it.

[lemire]

@debadree25 Compiling node with --enable-asan may help with segmentation faults and the like.

[debadree25]

Good idea @lemire I havent had tried with asan build let me get back to you by trying with that, i may that have rewrite this again once to remember the flow i was attempting!, thank you for taking the time to review!

[debadree25]

Will reopen have completely messed up my branch