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

Avoid extra memory allocation with templates #602

Closed
dmitryash opened this issue Nov 16, 2019 · 4 comments
Closed

Avoid extra memory allocation with templates #602

dmitryash opened this issue Nov 16, 2019 · 4 comments

Comments

@dmitryash
Copy link
Contributor

I suppose that it is possible to add methods in the following way to avoid extra memory allocation:

Example:

template <class T> 
class ObjectWrap {
    using MethodType = Value (T::*)(const CallbackInfo &info);

    template <MethodType method>
    static auto ObjectWrap<T>::InstanceMethod2(const char* utf8name) -> ClassPropertyDescriptor<T>; 
};

template <typename T>
template <typename ObjectWrap<T>::MethodType method>
inline auto ObjectWrap<T>::InstanceMethod2(const char* utf8name) -> ClassPropertyDescriptor<T>  {
    napi_property_descriptor desc = {};
    desc.utf8name = utf8name;
    desc.method = [](napi_evn env, napi_callback_info args) {
        return details:;WrapCallback([&]{
            CallbackInfo cb_info(env, args);
            T* instance = Unwrap(cb_info.This().As<Object>());
            return (instance->*cb)(callbackInfo);
        });
    };
    return desc;
}

// How to use
class Test: public Napi::ObjectWrap<Test> {
public:
    static auto define_class(Napi::Env env) -> Napi::Function {
        return DefineClass(env, "Test", {
            InstanceMethod2<&Test::get_value>("get_value");
        });
    }

    auto get_value(const Napi::CallbackInfo &args) -> Napi::Value;
};
@gabrielschulhof
Copy link
Contributor

This does indeed work, and avoiding a memory allocation is definitely worth it. I'm not sure how to incorporate this, though. We should probably phase out the existing instance methods and accessors.

@dmitryash
Copy link
Contributor Author

Let me take it

@gabrielschulhof
Copy link
Contributor

@dmitryash we discussed this during today's team meeting. The best approach is to add an alternate set of methods to ObjectWrap<T> and encourage their use over the existing methods. We can deprecate the existing methods and eventually remove them. Not sure what the name should be though – InstanceMethod2() does not seem too expressive. OTOH TemplatedInstanceMethod() is way too long of a name. I dunno ... InstanceMethodT() 🤷 😕

@gabrielschulhof
Copy link
Contributor

@dmitryash so if you would like to do a PR, then please, go right ahead 🙂

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: nodejs/node-addon-api#602
PR-URL: nodejs/node-addon-api#604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: nodejs/node-addon-api#602
PR-URL: nodejs/node-addon-api#604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: nodejs/node-addon-api#602
PR-URL: nodejs/node-addon-api#604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
ObjectWrap<T> was enhanced to support template methods for
defining properties and methods of JS class. Now C++ methods and
functions may be passed as template parameters for
ObjectWrap<T>::InstanceMethod, ObjectWrap<T>::StaticAccessor, etc.

There are several benefits:

  - no need to allocate extra memory for passing C++ function
    napi callback and use add_finalizer() to free memory;
  - a compiler can see whole chain of calls up to napi callback
    that may allow better optimisation.

Some examples:

```cpp
// Method
InstanceMethod<&MyClass::method>("method");

// Read-write property
InstanceAccessor<&MyClass::get, &MyClass::set>("rw_prop");

// Read-only property
InstanceAccessor<&MyClass::get>("ro_prop");
```

Fixes: nodejs/node-addon-api#602
PR-URL: nodejs/node-addon-api#604
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
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

Successfully merging a pull request may close this issue.

2 participants