addons: integrate workers with native addons by addaleax · Pull Request #118 · ayojs/ayo

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

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


Nit: extra space after the type.

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

+
+  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

+  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

+    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

+  Local<Object> module = args[0]->ToObject(env->isolate());

-  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

+  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

-// 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

Conversation

addaleax commented Oct 22, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aqrln Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aqrln commented Nov 3, 2017

Uh oh!

addaleax commented Nov 3, 2017

Uh oh!

aqrln commented Nov 3, 2017

Uh oh!

addaleax commented Nov 15, 2017

Uh oh!

aqrln commented Nov 16, 2017

Uh oh!

addaleax commented Nov 16, 2017

Uh oh!

aqrln commented Nov 16, 2017

Uh oh!

addaleax commented Nov 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 7, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aqrln Nov 3, 2017 •

edited

Loading