From b69261c3767e5c4708ca53f1e8ef3236543c236f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 22 Oct 2023 18:58:51 +0200 Subject: [PATCH 1/4] src: implement structuredClone in native Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots. --- benchmark/misc/structured-clone.js | 46 +++++++ .../bootstrap/web/exposed-window-or-worker.js | 7 +- lib/internal/perf/usertiming.js | 2 +- lib/internal/structured_clone.js | 29 ----- lib/internal/webstreams/readablestream.js | 4 +- src/node_messaging.cc | 112 ++++++++++++++---- test/parallel/test-structuredClone-global.js | 30 ++--- 7 files changed, 149 insertions(+), 81 deletions(-) create mode 100644 benchmark/misc/structured-clone.js delete mode 100644 lib/internal/structured_clone.js diff --git a/benchmark/misc/structured-clone.js b/benchmark/misc/structured-clone.js new file mode 100644 index 00000000000000..257671df44b07f --- /dev/null +++ b/benchmark/misc/structured-clone.js @@ -0,0 +1,46 @@ +'use strict'; + +const common = require('../common.js'); +const assert = require('assert'); + +const bench = common.createBenchmark(main, { + type: ['string', 'object', 'arraybuffer'], + n: [1e4], +}); + +function main({ n, type }) { + const data = []; + + switch (type) { + case 'string': + for (let i = 0; i < n; ++i) { + data.push(new Date().toISOString()); + } + break; + case 'object': + for (let i = 0; i < n; ++i) { + data.push({ ...process.config }); + } + break; + case 'arraybuffer': + for (let i = 0; i < n; ++i) { + data.push(new ArrayBuffer(10)); + } + break; + default: + throw new Error('Unsupported payload type'); + } + + const run = type === 'arraybuffer' ? (i) => { + data[i] = structuredClone(data[i], { transfer: [ data[i] ] }); + } : (i) => { + data[i] = structuredClone(data[i]); + }; + + bench.start(); + for (let i = 0; i < n; ++i) { + run(i); + } + bench.end(n); + assert.strictEqual(data.length, n); +} diff --git a/lib/internal/bootstrap/web/exposed-window-or-worker.js b/lib/internal/bootstrap/web/exposed-window-or-worker.js index 3cc555b82f35a7..f7298a9a9028df 100644 --- a/lib/internal/bootstrap/web/exposed-window-or-worker.js +++ b/lib/internal/bootstrap/web/exposed-window-or-worker.js @@ -31,11 +31,8 @@ const { } = require('internal/process/task_queues'); defineOperation(globalThis, 'queueMicrotask', queueMicrotask); -defineLazyProperties( - globalThis, - 'internal/structured_clone', - ['structuredClone'], -); +const { structuredClone } = internalBinding('messaging'); +defineOperation(globalThis, 'structuredClone', structuredClone); defineLazyProperties(globalThis, 'buffer', ['atob', 'btoa']); // https://html.spec.whatwg.org/multipage/web-messaging.html#broadcasting-to-other-browsing-contexts diff --git a/lib/internal/perf/usertiming.js b/lib/internal/perf/usertiming.js index ff417d94700550..cb7cfdd8596369 100644 --- a/lib/internal/perf/usertiming.js +++ b/lib/internal/perf/usertiming.js @@ -31,7 +31,7 @@ const { }, } = require('internal/errors'); -const { structuredClone } = require('internal/structured_clone'); +const { structuredClone } = internalBinding('messaging'); const { lazyDOMException, kEnumerableProperty, diff --git a/lib/internal/structured_clone.js b/lib/internal/structured_clone.js deleted file mode 100644 index 0392232badf9fc..00000000000000 --- a/lib/internal/structured_clone.js +++ /dev/null @@ -1,29 +0,0 @@ -'use strict'; - -const { - codes: { ERR_MISSING_ARGS }, -} = require('internal/errors'); - -const { - MessageChannel, - receiveMessageOnPort, -} = require('internal/worker/io'); - -let channel; -function structuredClone(value, options = undefined) { - if (arguments.length === 0) { - throw new ERR_MISSING_ARGS('value'); - } - - // TODO: Improve this with a more efficient solution that avoids - // instantiating a MessageChannel - channel ??= new MessageChannel(); - channel.port1.unref(); - channel.port2.unref(); - channel.port1.postMessage(value, options?.transfer); - return receiveMessageOnPort(channel.port2).message; -} - -module.exports = { - structuredClone, -}; diff --git a/lib/internal/webstreams/readablestream.js b/lib/internal/webstreams/readablestream.js index 83b49028eae8e0..a5f95596e3082a 100644 --- a/lib/internal/webstreams/readablestream.js +++ b/lib/internal/webstreams/readablestream.js @@ -85,9 +85,7 @@ const { kControllerErrorFunction, } = require('internal/streams/utils'); -const { - structuredClone, -} = require('internal/structured_clone'); +const { structuredClone } = internalBinding('messaging'); const { ArrayBufferViewGetBuffer, diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 908e76f399557d..3ee981b8652e6c 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -1008,6 +1008,47 @@ static Maybe ReadIterable(Environment* env, return Just(true); } +bool GetTransferList(Environment* env, + Local context, + Local transfer_list_v, + TransferList* transfer_list_out) { + if (transfer_list_v->IsNullOrUndefined()) { + // Browsers ignore null or undefined, and otherwise accept an array or an + // options object. + return true; + } + + if (!transfer_list_v->IsObject()) { + THROW_ERR_INVALID_ARG_TYPE( + env, "Optional transferList argument must be an iterable"); + return false; + } + + bool was_iterable; + if (!ReadIterable(env, context, *transfer_list_out, transfer_list_v) + .To(&was_iterable)) + return false; + if (!was_iterable) { + Local transfer_option; + if (!transfer_list_v.As() + ->Get(context, env->transfer_string()) + .ToLocal(&transfer_option)) + return false; + if (!transfer_option->IsUndefined()) { + if (!ReadIterable(env, context, *transfer_list_out, transfer_option) + .To(&was_iterable)) + return false; + if (!was_iterable) { + THROW_ERR_INVALID_ARG_TYPE( + env, "Optional options.transfer argument must be an iterable"); + return false; + } + } + } + + return true; +} + void MessagePort::PostMessage(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Local obj = args.This(); @@ -1018,33 +1059,10 @@ void MessagePort::PostMessage(const FunctionCallbackInfo& args) { "MessagePort.postMessage"); } - if (!args[1]->IsNullOrUndefined() && !args[1]->IsObject()) { - // Browsers ignore null or undefined, and otherwise accept an array or an - // options object. - return THROW_ERR_INVALID_ARG_TYPE(env, - "Optional transferList argument must be an iterable"); - } - TransferList transfer_list; - if (args[1]->IsObject()) { - bool was_iterable; - if (!ReadIterable(env, context, transfer_list, args[1]).To(&was_iterable)) - return; - if (!was_iterable) { - Local transfer_option; - if (!args[1].As()->Get(context, env->transfer_string()) - .ToLocal(&transfer_option)) return; - if (!transfer_option->IsUndefined()) { - if (!ReadIterable(env, context, transfer_list, transfer_option) - .To(&was_iterable)) return; - if (!was_iterable) { - return THROW_ERR_INVALID_ARG_TYPE(env, - "Optional options.transfer argument must be an iterable"); - } - } - } + if (!GetTransferList(env, context, args[1], &transfer_list)) { + return; } - MessagePort* port = Unwrap(args.This()); // Even if the backing MessagePort object has already been deleted, we still // want to serialize the message to ensure spec-compliant behavior w.r.t. @@ -1535,6 +1553,48 @@ static void SetDeserializerCreateObjectFunction( env->set_messaging_deserialize_create_object(args[0].As()); } +static void StructuredClone(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + Realm* realm = Realm::GetCurrent(context); + Environment* env = realm->env(); + + if (args.Length() == 0) { + return THROW_ERR_MISSING_ARGS(env, "The value argument must be specified"); + } + + Local value = args[0]; + + TransferList transfer_list; + if (!args[1]->IsNullOrUndefined()) { + if (!args[1]->IsObject()) { + return THROW_ERR_INVALID_ARG_TYPE( + env, "The options argument must be either an object or undefined"); + } + Local options = args[1].As(); + Local transfer_list_v; + if (!options->Get(context, FIXED_ONE_BYTE_STRING(isolate, "transfer")) + .ToLocal(&transfer_list_v)) { + return; + } + + // TODO(joyeecheung): implement this in JS land to avoid the C++ -> JS + // cost to convert a sequence into an array. + if (!GetTransferList(env, context, transfer_list_v, &transfer_list)) { + return; + } + } + + std::shared_ptr msg = std::make_shared(); + Local result; + if (msg->Serialize(env, context, value, transfer_list, Local()) + .IsNothing() || + !msg->Deserialize(env, context, nullptr).ToLocal(&result)) { + return; + } + args.GetReturnValue().Set(result); +} + static void MessageChannel(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (!args.IsConstructCall()) { @@ -1615,6 +1675,7 @@ static void InitMessaging(Local target, "setDeserializerCreateObjectFunction", SetDeserializerCreateObjectFunction); SetMethod(context, target, "broadcastChannel", BroadcastChannel); + SetMethod(context, target, "structuredClone", StructuredClone); { Local domexception = GetDOMException(context).ToLocalChecked(); @@ -1638,6 +1699,7 @@ static void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(MessagePort::ReceiveMessage); registry->Register(MessagePort::MoveToContext); registry->Register(SetDeserializerCreateObjectFunction); + registry->Register(StructuredClone); } } // anonymous namespace diff --git a/test/parallel/test-structuredClone-global.js b/test/parallel/test-structuredClone-global.js index 95dab1e8e8963b..2b49a7a2e70696 100644 --- a/test/parallel/test-structuredClone-global.js +++ b/test/parallel/test-structuredClone-global.js @@ -1,23 +1,17 @@ -// Flags: --expose-internals 'use strict'; -/* eslint-disable no-global-assign */ require('../common'); +const assert = require('assert'); -const { - structuredClone: _structuredClone, -} = require('internal/structured_clone'); +assert.throws(() => structuredClone(), { code: 'ERR_MISSING_ARGS' }); +assert.throws(() => structuredClone(undefined, ''), { code: 'ERR_INVALID_ARG_TYPE' }); +assert.throws(() => structuredClone(undefined, 1), { code: 'ERR_INVALID_ARG_TYPE' }); +assert.throws(() => structuredClone(undefined, { transfer: 1 }), { code: 'ERR_INVALID_ARG_TYPE' }); +assert.throws(() => structuredClone(undefined, { transfer: '' }), { code: 'ERR_INVALID_ARG_TYPE' }); -const { - strictEqual, - throws, -} = require('assert'); - -strictEqual(globalThis.structuredClone, _structuredClone); -structuredClone = undefined; -strictEqual(globalThis.structuredClone, undefined); - -// Restore the value for the known globals check. -structuredClone = _structuredClone; - -throws(() => _structuredClone(), /ERR_MISSING_ARGS/); +// Options can be null or undefined. +assert.strictEqual(structuredClone(undefined), undefined); +assert.strictEqual(structuredClone(undefined, null), undefined); +// Transfer can be null or undefined. +assert.strictEqual(structuredClone(undefined, { transfer: null }), undefined); +assert.strictEqual(structuredClone(undefined, { }), undefined); From 571f8b977313bb39e82523377ddc99a95967c910 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 24 Oct 2023 23:02:54 +0200 Subject: [PATCH 2/4] fixup! src: implement structuredClone in native --- lib/.eslintrc.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index 5b7fa7d3882d7d..6b716e0a3d84ac 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -171,8 +171,6 @@ rules: message: Use `const { setInterval } = require('timers');` instead of the global. - name: setTimeout message: Use `const { setTimeout } = require('timers');` instead of the global. - - name: structuredClone - message: Use `const { structuredClone } = require('internal/structured_clone');` instead of the global. - name: SubtleCrypto message: Use `const { SubtleCrypto } = require('internal/crypto/webcrypto');` instead of the global. no-restricted-modules: From f3dc8f40565c226b2bf56a4da1dc40f272d773ed Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 25 Oct 2023 13:29:09 +0200 Subject: [PATCH 3/4] fixup! fixup! src: implement structuredClone in native --- lib/.eslintrc.yaml | 2 ++ src/node_messaging.cc | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index 6b716e0a3d84ac..5b7fa7d3882d7d 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -171,6 +171,8 @@ rules: message: Use `const { setInterval } = require('timers');` instead of the global. - name: setTimeout message: Use `const { setTimeout } = require('timers');` instead of the global. + - name: structuredClone + message: Use `const { structuredClone } = require('internal/structured_clone');` instead of the global. - name: SubtleCrypto message: Use `const { SubtleCrypto } = require('internal/crypto/webcrypto');` instead of the global. no-restricted-modules: diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 3ee981b8652e6c..571b271b69605a 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -1573,7 +1573,7 @@ static void StructuredClone(const FunctionCallbackInfo& args) { } Local options = args[1].As(); Local transfer_list_v; - if (!options->Get(context, FIXED_ONE_BYTE_STRING(isolate, "transfer")) + if (!options->Get(context, env->transfer_string()) .ToLocal(&transfer_list_v)) { return; } From 0a090f59e66dbbc09ccc6b4fc1b07142d5eee7da Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 25 Oct 2023 13:56:25 +0200 Subject: [PATCH 4/4] fixup! fixup! fixup! src: implement structuredClone in native --- lib/.eslintrc.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index 5b7fa7d3882d7d..c197cedfeab3db 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -172,7 +172,7 @@ rules: - name: setTimeout message: Use `const { setTimeout } = require('timers');` instead of the global. - name: structuredClone - message: Use `const { structuredClone } = require('internal/structured_clone');` instead of the global. + message: Use `const { structuredClone } = internalBinding('messaging');` instead of the global. - name: SubtleCrypto message: Use `const { SubtleCrypto } = require('internal/crypto/webcrypto');` instead of the global. no-restricted-modules: