worker: implement SharedArrayBuffer/MessagePort transferring #106

addaleax · 2017-10-15T19:55:56Z

These have already been reviewed by @Qard but I would like to give them a bit of public review time on their own :) They can be merged pretty soon, though.

addaleax · 2017-10-15T20:12:07Z

src/sharedarraybuffer-metadata.h

+  // Create a SharedArrayBuffer object for a specific Environment and Context.
+  // The created SharedArrayBuffer will be in externalized mode and has
+  // a hidden object attached to it, during whose lifetime the reference
+  // count is increased by 1.


@Qard pointed out in #58 (rightfully) that this is somewhat awkward phrasing … I’ll need a bit of time to come up with sth better, I am afraid 😄

Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. PR-URL: #106 Reviewed-By: Stephen Belanger <[email protected]>

Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. PR-URL: #106 Reviewed-By: Stephen Belanger <[email protected]>

TimothyGu · 2017-10-16T22:43:56Z

src/sharedarraybuffer-metadata.cc

+         ->HasInstance(lifetime_partner)) {
+    if (!source->IsExternal()) {
+      env->ThrowError("Found internalized SharedArrayBuffer with "
+                      "lifetime partner object");


Assume this is an internal bug if this happens? If so we should make it clear that this error should be reported back to us.

Hm, I guess so, yes … you could mess with this using process.binding() but I don’t think that really counts :)

TimothyGu · 2017-10-16T22:44:04Z

src/sharedarraybuffer-metadata.cc

+    // If this is an external SharedArrayBuffer but we do not see a lifetime
+    // partner object, we did not externalize it. In that case, there is no
+    // way to serialize it.
+    env->ThrowError("Cannot serialize externalized SharedArrayBuffer");


Tests would be nice although not necessary. TypeError.

Yeah, right. I think this is going to require an addon test

TimothyGu · 2017-10-16T22:44:43Z

src/node_messaging.cc

+      return WriteMessagePort(Unwrap<MessagePort>(object));
+    }
+
+    env_->ThrowError("Cannot serialize unknown type of host object");


Tests would be nice although not necessary.

Yeah, I know it’s kind of unfortunate that they are in a separate commit – the thing is, it just makes a lot more sense to test these methods when actually transferring data between workers…

TimothyGu · 2017-10-16T22:46:04Z

src/node_messaging.cc

+      }
+    }
+
+    env_->ThrowError("MessagePort was not listed in transferList");


Test needed. Also TypeError.

Hm, at least Firefox throws a special DataCloneError, which is a DOMException and therefore a plain Error…

DOMException isn't really encouraged for new errors any more. TypeErrors and other error types that map to JS are the future for the web. Plus we don't really care about compatibility, do we?

Yeah, not really the point :) Still, why TypeError? I don’t really think the semantics here are “this thing has the wrong type”, more like, “you missed an entry in a list, please add it”, which doesn’t seem like a type mismatch to me…

Plus we don't really care about compatibility, do we?

Well … for MessagePorts/MessageChannels we kind of do? 😄 Maybe not in this first iteration but it should be doable to get some degree of compatibility

Still, why TypeError?

TypeError is used to indicate an unsuccessful operation when none of the other NativeError objects are an appropriate indication of the failure cause.

https://tc39.github.io/ecma262/#sec-native-error-types-used-in-this-standard-typeerror

Either way I just realized require('v8').Serializer.prototype._getDataCloneError is also Error, so I guess consistency trumps all.

https://tc39.github.io/ecma262/#sec-native-error-types-used-in-this-standard-typeerror

I think that’s because the language spec isn’t really concerned with these kinds of errors?

Either way I just realized require('v8').Serializer.prototype._getDataCloneError is also Error, so I guess consistency trumps all.

I mean … that’s because I wrote it that way. 😄 We definitely can change that if you think it makes sense

TimothyGu · 2017-10-16T23:00:30Z

src/node_messaging.cc

+    }
+    seen_shared_array_buffers_.push_back(shared_array_buffer);
+    msg_->AddSharedArrayBuffer(reference);
+    return Just(i);


Is the returned ID actually used anywhere in Ayo code or is it just for V8?

Only for V8, to match the objects, really… the matching call on the deserializing side is deserializer.TransferSharedArrayBuffer(i, sab);

TimothyGu · 2017-10-17T00:08:20Z

Sorry about my late review... I've been kinda busy the past couple of days.

Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106

Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106

Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106

Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106

Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106 PR-URL: #20876 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106 PR-URL: #20876 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106 PR-URL: #20876 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106 PR-URL: #20876 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106 PR-URL: #20876 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Logic is added to the `MessagePort` mechanism that attaches hidden objects to those instances when they are transferred that track their lifetime and maintain a reference count, to make sure that memory is freed at the appropriate times. Thanks to Stephen Belanger for reviewing this change in its original PR. Refs: ayojs/ayo#106 PR-URL: #20876 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

addaleax added the worker label Oct 15, 2017

Qard approved these changes Oct 15, 2017

View reviewed changes

addaleax commented Oct 15, 2017

View reviewed changes

addaleax added 2 commits October 16, 2017 21:25

worker: support MessagePort passing in messages

734866f

Support passing `MessagePort` instances through other `MessagePort`s, as expected by the `MessagePort` spec. PR-URL: #106 Reviewed-By: Stephen Belanger <[email protected]>

addaleax force-pushed the messageport-transfer branch from df2416b to b8ff855 Compare October 16, 2017 19:25

addaleax merged commit b8ff855 into latest Oct 16, 2017

addaleax deleted the messageport-transfer branch October 16, 2017 19:31

TimothyGu reviewed Oct 16, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worker: implement SharedArrayBuffer/MessagePort transferring #106

worker: implement SharedArrayBuffer/MessagePort transferring #106

addaleax commented Oct 15, 2017

addaleax Oct 15, 2017

TimothyGu Oct 16, 2017

addaleax Oct 17, 2017

TimothyGu Oct 16, 2017

addaleax Oct 17, 2017

TimothyGu Oct 16, 2017

addaleax Oct 17, 2017

TimothyGu Oct 16, 2017

addaleax Oct 17, 2017

TimothyGu Oct 17, 2017

addaleax Oct 17, 2017

TimothyGu Oct 17, 2017

addaleax Oct 17, 2017

TimothyGu Oct 16, 2017

addaleax Oct 17, 2017

TimothyGu commented Oct 17, 2017

worker: implement SharedArrayBuffer/MessagePort transferring #106

worker: implement SharedArrayBuffer/MessagePort transferring #106

Conversation

addaleax commented Oct 15, 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

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

TimothyGu commented Oct 17, 2017