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: render finalizers as env cleanup hooks #28428

Closed

Conversation

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jun 25, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@gabrielschulhof gabrielschulhof requested review from addaleax and mhdawson Jun 25, 2019
@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:track-all-finalizers branch from 42afad0 to a3955d6 Jun 25, 2019
Copy link
Member

addaleax left a comment

This might technically be considered a breaking change, although I’d 100 % agree that this is the right thing to do. Do we have a list of known or at least a list of relevant N-API packages, so that we can run more test suites against it than what CITGM provides?

src/js_native_api_v8.cc Outdated Show resolved Hide resolved
inline void SetWeak() {
_persistent.SetWeak(
this, FinalizeCallback, v8::WeakCallbackType::kParameter);
_env->track_finalizer(EnvCleanupHook, this);

This comment has been minimized.

Copy link
@addaleax

addaleax Jun 25, 2019

Member

I’d go further and suggest the behaviour that Node.js itself uses (via BaseObject): Always call finalizers at the end of the current Environment, not just when they’re weak references.

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof Jun 25, 2019

Author Contributor

I think we have to work these kinds of changes through the whole code base. For example, I still need to add this to v8impl::CallbackBundle as well. I'd like to make that a subclass of Reference.

@@ -25,6 +25,9 @@ struct napi_env__ {

virtual bool can_call_into_js() const { return true; }

virtual void track_finalizer(void (*finalizer)(void*), void* data) {}
virtual void untrack_finalizer(void (*finalizer)(void*), void* data) {}

This comment has been minimized.

Copy link
@addaleax

addaleax Jun 25, 2019

Member

style nit that can totally be ignored: These methods would more typically be called TrackFinalizer and UntrackFinalizer since they aren’t simple getters/setters.

Going even further, I think every implementor of N-API should support napi_add_env_cleanup_hook() and napi_remove_env_cleanup_hook(); they could totally be no-ops for some non-Node.js variants that do follow a one-env-per-process model like Node.js formerly did, but the concept of “end of execution of the current runtime instance” is not specific to Node.js and addons should handle it properly, regardless of the choice of the runtime.

If those methods are moved to js_native_api.h, we could use them directly instead, which might be nice.

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof Jun 25, 2019

Author Contributor

I guess the separation between node_api.h and js_native_api.h is not 100% clear. The initial thrust was to separate APIs related to JS values from ones related to the environment. Runtime-specific things like AsyncWorker and TSFN have thus also ended up in node_api.h.

This comment has been minimized.

Copy link
@addaleax

addaleax Jun 25, 2019

Member

The initial thrust was to separate APIs related to JS values from ones related to the environment.

I get that, but I would argue that because Agents are defined in ECMA-262, management of an Agent’s lifetime is also implicitly part of JS ;) Also, like I said, if runtimes don’t support any kind of cleanup, that’s fine and they can just make the functions have empty bodies.

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof Jun 25, 2019

Author Contributor

@addaleax I'll talk it over with @nodejs/n-api and make that into a separate PR.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jun 25, 2019

@addaleax should I include the change whereby I make the CallbackBundle a subclass of Reference so as to obtain env cleanup tracking into this PR?

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jun 25, 2019

@addaleax should I include the change whereby I make the CallbackBundle a subclass of Reference so as to obtain env cleanup tracking into this PR?

I think that makes sense, yes, if it’s not too much work

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:track-all-finalizers branch 2 times, most recently from af2c2e6 to aafa352 Jun 25, 2019
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jun 26, 2019

@addaleax why would this be considered a breaking change? If you provide a finalizer, you'd expect it to be called, right? So, as long as we call it within a valid environment, this should not break anything. What it might do though is expose bugs.

BTW: I have now changed CallbackBundle to manage its life cycle via a Reference.

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Jun 26, 2019

@gabrielf what are your thoughts in terms of performance implications?

Related, we had talked about excluding the main thread for performance reasons. Can you comment on the rational if this did not end up making sense?

src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jun 26, 2019

@mhdawson I have turned off tracking for the main thread, and untracking if the finalizer is being called from the env cleanup.

@gabrielschulhof gabrielschulhof mentioned this pull request Jul 1, 2019
4 of 4 tasks complete
@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:track-all-finalizers branch 2 times, most recently from 874bbec to 0b9b0f3 Jul 4, 2019
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jul 4, 2019

@mhdawson I added some counters to see how often an object gets tracked/untracked by having an env cleanup hook added/removed:

diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
index 5fe79f75bc..df4bd2e745 100644
--- a/src/js_native_api_v8.cc
+++ b/src/js_native_api_v8.cc
@@ -341,6 +341,7 @@ class Reference : private Finalizer {
   static void EnvCleanupHook(void* data) {
     Reference* reference = static_cast<Reference*>(data);
     reference->_persistent.Reset();
+    reference->_env->track_off_cleanup++;
     reference->RunFinalizer(false);
   }
 
diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h
index fffa4ba8df..732a170fe3 100644
--- a/src/js_native_api_v8.h
+++ b/src/js_native_api_v8.h
@@ -33,6 +33,11 @@ struct napi_env__ {
   int open_handle_scopes = 0;
   int open_callback_scopes = 0;
   int refs = 1;
+
+  bool track_hook_added = false;
+  size_t track_off = 0;
+  size_t track_on = 0;
+  size_t track_off_cleanup = 0;
 };
 
 static inline napi_status napi_clear_last_error(napi_env env) {
diff --git a/src/node_api.cc b/src/node_api.cc
index b3f7234a1e..d3ec223f18 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -26,10 +26,12 @@ struct node_napi_env__ : public napi_env__ {
   }
 
   void TrackFinalizer(void (*finalizer)(void*), void* data) override {
+    track_on++;
     node_env()->AddCleanupHook(finalizer, data);
   }
 
   void UntrackFinalizer(void (*finalizer)(void*), void* data) override {
+    track_off++;
     node_env()->RemoveCleanupHook(finalizer, data);
   }
 };
@@ -106,6 +108,16 @@ static inline napi_env GetEnv(v8::Local<v8::Context> context) {
         static_cast<void*>(result));
   }
 
+  if (!result->track_hook_added) {
+    result->track_hook_added = true;
+    result->node_env()->AddCleanupHook([](void* arg) {
+      napi_env env = static_cast<napi_env>(arg);
+      fprintf(stderr, "track_on: %lu\n", env->track_on);
+      fprintf(stderr, "track_off: %lu\n", env->track_off);
+      fprintf(stderr, "track_off_cleanup: %lu\n", env->track_off_cleanup);
+    }, result);
+  }
+
   return result;
 }
 

I then ran the leveldown benchmark from https://github.com/Level/bench and found that leveldown uses no weak references so, at least in its case, there is no interaction with the environment cleanup hooks via the new tracking mechanism.

I'll try and find a N-API addon that uses weak references and has a good benchmarking suite to find out what kind of effect the new tracking has.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jul 4, 2019

I'll keep searching with @NickNaso's help for a N-API addon that uses weak references and that has a good benchmarking suite.

I can't shake the thought though that a performant library would not leave matters of native memory management to the whim of the garbage collector, lest usage balloon out of control in certain cases. That is, in a tight enough loop, it's always better to use strong references with explicit end-of-life than weak references.

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Jul 4, 2019

@gabrielschulhof for some reason I was thinking this might have also affected some of the internal N-API infrastructure for calling functions etc. For example AsyncWrap in node-addon-api looks like it uses a Reference. So maybe look at something that uses AsyncWrap with node-addon-api and has a good benchmark suite?

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jul 5, 2019

@mhdawson I ran the async PI estimate which creates a single async worker and lets it self-destruct with the instrumented version of Node.js and it doesn't produce and weak references.

This change does affect the internal N-API and node-addon-api infrastructure for calling functions. For each binding created, a weak reference is created to the resulting JS function (or in the case of accessors to the object on which the accessor is defined). The purpose of the weak reference is to free the v8impl::CallbackBundle in the case of N-API and the Napi::CallbackInfo in the case of node-addon-api. With this change it attaches an env cleanup handler in addition to attaching a weak callback.

In the case of static bindings this will only affect startup and shutdown. However, in cases where we have function factories and the functions start going out of scope this change will bring with it the additional task of removing the env cleanup handler when the weak callback gets called.

@NickNaso do you happen to know any N-API or node-addon-api addons that use function factories?

@NickNaso

This comment has been minimized.

Copy link
Member

NickNaso commented Jul 5, 2019

@gabrielschulhof I have to search now I’m traveling I will do when I will came back to home on Sunday.

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Jul 8, 2019

This change does affect the internal N-API and node-addon-api infrastructure for calling functions

That is what I'd thought and was worried about with respect to perf. Having said that I guess in the current form it will only affect worker threads of than the check to see if we are running on the main thread or not.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jul 8, 2019

@mhdawson please note that this only affects the setup and teardown of bindings. It does not change the way a call from JS is passed through to the N-API addon. That's why I singled out function factories. In the case of function factories the setup/teardown may happen often during the life cycle of the addon, but for static bindings, the setup only happens once at init and the teardown only happens once at env cleanup.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Jul 8, 2019

@mhdawson I guess I misunderstood "internal N-API infrastructure for calling function".

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Jul 9, 2019

We might not have our terminology lined up yet but I think we talked about this in the recent N-API team meeting and agree that it's only in more limited cases where the extra cleanup will be invoked and only on worker threads.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jul 9, 2019

We might not have our terminology lined up yet but I think we talked about this in the recent N-API team meeting and agree that it's only in more limited cases where the extra cleanup will be invoked and only on worker threads.

… and embedder scenarios? Ideally, I would also like to see this enabled for a default Node.js main thread, but I can see that as a backwards compatibility concern.

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Jul 10, 2019

@addaleax that is a good point about embedder scenarios. Ideally we'd be able to do that only for embedder scenarios, possibly through an opt-in versus by default as that would minimize the potential performance/backward compatibility concerns.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 7, 2019

@mhdawson @addaleax may could you please review this whenever you get a chance?

Copy link
Member

mhdawson left a comment

LGTM once suggested additional comment is added about why there are 2 lists.

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:track-all-finalizers branch from bccf6b3 to 908cc59 Oct 10, 2019
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 10, 2019

@mhdawson I added two comments: One in the destructor where it loops over the reference lists, explaining why it must traverse the list of references containing napi_finalize callbacks first, and another where the two lists are declared, explaining that references with napi_finalize callbacks must be stored separately, and referring the reader to the destructor for the reason why.

@nodejs-github-bot

This comment has been minimized.

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Oct 11, 2019

@gabrielschulhof thanks, looks good :)

src/js_native_api_v8.cc Outdated Show resolved Hide resolved
src/js_native_api_v8_internals.h Outdated Show resolved Hide resolved
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:track-all-finalizers branch from 908cc59 to 4f39a84 Oct 13, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

gabrielschulhof added a commit that referenced this pull request Oct 13, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Oct 13, 2019

Landed in 53ca0b9.

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:track-all-finalizers branch Oct 13, 2019
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Oct 22, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: nodejs#28428
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Oct 22, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: nodejs#28428
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof gabrielschulhof mentioned this pull request Oct 26, 2019
2 of 2 tasks complete
targos added a commit that referenced this pull request Nov 8, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos added a commit that referenced this pull request Nov 10, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@addaleax addaleax mentioned this pull request Dec 7, 2019
2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.