addons: integrate workers with native addons #118

addaleax · 2017-10-22T20:26:18Z

Native addons need to use flags to indicate that they are capable of being loaded by worker threads.

Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

workers/addons

addaleax · 2017-10-24T21:47:06Z

@ayojs/core I realize this is quite a bit trickier to go through – happy to have a chat about this or something like that if you like :)

aqrln · 2017-11-01T15:30:42Z

src/node.cc

+      Environment* env;
+    };
+    env->AddCleanupHook([](void* arg) {
+      Mutex::ScopedLock  lock(dlib_mutex);


Nit: extra space after the type.

aqrln · 2017-11-01T15:38:21Z

src/node.cc

+
+  void AddEnvironment(Environment* env) {
+    if (users_.count(env) > 0) return;
+    users_.insert(env);


Not sure if this won't hurt readability, but these two lines can be merged into something like if (!users_.insert(env).second) return;.

Hm yeah, I think the current way is a bit more readable …

aqrln · 2017-11-01T15:46:59Z

src/node.cc

+  void AddEnvironment(Environment* env) {
+    if (users_.count(env) > 0) return;
+    users_.insert(env);
+    if (env->is_main_thread()) return;


Also, does the order of these checks matter, and if yes, shouldn't this one be the first one?

I think it matters, because non-main environments check whether the set is empty once they’re being torn down. The idea is that we preserve the current behaviour; if an addon is loaded by the main thread, it is never unloaded.

Okay, thanks for the explanation!

aqrln · 2017-11-02T23:48:49Z

src/node.cc

+    bool is_opened = dlib->Open();
+
+    if (is_opened) {
+      if (handle_to_dlib.count(dlib->handle_) > 0) {


Nit: if (is_opened && handle_to_dlib.count(dlib->handle_) > 0)

good catch, done!

aqrln · 2017-11-03T00:15:53Z

src/node.cc


-  CHECK_EQ(modpending, nullptr);
+  do {


Is there any particular reason to use the do { ... } while (false); pattern instead of a plain { ... } scope here?

Yeah, the fact that you can break; out of it if you discover that you don’t want to do most of the work here. The alternative would be having quite a sizeable block of code with 2 levels of extra indentation, which would also work

Oh, right. I just noticed the loop when I had opened the PR again, without really looking into its body this time, so I didn't notice the break statements, heh. Thanks for clarifying!

aqrln · 2017-11-03T00:18:57Z

@addaleax

I realize this is quite a bit trickier to go through – happy to have a chat about this or something like that if you like :)

I think that might be helpful. I mean, currently I'm just reviewing the code itself, but I don't think I'm really qualified to review the logic that happens here.

addaleax · 2017-11-03T00:25:10Z

@aqrln The only issue I see is that I’ll be at NodeConf EU for basically all of the next 7 days, so setting up a time for a hangout or so would probably be a bit hard. I guess there’s no rush for this to land tho

aqrln · 2017-11-03T00:34:50Z

@addaleax okay, whichever time works for you, especially if the PR can wait for a bit :)

Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment.

addaleax · 2017-11-15T21:15:26Z

@aqrln Okay, getting back on all of this … maybe schedule a call for the weekend, if that works with you?

aqrln · 2017-11-16T04:55:07Z

@addaleax Monday or Tuesday evening would be ideal, but if you prefer the weekend, I think I can find some time after 18:00 EET / 17:00 CET on Sunday.

addaleax · 2017-11-16T12:15:31Z

@aqrln I think Monday evening (>= 18:00 CET/17:00 UTC/19:00 EET) should be fine :)

aqrln · 2017-11-16T22:31:59Z

@addaleax great, thanks. How about 20:00 CET/19:00 UTC/21:00 EET?

addaleax · 2017-11-18T15:51:53Z

@aqrln Sounds good to me!

TimothyGu · 2017-11-20T23:53:38Z

src/node.cc

+  DLib& operator=(DLib&& other) = delete;
+
+  void AddEnvironment(Environment* env) {
+    if (users_.count(env) > 0) return;


Would access to users_ need to be protected by a mutex?

It is kinda protected by dlib_mutex, because this method is only called from DLOpen(), but it would be nice to actually restrict access to the method so that it couldn't be called from another place.

TimothyGu · 2017-11-20T23:56:07Z

test/addons/dlopen-ping-pong/test.js

-// process.dlopen() a require() call fails.
-const re = /^Error: Module did not self-register\.$/;
-assert.throws(() => require(`./build/${common.buildType}/binding`), re);
+assert.doesNotThrow(() => require(`./build/${common.buildType}/binding`));


Why did this change?

i'm assuming because DLOpen() doesn't throw on missing self-registration anymore, instead it just abandons

@TimothyGu It changed because re-dlopen()-ing a module will just give you the module again, like what you’d expect require() to do

ghost

looks good to me!

Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment. PR-URL: #118 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Olivia Hugger <olivia@fastmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

addaleax · 2017-12-07T15:52:42Z

Landed in fbd7c6e

Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment. Thanks go to Alexey Orlenko, Olivia Hugger and Stephen Belanger for reviewing the original version of this change. Refs: ayojs/ayo#118

addaleax added addons semver-minor worker labels Oct 22, 2017

Qard approved these changes Nov 1, 2017

View reviewed changes

aqrln reviewed Nov 1, 2017

View reviewed changes

addaleax force-pushed the workers-addons branch from cb9131a to 2b21717 Compare November 2, 2017 23:33

aqrln reviewed Nov 2, 2017

View reviewed changes

addaleax force-pushed the workers-addons branch from 2b21717 to 14c2355 Compare November 2, 2017 23:50

aqrln reviewed Nov 3, 2017

View reviewed changes

addons: integrate workers with native addons

66f85d9

Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment.

addaleax force-pushed the workers-addons branch from 14c2355 to 66f85d9 Compare November 15, 2017 21:15

aqrln approved these changes Nov 20, 2017

View reviewed changes

TimothyGu reviewed Nov 21, 2017

View reviewed changes

ghost approved these changes Nov 27, 2017

View reviewed changes

addaleax closed this Dec 7, 2017

addaleax deleted the workers-addons branch December 7, 2017 15:52

addaleax mentioned this pull request Apr 26, 2018

multi-thread support proposal nodejs/node#20239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

addons: integrate workers with native addons #118

addons: integrate workers with native addons #118

addaleax commented Oct 22, 2017

addaleax commented Oct 24, 2017

aqrln Nov 1, 2017

addaleax Nov 2, 2017

aqrln Nov 1, 2017

addaleax Nov 2, 2017

aqrln Nov 1, 2017

addaleax Nov 2, 2017

aqrln Nov 2, 2017

aqrln Nov 2, 2017

addaleax Nov 2, 2017

aqrln Nov 3, 2017

addaleax Nov 3, 2017

aqrln Nov 3, 2017 •

edited

Loading

aqrln commented Nov 3, 2017

addaleax commented Nov 3, 2017

aqrln commented Nov 3, 2017

addaleax commented Nov 15, 2017

aqrln commented Nov 16, 2017

addaleax commented Nov 16, 2017

aqrln commented Nov 16, 2017

addaleax commented Nov 18, 2017

TimothyGu Nov 20, 2017

aqrln Nov 21, 2017

TimothyGu Nov 20, 2017

ghost Nov 27, 2017

addaleax Dec 7, 2017

ghost left a comment

addaleax commented Dec 7, 2017

addons: integrate workers with native addons #118

addons: integrate workers with native addons #118

Conversation

addaleax commented Oct 22, 2017

Checklist

Affected core subsystem(s)

addaleax commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

aqrln commented Nov 3, 2017

addaleax commented Nov 3, 2017

aqrln commented Nov 3, 2017

addaleax commented Nov 15, 2017

aqrln commented Nov 16, 2017

addaleax commented Nov 16, 2017

aqrln commented Nov 16, 2017

addaleax commented Nov 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

addaleax commented Dec 7, 2017

aqrln Nov 3, 2017 •

edited

Loading