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

node-api: enable napi_ref for all value types #42557

Closed
wants to merge 5 commits into from

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Apr 1, 2022

The issue

In the Node-API the napi_value exists only on the call stack. When the value needs to be persisted after the call stack is unwinded, we can use the napi_ref. Currently the napi_ref keeps napi_value only for napi_object, napi_function, napi_external, and napi_symbol types because napi_ref must support weak pointer semantic. Thus, there was a decision to not keep persistent values for other types such as napi_string or napi_number. Though, the napi_symbol cannot have the weak semantic, but we still can have a napi_ref for it.

Solution

In this PR we enable use of napi_ref for strong references to all napi_value types. Though, only references for napi_object, napi_external, and napi_function types could have the true weak reference semantic. These three types could be collected by GC when the ref count is 0, while other types cannot be collected because they are always string references. To keep the backward compatibility, this PR enables the new behavior under a special node_api_features bit flag node_api_feature_reference_all_types. The node_api_features are being introduced in this PR.

The new node_api_features allow changing internal behavior of existing Node-API functions.

We pass a node_api_features pointer to the napi_module struct in the NAPI_MODULE_X macro. This macro is used for the module registration. If the module is initialized without using this macro, then there will be no features selected and the module will use the node_api_feature_none.

Each Node-API version is going to define its own default set of features. For the current version it can be accessed using node_api_default_features. A module can override the set of its enabled features by adding NODE_API_CUSTOM_FEATURES definition to the .gyp file and then setting the value of the global node_api_module_features variable. To check enabled features, use the node_api_is_feature_enabled function.

For example, to disable node_api_feature_reference_all_types we can exclude its bit from the node_api_default_features:

node_api_features node_api_module_features =
    node_api_default_features & ~node_api_feature_reference_all_types;

Documentation

The n-api.md documentation is updated with the info about node_api_features enum and the node_api_is_feature_enabled function.

Testing

Added three new tests:

  • js-native-api/test_reference_all_types.
    The test stores napi_value of different types and then retrieves them for the strong and weak napi_ref references.
  • node-api/test_init_reference_all_types. The same as the previous test, but it uses NAPI_MODULE_INIT macro to initialize module instead of NAPI_MODULE. This is to verify that both NAPI_MODULE and NAPI_MODULE_INIT macro support feature specification.
  • node-api/test_init_reference_obj_only. To test old references behavior and module initialization with NAPI_MODULE_INIT macro.

The test_reference and test_init_reference_obj_only disable the node_api_feature_reference_all_types feature and make sure that the old napi_ref behavior continues to work.

The NAPI_EXPERIMENTAL is added to common.h and entry_point.c in test/js-native-api folder to make sure that the js-native-api tests always use the latest Node-API version features.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 1, 2022
@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label Apr 1, 2022
@vmoroz
Copy link
Member Author

vmoroz commented Apr 1, 2022

This PR was discussed today in the Node-API meeting and the suggestion was to reuse the existing napi_ref instead of adding the new napi_persistent type. I am going to add new versions of functions related to napi_ref that will have a flag that will control behavior of napi_ref depending on whether it supports the weak ref semantic:

  • With the weak ref semantic it will continue to work as today.
  • Without the weak ref semantic it will behave as ref counted Persistent value that can keep value of any type.

The new functions are going to have the same name as the current function, but to have prefix node_api_ instead of napi_, and to have a new flags parameter.

@legendecas legendecas added the blocked PRs that are blocked by other issues or PRs. label Apr 6, 2022
@legendecas
Copy link
Member

legendecas commented Apr 6, 2022

Adding "blocked" label as #42557 (comment) said to prevent unexpected landing. Feel free to remove the label when it is not the case anymore.

@vmoroz vmoroz changed the title node-api: add napi_persistent to keep value of any type node-api: allow napi_ref to keep value of any type Apr 6, 2022
@KevinEady
Copy link
Contributor

Hi @nodejs/node-api , can we get a new review after @vmoroz 's implementation changes that address the blocked issue re. #42557 (comment) ? Thanks

@vmoroz vmoroz changed the title node-api: allow napi_ref to keep value of any type node-api: add new napi_ref type for any napi_value Apr 26, 2022
doc/api/n-api.md Outdated

<!-- YAML
added: REPLACEME
-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc should indicated as experimental as well.

@KevinEady
Copy link
Contributor

We discussed in the 20 May Node-API meeting that @vmoroz would like to change the enum names for node_api_reftype_strong_or_weak and node_api_reftype_strong. If anyone has some suggestions, please feel free to comment on the PR. Thanks!

@vmoroz
Copy link
Member Author

vmoroz commented May 27, 2022

@mhdawson, @legendecas, @NickNaso, per our discussion today at the Node-API meeting, I had updated the PR description and added a note to the doc that node_api_create_reference is targeting to replace the napi_create_reference. The PR is ready to be reviewed and merged.

@legendecas legendecas removed the blocked PRs that are blocked by other issues or PRs. label May 28, 2022
@mhdawson
Copy link
Member

Some thoughts on naming. Maybe these would work?

  • node_api_reftype_any
  • node_api_reftype_object

Where we can explain the difference being that

  • for node_api_reftype_any, they can point to any type of node_api value, however, the application must fully manage the creation/deletion through the ref count and that when the ref count hits 0 the reference will be deleted.
  • for node_api_reftype_object, they can only point to node_api values for and object or related like functions and in addition to being able to fully manage the creation/deletion there is the additional option of allowing the gc to clean up the object and it's related reference when the reference is made weak by lowering the reference count to 0.

In addition to the naming are we sure that all JavaScript engines will support making persistent references to all types or have what's needed for the node-api implementation for the engine to be able implement something that looks like a reference?

@legendecas
Copy link
Member

legendecas commented Aug 9, 2022

AFAICT, the main blocker for supporting arbitrary values in napi_create_reference is the concern on various engine supports for the feature. I take a superficial dig into engines on the edge like JerryScript and found that they all support creating references on primitive values (with reference counting): https://jerryscript.net/api-reference/#jerry_acquire_value, and QuickJS JSValue https://bellard.org/quickjs/quickjs.html.

However, node-api implementation of napi_create_reference in the iotjs, a Node.js implementation based on JerryScript, is not supporting weak references: https://github.com/jerryscript-project/iotjs/blob/master/src/napi/node_api_lifetime.c#L210

As the weak reference is not guaranteed to be consistent, as those values can be collected anytime, I believe the support of create strong references on primitive values can meet the criteria New API should be agnostic towards the underlying JavaScript VM and "A new API addition should be simultaneously implemented in at least one other VM implementation of Node.js".

I believe it would be beneficial to strive to extend the maximum capability of existing APIs and try not to introduce new APIs for differences that are hard for end-users to choose between.

@vmoroz
Copy link
Member Author

vmoroz commented Sep 2, 2022

I believe it would be beneficial to strive to extend the maximum capability of existing APIs and try not to introduce new APIs for differences that are hard for end-users to choose between.

@legendecas, do you think that instead of adding new API we should just change the implementation of the existing references to support all value types and just say in the docs that the weak references work only object-like values?
I would expect that all JS engines must support weak reference semantics for objects to implement JS WeakRef.

@vmoroz
Copy link
Member Author

vmoroz commented Sep 2, 2022

Some thoughts on naming. Maybe these would work?

  • node_api_reftype_any
  • node_api_reftype_object

@mhdawson, I have changed the names.

@legendecas
Copy link
Member

do you think that instead of adding new API we should just change the implementation of the existing references to support all value types and just say in the docs that the weak references work only object-like values?

Yes, we should try to extend the current API. For documentation, we should not distinguish types but instead say that the behavior of the weak reference is determined by the engine implementation, and is not guaranteed to be able to get the referenced value when the ref-count is reached 0.

@KevinEady
Copy link
Contributor

Hi @nodejs/node-api ,

This PR introduces two changes: (1) allowing references to all values and (2) the ability for modules to request certain feature sets at module registration. @vmoroz has requested some feedback on both of these points if possible.

@vmoroz vmoroz changed the title node-api: add new napi_ref type for any napi_value node-api: enable napi_ref for all value types Oct 10, 2022
@vmoroz vmoroz marked this pull request as ready for review October 10, 2022 19:33
@vmoroz
Copy link
Member Author

vmoroz commented Oct 10, 2022

@mhdawson, @legendecas, I have updated the PR - please review.
It has the napi_ref updates, napi_features implementation, new unit tests, and updated documentation.

src/node_api.h Outdated
@@ -38,7 +38,12 @@ typedef struct napi_module {
napi_addon_register_func nm_register_func;
const char* nm_modname;
void* nm_priv;
#ifdef NAPI_EXPERIMENTAL
napi_features* nm_features;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a good start for node-api to get started with backward-compatible feature additions!

As you mentioned in the node-api meetings, this bit flag (an enum is an int size) may only represent 32 features at most. I'm wondering if it would be more extensible to save the module's defined NAPI_VERSION as nm_napi_version here like nm_version instead. In this way, node can refuse to load a node-api addon when an addon requires NAPI_VERSION 10, but the node is compiled as NAPI_VERSION 9.

The drawback of the alternative is that people have to pick up all feature changes with the new NAPI_VERSION, not part of it. But this could also be a relief that we don't need to maintain a long list of features, but a version support list instead.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of passing Node-API version used for a module. This way we can enforce the version compatibility.
The only drawback could be that developers will not be able to choose functionality depending on the Node-API version at runtime. I am not sure if anyone does it though.

As for the 32-bit feature set limit, the proposal is to pass not the feature bits to the module, but rather the pointer to the feature set. This way we can use the first 31 bits normally. In case if we need more, then we can set the 32nd bit and it will mean that the feature set has the second 32bit number, and the feature set pointer becomes a pointer to the feature set array with two elements. We can extend this array to be as long as needed. Obviously, each new entry in this array will require its own enum type. In practice I doubt that we ever exceed the 31 bit limit, but if we do, then we can extend it using this approach.

@@ -576,7 +576,9 @@ Reference::Reference(napi_env env, v8::Local<v8::Value> value, Args&&... args)
: RefBase(env, std::forward<Args>(args)...),
_persistent(env->isolate, value),
_secondPassParameter(new SecondPassCallParameterRef(this)),
_secondPassScheduled(false) {
_secondPassScheduled(false),
_canBeWeak(!env->IsFeatureEnabled(napi_feature_reference_all_types) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never allowed creating a reference on primitive values (except Symbols). So I believe there is no need to check the feature bit here: reference on primitive values can not be a weak reference.

Suggested change
_canBeWeak(!env->IsFeatureEnabled(napi_feature_reference_all_types) ||
_canBeWeak(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent here is to stop offering weak references for Symbols with the new flag and only do it for Objects and Functions to better match the JavaScript spec. But since currently we support weak references for Symbols and have unit tests in napi_references for them, I had to put the flag check here. This way the old code works without changes if the flag is not set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the _canBeWeak field completely to let V8 engine to decide on the weak references' behavior.
The only true weak references, as it used to be before, are object, external objects, and functions.
All other types are strong references when the ref count is 0.
This behavior can be seen in the new tests.
The only difference between what we used to have before and now is that we allow all types instead of only four (object, external object, function, and symbol) to be ref counted.

It almost feels like that allowing use of napi_ref for all value types may not need a special feature, but we should rather just extend the existing behavior as we did previously for Symbols.

Comment on lines +2062 to +2065
node_api_default_experimental_features = node_api_feature_reference_all_types,

// version specific
node_api_default_features = node_api_default_experimental_features,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For compatiblity I'm wondering if we should tie the defaults to a Node-API version versus the vesion of Node.js Unless the module author choses to opt-in, they behaviour should not change unless they have moved up to a newer Node-API vesion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking more closely I think that is the approach that is described, but but nothing is guarded because its under experimental. Will make a comment in src/js_native_api_types.h

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intent is to make it per Node-API version.

doc/api/n-api.md Outdated Show resolved Hide resolved
// Not only objects, external objects, functions, and symbols as before.
node_api_feature_reference_all_types = 1 << 0,
// Each version of Node-API is going to have its own default set of features.
node_api_default_experimental_features = node_api_feature_reference_all_types,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should have this, I think we should guard the availabilty to be able to turn on experimental features with NAPI_EXPERIMENTAL, but I'm not sure if having a set different than the default for the Node-API version makes sense. Otherwise somebody who wants to use any experimental API is also forced into to new defaults.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case if someone does not want to use the new experimental default, they can always override it per module.

doc/api/n-api.md Outdated
```

* `[in] env`: The environment that the API is invoked under.
* `[in] feature`: The feature that we want to test.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `[in] feature`: The feature that we want to test.
* `[in] feature`: The features that we want to test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed. Though I am not sure why we need to use plural form, while the parameter is singular. Should I also rename the parameter?

@@ -3257,3 +3259,12 @@ napi_status NAPI_CDECL napi_is_detached_arraybuffer(napi_env env,

return napi_clear_last_error(env);
}

napi_status NAPI_CDECL node_api_is_feature_enabled(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be in node_api.cc instead of this file. It is not tied to v8 right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not tied to V8. I was thinking that we want to have features available for different JS engines.

#else // NODE_API_CUSTOM_FEATURES

#define NODE_API_DEFINE_DEFAULT_FEATURES \
static node_api_features node_api_module_features = node_api_default_features;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments as before, I think this will end up setting to be specific to the Node.js version used to build versus the Node-API version selected. Should discuss this more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how to address this. Let's discuss it in the Node-API meeting. The concern as I understand it is that if we add more experimental features, we may fail to propagate them to the previous versions of Node.JS, and thus it may cause differences in the behavior between Node.JS versions, while we have the same version of Node-API, right?

#define NODE_API_DEFINE_DEFAULT_FEATURES
#define NODE_API_FEATURES_PTR
#endif // NAPI_EXPERIMENTAL

#define NAPI_MODULE_X(modname, regfunc, priv, flags) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just create a new NAPI_MODULE_X_FEATURES which takes the features parameter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be simpler - let me try.

@vmoroz
Copy link
Member Author

vmoroz commented May 7, 2023

Closing this PR because PR #45715 that was recently merged implements an alternative approach for changing existing behavior. It uses Node-API version instead of features. The PR #45715 also implements support for all value types supported by napi_ref.

@vmoroz vmoroz closed this May 7, 2023
@vmoroz vmoroz deleted the PR/Add_napi_permanent branch May 7, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants