Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

N-API support? #836

Open
reconbot opened this issue Dec 18, 2018 · 12 comments
Open

N-API support? #836

reconbot opened this issue Dec 18, 2018 · 12 comments

Comments

@reconbot
Copy link

Would it be feasible to target N-API for it's subset of features on node versions 8+?

I've been using NAN for years but with node 6 hitting it's EOL next year it's becoming more interesting to explore N-Api. It covers most of the use cases that I have and would help keep my projects working with out having to upgrade for new node versions.

However if NAN supported it I wouldn't have to do a major rewrite.

Are there thoughts around this?

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 18, 2018

Yes, it would surely be possible to support the napi subset of features. See #831 (comment). Unfortunately, I do not have time to do it myself, but overall it should be a fairly simple, if laborious task.

@gabrielschulhof
Copy link

FWIW I've started working on an approach where I write a V8 shim which uses the node-addon-api wrapper and inline functions to transform code that would normally link to V8 into code that makes N-API calls. I'm working my way through the node-addon-examples to complete the shim.

https://github.com/gabrielschulhof/node-addon-api/tree/napi-nan/shim
https://github.com/gabrielschulhof/node-addon-examples/tree/napi-nan (check subdirectories */v8/ for pure V8 examples I'm making work with the shim)

@gabrielschulhof
Copy link

Well, I'm running into a serious snag with my shim. In order to keep from having to heap-allocate my definition of class Local differs from V8 in that the value it stores is not stored by pointer but inline. Now, this breaks down when attempting to define Local<FunctionTemplate> and Local<Signature> because those need each other in their respective factory functions. So, I have a circular class dependency that can only be solved by storing a pointer in Local. In case of V8 it's easy to store a pointer in Local, because it deals with real heap-allocated objects, but the shim should not heap-allocate if it is to boil down to N-API. I'm not sure I have the C++-fu to define classes Local, FunctionTemplate, and Signature so

  1. they boil down to N-API calls, and so
  2. no heap allocation needs to be done, and so
  3. they satisfy their V8 API

@gabrielschulhof
Copy link

The other way of looking at it is that Signature and FunctionTemplate need to be pointer types, but then you cannot easily define Signature::New and FunctionTemplate::New.

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 31, 2018

Can you not solve it by first declaring the offending classes and then then defining their methods?

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 31, 2018

class FunctionTemplate {
 public:
  FunctionTemplate(Isolate* isolate,
                          FunctionCallback v8_method,
                          Local<Value> data,
                          Local<Signature> signature);
  inline void SetClassName(Local<String> class_name_) {
    class_name = class_name_;
  }
  inline MaybeLocal<Function> GetFunction(Local<Context> context) {
    napi_status status;
    napi_value result;
    V8FunctionCallbackData* callbackData =
        new V8FunctionCallbackData(v8_method);
    Isolate* isolate = context->GetIsolate();
    napi_env local_env = (isolate ? isolate->Env() : env);

    status = napi_define_class(local_env,
                               nullptr,
                               0,
                               NapiFunctionCallback,
                               callbackData,
                               0,
                               nullptr,
                               &result);
    NAPI_THROW_IF_FAILED(local_env, status, Local<Function>(Napi::Function()));

    status = Napi::details::AttachData(env, result, callbackData);
    NAPI_THROW_IF_FAILED(local_env, status, Local<Function>(Napi::Function()));

    return MaybeLocal<Function>(Local<Function>(Napi::Function(local_env, result)));
  }
  inline Local<ObjectTemplate> InstanceTemplate() {
    return Local<ObjectTemplate>(this);
  }
  inline Local<Function> GetFunction() {
    return GetFunction(Local<Context>(nullptr)).ToLocalChecked();
  }
  static Local<FunctionTemplate> New(Isolate* isolate,
                                            FunctionCallback v8_method,
                                            Local<Value> data,
                                            Local<Signature> signature);
 private:
  napi_env env;
  FunctionCallback v8_method;
  Local<String> class_name;
  Local<Value> data;
  Local<Signature> signature;
};

class Signature {
 public:
  Signature() {}
  static Local<Signature> New(Isolate* isolate,
      Local<FunctionTemplate> receiver);
};

inline FunctionTemplate::FunctionTemplate(Isolate* isolate,
                        FunctionCallback v8_method,
                        Local<Value> data,
                        Local<Signature> signature) :
    env(isolate->Env()),
    v8_method(v8_method),
    data(data),
    signature(signature) { }

inline Local<FunctionTemplate> FunctionTemplate::New(Isolate* isolate,
                                          FunctionCallback v8_method,
                                          Local<Value> data = Local<Value>(),
                                          Local<Signature> signature = Local<Signature>()) {
  return Local<FunctionTemplate>(isolate, v8_method, data, signature);
}

inline Local<Signature> Signature::New(Isolate* isolate,
    Local<FunctionTemplate> receiver) {
  return Local<Signature>();
}

@gabrielschulhof
Copy link

@kkoopa yes, thank you!

@gabrielschulhof
Copy link

Another potentially fundamental problem:

Since the whole v8 namespace is essentially a bunch of inline definitions it works well when building in Release mode, however, when building in Debug mode, I guess it doesn't inline anything, so this results in a bunch of weak symbols which end up resolved against the actual V8 symbols at runtime.

This means that in Debug mode the addon tends to crash horribly even though it works in Release mode.

The only solution I've found is to enclose the v8 namespace in an anonymous namespace so the local symbols take precedence, but then there's a problem in places like https://github.com/nodejs/node-addon-examples/blob/master/6_object_wrap/node_0.12/addon.cc#L7 because there a Local<Object> gets passed from one compilation unit to the other, but each compilation unit has its own anonymous namespace so a Local<Object> in one compilation unit is a different class than a Local<Object> in a different compilation unit, and the symbols no longer match.

@gabrielschulhof
Copy link

A potential solution on gcc is -Wl,-Bsymbolic, but I'm not sure I'll find an equivalent for all supported platforms.

@Flarna
Copy link
Member

Flarna commented Jan 2, 2019

Some questions comes in my mind in this: V8 API changes frequently so which version of V8 API have you planned to shim here?
What is the benefit of a V8 shim compared to the more stable NAN API targeting NAPI instead of V8 for newer Node versions?

@gabrielschulhof
Copy link

@Flarna NAN only targets the parts of V8 that break. You can still use a plain v8::Local<v8::Object> with Nan, so that also has to be shimmed. Ideally, of course, Nan would use N-API directly, but there will always be parts of V8 a N-API implementation couldn't cover because N-API itself doesn't cover it. The basic exercise I think is to attempt to get to the point where the existing code compiles down to N-API calls instead of V8 calls.

@gabrielschulhof
Copy link

Also, Nan can call the V8 shim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants