[jasongin]

• Add a napi_get_cb_info function to get args length, args, this, and data in a single call
• Move the static exception to a class member to enable inlining of the lastException() method.
• Refactor the CallbackWrapper group of helper classes to avoid most (non-inlinable) virtual function calls
• Remove use of v8::TryCatch (via NAPI_PREAMBLE) from several places where it shouldn't be needed.

Together, these optimizations reduce the overhead of every JS-to-C++ call through the NAPI layer by approximately 50%.

[jasongin]

CI: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-commit-linux-abi/12/

[kkoopa]

Would it not be possible to do the same to napi_delete_reference?

napi_status napi_delete_reference(napi_env e, napi_ref ref) {
  NAPI_PREAMBLE(e);
  CHECK_ARG(ref);

  v8impl::Reference* reference = reinterpret_cast<v8impl::Reference*>(ref);
  delete reference;

  return GET_RETURN_STATUS();
}

napi_status napi_delete_reference(napi_env, napi_ref ref) {
  delete static_cast<v8impl::Reference*>(ref);
  return napi_ok;
}

[jasongin]

@kkoopa Yes good point. That one wasn't related to any areas I was focusing on for performance, but the TryCatch obviously isn't necessary there either. I'll make that change.

[jasongin]

Actually I'm not sure about applying that change to napi_delete_reference(), because the v8impl::Reference destructor calls v8::Persistent::Reset(), which might do non-trivial things in the VM.

[kkoopa]

Define non-trivial. I would not assume that v8::Persistent::Reset() can throw a js exception. It does not even take an Isolate *. All v8::Persistent::Reset() should do is clear the val_ member and the storage cell.

template <class T>
void PersistentBase<T>::Reset() {
  if (this->IsEmpty()) return;
  V8::DisposeGlobal(reinterpret_cast<internal::Object**>(this->val_));
  val_ = 0;
}

void V8::DisposeGlobal(i::Object** location) {
  i::GlobalHandles::Destroy(location);
}

void GlobalHandles::Destroy(Object** location) {
  if (location != NULL) Node::FromLocation(location)->Release();
}

  void Release() {
    DCHECK(IsInUse());
    set_state(FREE);
    // Zap the values for eager trapping.
    object_ = reinterpret_cast<Object*>(kGlobalHandleZapValue);
    class_id_ = v8::HeapProfiler::kPersistentHandleNoClassId;
    set_independent(false);
    set_active(false);
    weak_callback_ = NULL;
    DecreaseBlockUses();
  }
/*-------------------------------------------------------------------------------*/
void GlobalHandles::Node::DecreaseBlockUses() {
  NodeBlock* node_block = FindBlock();
  GlobalHandles* global_handles = node_block->global_handles();
  parameter_or_next_free_.next_free = global_handles->first_free_;
  global_handles->first_free_ = this;
  node_block->DecreaseUses();
  global_handles->isolate()->counters()->global_handles()->Decrement();
  global_handles->number_of_global_handles_--;
}

  void DecreaseUses() {
    DCHECK(used_nodes_ > 0);
    if (--used_nodes_ == 0) {
      if (next_used_ != NULL) next_used_->prev_used_ = prev_used_;
      if (prev_used_ != NULL) prev_used_->next_used_ = next_used_;
      if (this == global_handles_->first_used_block_) {
        global_handles_->first_used_block_ = next_used_;
      }
    }
  }

As far as I see by skimming through the code, it is fairly trivial.

[jasongin]

OK you convinced me, it should be fine then.