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

feat: add support for nogc types via BasicEnv #1514

Merged
merged 19 commits into from
Sep 3, 2024

Conversation

KevinEady
Copy link
Contributor

@KevinEady KevinEady commented Jun 6, 2024

  • Introduce Napi::NogcEnv class, taking the place of Napi::Env in finalizer callbacks.
  • Introduce void Napi::NogcEnv::AddPostFinalizer() and overloads.
  • Modify tests to support above changes

Closes: #1508

TODO:

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 69.38776% with 15 lines in your changes missing coverage. Please review.

Project coverage is 64.40%. Comparing base (12ffe91) to head (195ec28).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
napi-inl.h 70.83% 2 Missing and 12 partials ⚠️
napi.h 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1514      +/-   ##
==========================================
- Coverage   64.69%   64.40%   -0.30%     
==========================================
  Files           3        3              
  Lines        1997     2003       +6     
  Branches      687      693       +6     
==========================================
- Hits         1292     1290       -2     
- Misses        144      146       +2     
- Partials      561      567       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KevinEady
Copy link
Contributor Author

@vmoroz your comment on removing const ref on std::nullptr_t has been addressed, and @mhdawson your comment on reference deletion has been addressed, both in 2a689b6

matrix:
node-version: [ 18.x, 20.x, 21.x, 22.x ]
api_version:
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 add checks for the standard/experimental to ci-win.yml too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought... I pretty much copied Gabriel's CI file. I'll comment on his PR to verify, since mine would end up being overwritten when i rebase to master after his is merged.

@mhdawson
Copy link
Member

Agreed we should do a release to get out the previous fixe that @gabrielschulhof made and then put this out in a SemVer major.

@KevinEady KevinEady force-pushed the add-nogc-types branch 4 times, most recently from 674fa28 to 83af80c Compare June 15, 2024 20:57
@KevinEady
Copy link
Contributor Author

Hi team,

Using some SFINAE magic, I was able to determine at compile time how to associate the finalizer -- either directly or with node_api_post_finalizer. This would address #1367 as implemented.

This means no changes were needed to any existing tests, and I created a new test in finalizer_order to ensure finalizers are being called correctly.

PTAL! @gabrielschulhof @vmoroz @legendecas @mhdawson

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

Looks good!

I think the Gc vs. NoGc nomenclature is a bit confusing. Unfortunately, we established this in core. I think maybe we should go with DuringGc and OutsideGc. For env, we'd have DuringGcEnv and Env, since Env is already established, but for the finalizer type names I think we can go with DuringGc and OutsideGc for clarity.

if (isExperimental) {
assert.strictEqual(binding.finalizer_order.Test.isNogcFinalizerCalled, true, 'Expected nogc finalizer to be called [before ticking]');
assert.strictEqual(binding.finalizer_order.Test.isGcFinalizerCalled, false, 'Expected gc finalizer to not be called [before ticking]');
assert.strictEqual(isCallbackCalled, false, 'Expected callback not be called [before ticking]');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert.strictEqual(isCallbackCalled, false, 'Expected callback not be called [before ticking]');
assert.strictEqual(isCallbackCalled, false, 'Expected callback to not be called [before ticking]');

napi.h Outdated
@@ -2415,6 +2448,7 @@ class ObjectWrap : public InstanceWrap<T>, public Reference<Object> {
napi_property_attributes attributes = napi_default);
static Napi::Value OnCalledAsFunction(const Napi::CallbackInfo& callbackInfo);
virtual void Finalize(Napi::Env env);
virtual void Finalize(NogcEnv env);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
virtual void Finalize(NogcEnv env);
virtual void Finalize(Napi::NogcEnv env);

@gabrielschulhof
Copy link
Contributor

I tested this with Napi::External and it works great!

const { makeSync, makeAsync } = require('bindings')('test_sync_fini')

const doAsync = process.argv[2] === 'async'

;(function test(iter = 0) {
  for (let i = 0; i < 1000000; i++) {
    const id = `${iter}:${i}`
    if (doAsync) {
      makeAsync(id)
    } else {
      makeSync(id)
    }
  }
  setTimeout(test, 0, iter + 1)
})()

Here's a video of the output. Guess which column is async 🙂

Screen.Recording.2024-06-24.at.8.46.57.AM.mov

@gabrielschulhof
Copy link
Contributor

We should rename AddPostFinalizer to just PostFinalizer.

@KevinEady
Copy link
Contributor Author

@gabrielschulhof ,

For env, we'd have DuringGcEnv and Env, since Env is already established, ...

One could argue that being a Node-API wrapper, the naming in this library should be as consistent with Node-API as possible. The (almost) 1-to-1 mapping helps link up with other documentation (eg. referencing the Node-API docs), or translating between Node-API C code and node-addon-api C++ code.

I do agree though, the name DuringGcEnv better describes the restriction ("during GC") on how the environment can be used, but not sure if we should stray from the Node-API names.


... but for the finalizer type names I think we can go with DuringGc and OutsideGc for clarity.

By "finalizer type names" do you mean changing the Finalizer in places eg.:

template <typename Finalizer>
static External New(napi_env env, T* data, Finalizer finalizeCallback);

If so, I don't think we should change this name since the finalizer passed can accept either a DuringGcEnv or Env. A Finalizer is a callback ran either during the GC cycle (if using DuringGcEnv) or on a future tick after the GC cycle completes (if using Env). This means the concept of "finalization" is dependent on your callback signature, and would be documented as such.


We should rename AddPostFinalizer to just PostFinalizer.

Makes sense, esp. considering what I said previously.


While on the subject of documentation: we don't have any specific entry for "finalization", as a finalizer callback is always documented on each individual object or function, eg:

  • Napi::External:
    `Napi::External` objects can be created with an optional Finalizer function and optional Hint value. The Finalizer function, if specified, is called when your `Napi::External` object is released by Node's garbage collector. It gives your code the opportunity to free any dynamically created data. If you specify a Hint value, it is passed to your Finalizer function.
  • Napi::Buffer::New:
    - `[in] finalizeCallback`: The function to be called when the `Napi::Buffer` is
    destroyed. It must implement `operator()`, accept an Napi::Env, a `T*` (which is the
    external data pointer), and return `void`.
  • and others

I propose we create a new top-level documentation page for finalization describing the above, and can have each use of finalizers in the documentation link to this page.

napi.h Outdated
private:
napi_env _env;
class NogcEnv {
protected:
Copy link
Contributor Author

@KevinEady KevinEady Jul 4, 2024

Choose a reason for hiding this comment

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

Should we make this private and have Env be a friend class? With it being protected, the default finalizers (as well as _env) would be exposed in any user-defined classes that extend NogcEnv.

EDIT: I went with this private-friend approach, making this comment out-dated. Can discuss in meeting.

@KevinEady
Copy link
Contributor Author

I've added some WIP documentation on the top-level finalization page: https://github.com/nodejs/node-addon-api/blob/6591a3609c2b94f22956b72736e06b86586556cf/doc/finalization.md

@KevinEady
Copy link
Contributor Author

@mhdawson @gabrielschulhof as discussed in the 5 July Node-API meeting, I updated the docs to use the concept of a "basic finalizer" as a special categorization of finalizers, and removing the concept of "[a]synchronous". PTAL: https://github.com/nodejs/node-addon-api/blob/7d7b8211552216657494144d0417bf57b690871e/doc/finalization.md

Once we finalize (hah) this verbiage, I will update any associated type names in the headers + tests, and update the other docs that have any finalization features.

@gabrielschulhof
Copy link
Contributor

@KevinEady the doc LGTM 👍

@legendecas
Copy link
Member

legendecas commented Jul 12, 2024

(this is still a draft PR, is it ready for review?)

@mhdawson
Copy link
Member

doc LGTM with a few suggestions:

  1. I would remove

NOTE: Optimizations via basic finalizers will only occur if using NAPI_EXPERIMENTAL and the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT define flag has not been set. Otherwise, the engine will not differentiate between basic and (extended) finalizers.

there is never any promise that optimization will take place so I think we can just remove that.

  1. In terms of

In addition to passing finalizers to Napi::Externals and other Node-API constructs, use Napi::BasicEnv::PostFinalize(Napi::BasicEnv, Finalizer) to schedule a callback to run outside of the garbage collector finalization.

can we explain without promising it will run outside of gc collection? Maybe something like "to maximize the likelyhood that the finalization not being tied to garbage collection finalization" ...

@KevinEady
Copy link
Contributor Author

@legendecas the overall C++ work/implementation is finished, but the PR still needs some more work (markdown docs, test docs, ...)

@mhdawson

  1. I would remove

NOTE: Optimizations via basic finalizers will only occur if using NAPI_EXPERIMENTAL and the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT define flag has not been set. Otherwise, the engine will not differentiate between basic and (extended) finalizers.

there is never any promise that optimization will take place so I think we can just remove that.

With this, I was trying to convey that using basic finalizers only changes functionality in experimental mode. I could see someone using basic finalizers without experimental mode, running into #1213 and being a bit confused. Maybe we can use some other verbiage?

NOTE: Node-API will only differentiate between basic and (extended) finalizers when using NAPI_EXPERIMENTAL and the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT define flag has not been set. Otherwise, basic finalizers execute in the same manner as (extended) finalizers.

What do we think of that?


  1. In terms of

In addition to passing finalizers to Napi::Externals and other Node-API constructs, use Napi::BasicEnv::PostFinalize(Napi::BasicEnv, Finalizer) to schedule a callback to run outside of the garbage collector finalization.

can we explain without promising it will run outside of gc collection?

Well, I took inspiration from the Node-API docs for node_api_post_finalizer:

node_api_post_finalizer helps to work around this limitation by allowing the add-on to defer calls to such Node-APIs to a point in time outside of the GC finalization.

Also, when you say:

"to maximize the likelyhood that the finalization not being tied to garbage collection finalization"

The API is intended to guarantee the callback does not run inside GC, so saying "maximize the likelihood" is not completely accurate, as it 100% will not run inside GC finalization.

Since this API strictly deals with callbacks + GC finalization order, I think it's a good thing to keep.

@KevinEady
Copy link
Contributor Author

KevinEady commented Jul 19, 2024

In Node API meeting 19 July, we discussed:

  1. I would remove

NOTE: Optimizations via basic finalizers will only occur if using NAPI_EXPERIMENTAL and the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT define flag has not been set. Otherwise, the engine will not differentiate between basic and (extended) finalizers.

there is never any promise that optimization will take place so I think we can just remove that.

Yes, remove this.


  1. In terms of

In addition to passing finalizers to Napi::Externals and other Node-API constructs, use Napi::BasicEnv::PostFinalize(Napi::BasicEnv, Finalizer) to schedule a callback to run outside of the garbage collector finalization.

can we explain without promising it will run outside of gc collection?

The name node_api_post_finalizer can be made in the context of "running (non-basic) code after (post) a finalizer" , so maybe we can change the documentation in core (and also here) if needed, but the current wording is fine.


Additionally, we discussed adding an opt-in flag that requires -- at compilation time -- all finalizers are of the basic type, under the realization that:

  • if using the C-based Node-API, opting into basic finalizers requires all finalizers to be basic, providing devs a way to guarantee all of their finalizers would be run in the "maybe optimized" way and fail compilation if callbacks do not match the current basic finalizer signature.
  • if using this wrapper, (currently) there is no guarantee that all finalizers will be of the basic signature, meaning developers do not have an easy way to forcefully get compilation errors for finalizers that are not basic.

I suggest using a define: NODE_API_REQUIRE_BASIC_FINALIZERS NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS

@KevinEady KevinEady changed the title src: add support for nogc types via NogcEnv feat: add support for nogc types via BasicEnv Jul 25, 2024
@KevinEady KevinEady dismissed gabrielschulhof’s stale review August 6, 2024 13:27

Changes since last approval (22 June)

@KevinEady
Copy link
Contributor Author

The Windows experimental CI passed, eg: test (experimental, 20.x, x64, windows-2019)

Testing with Node-API Version '2147483647'.

@legendecas @mhdawson @gabrielschulhof @vmoroz PTAL! 👍

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Good work! Thanks

napi.h Outdated Show resolved Hide resolved
doc/finalization.md Outdated Show resolved Hide resolved
@KevinEady
Copy link
Contributor Author

Hi @gabrielschulhof , IIRC you mentioned you had an in-progress review. Any movement on that?

doc/basic_env.md Outdated Show resolved Hide resolved
doc/basic_env.md Outdated Show resolved Hide resolved
doc/basic_env.md Outdated Show resolved Hide resolved
doc/basic_env.md Outdated Show resolved Hide resolved
doc/basic_env.md Outdated Show resolved Hide resolved
doc/env.md Show resolved Hide resolved
doc/finalization.md Outdated Show resolved Hide resolved
doc/finalization.md Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member

mhdawson commented Sep 3, 2024

Looks like all comments have been addressed and CI is green. Landing.

@mhdawson mhdawson merged commit b4aeecb into nodejs:main Sep 3, 2024
48 checks passed
@KevinEady KevinEady deleted the add-nogc-types branch September 3, 2024 19:51
@KevinEady
Copy link
Contributor Author

Hi @legendecas ,

This PR was merged into main, but the release-please action failed: https://github.com/nodejs/node-addon-api/actions/runs/10687066491/job/29623803353

I think it has to do with permissions?

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

Successfully merging this pull request may close these issues.

Add wrappers for node_api_nogc_env and node_api_nogc_finalize
6 participants