src: implement structuredClone in native by joyeecheung · Pull Request #50330 · nodejs/node · GitHub

Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots.

Local benchmark numbers:

                                                    confidence improvement accuracy (*)   (**)  (***)
misc/structured-clone.js n=10000 type='arraybuffer'        ***     12.79 %       ±1.72% ±2.29% ±2.99%
misc/structured-clone.js n=10000 type='object'             ***      7.65 %       ±2.22% ±2.96% ±3.85%
misc/structured-clone.js n=10000 type='string'             ***     75.01 %       ±3.84% ±5.14% ±6.75%

Review requested:

@nodejs/crypto
@nodejs/net
@nodejs/startup

CI: https://ci.nodejs.org/job/node-test-pull-request/55129/

will this fix #49940 too?

lgtm

Am I correct in assuming this will improve all messaging through worker threads?

@mcollina no, if you look at the changes this instead of creating a MessageChannel object in JS land creates it in C++ land - basically moving that part from JS to C++, see https://github.com/nodejs/node/pull/50330/files#diff-512ab8348327408c9a5ba3f7c60d8f8977a672a22b96345b8cfc2562936062f5R1582-R1583

FYI this is not yet ready because I want to see if we can just avoid the message ports all together without doing a lot of refactoring (there are a lot of code in place to check that the port isn't in the arguments, which then rely on having a port instance to check on) - if not, I'll just try getting this in first before following up with a PR to get rid of the message ports

From what I can tell simply using the existing Message::Serialize/Message::Deserialize should yield equivalent results. This should also get rid of the previous peculiar behavior of emitting deserialization errors as messageerror events instead of throwing them directly.

CI: https://ci.nodejs.org/job/node-test-pull-request/55182/

will this fix #49940 too?

I don't think so, the assertion comes from the ReadIterable implementation and that's still unfixed - on the other hand it might be better to just implement that method in JS land and do a glue of structuredClone with that method. This calls into JS so many times that doing it in C++ would be slower. I can do that as a follow-up.

I removed the messaging namespace rename (this should still be done in another PR though) and the BindingData because it now no longer relies on MessagePorts. The benchmark numbers are improved a bit too due to the removal of unnecessary hoops:

                                                    confidence improvement accuracy (*)   (**)  (***)
misc/structured-clone.js n=10000 type='arraybuffer'        ***     12.79 %       ±1.72% ±2.29% ±2.99%
misc/structured-clone.js n=10000 type='object'             ***      7.65 %       ±2.22% ±2.96% ±3.85%
misc/structured-clone.js n=10000 type='string'             ***     75.01 %       ±3.84% ±5.14% ±6.75%

This should be ready for review now - I'd appreciate another pair of eyeballs to confirm that this still yields equivalent results.

CI: https://ci.nodejs.org/job/node-test-pull-request/55186/

Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots.

Can you please update the message in the linter error:

node/lib/.eslintrc.yaml

Lines 174 to 175 in fd21429

    
           - name: structuredClone 
        
             message: Use `const { structuredClone } = require('internal/structured_clone');` instead of the global.

CI: https://ci.nodejs.org/job/node-test-pull-request/55224/

CI: https://ci.nodejs.org/job/node-test-pull-request/55232/

Landed in c3a41d8

Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots. PR-URL: #50330 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>

This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: #55317 Fixes: #55280 Refs: #50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: nodejs#55317 Fixes: nodejs#55280 Refs: nodejs#50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

This commit lets `tranfer` passed to `structuredClone` get validated at JS layer by doing webidl conversion. This avoids the C++ to JS function call overhead in the native implementaiton of `structuredClone` PR-URL: #55317 Fixes: #55280 Refs: #50330 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 22, 2023

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2023

mcollina approved these changes Oct 22, 2023

View reviewed changes

benjamingr approved these changes Oct 23, 2023

View reviewed changes

legendecas reviewed Oct 24, 2023

View reviewed changes

Comment thread src/node_messaging.cc Outdated

joyeecheung force-pushed the native-structured-clone branch from d04776a to 81ff9d9 Compare October 24, 2023 10:13

joyeecheung force-pushed the native-structured-clone branch from 81ff9d9 to d7538ee Compare October 24, 2023 12:43

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023

joyeecheung marked this pull request as ready for review October 24, 2023 13:24

joyeecheung force-pushed the native-structured-clone branch from d7538ee to 7d8ecdd Compare October 24, 2023 13:30

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023

src: implement structuredClone in native

b69261c

Simplify the implementation by implementing it directly in C++. This improves performance and also makes structuredClone supported in custom snapshots.

joyeecheung force-pushed the native-structured-clone branch from 7d8ecdd to b69261c Compare October 24, 2023 17:53

aduh95 reviewed Oct 24, 2023

View reviewed changes

benjamingr approved these changes Oct 24, 2023

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2023

aduh95 approved these changes Oct 25, 2023

View reviewed changes

daeyeon approved these changes Oct 25, 2023

View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 25, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2023

nodejs-github-bot merged commit c3a41d8 into nodejs:main Oct 25, 2023

joyeecheung mentioned this pull request Oct 26, 2023

Improve performance of structuredClone #50320

Closed

This was referenced Oct 26, 2023

CI Reliability 2023-10-26 nodejs/reliability#698

Open

CI Reliability 2023-10-27 nodejs/reliability#699

Open

CI Reliability 2023-10-28 nodejs/reliability#700

Open

targos mentioned this pull request Nov 12, 2023

v21.2.0 release proposal #50681

Merged

UlisesGascon mentioned this pull request Dec 12, 2023

v20.11.0 proposal #51124

Merged

jazelly mentioned this pull request Oct 8, 2024

lib: convert transfer sequence to array in js #55317

Merged

jazelly mentioned this pull request Nov 27, 2024

[v22.x backport] lib: convert transfer sequence to array in js #56015

Closed

	- name: structuredClone
	message: Use `const { structuredClone } = require('internal/structured_clone');` instead of the global.

src: implement structuredClone in native by joyeecheung · Pull Request #50330 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: implement structuredClone in native - #50330

src: implement structuredClone in native#50330nodejs-github-bot merged 4 commits intonodejs:mainnodejs/node:mainfrom joyeecheung:native-structured-clonejoyeecheung/node:native-structured-cloneCopy head branch name to clipboard

Conversation

joyeecheung commented Oct 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 22, 2023

Uh oh!

nodejs-github-bot commented Oct 22, 2023

Uh oh!

KhafraDev commented Oct 22, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Oct 23, 2023

Uh oh!

Uh oh!

joyeecheung commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 24, 2023

Uh oh!

joyeecheung commented Oct 24, 2023

Uh oh!

joyeecheung commented Oct 24, 2023

Uh oh!

nodejs-github-bot commented Oct 24, 2023

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: implement structuredClone in native#50330
nodejs-github-bot merged 4 commits into
nodejs:mainfrom
joyeecheung:native-structured-clone

joyeecheung commented Oct 22, 2023 •
edited
Loading

joyeecheung commented Oct 24, 2023 •
edited
Loading

joyeecheung commented Oct 24, 2023 •
edited
Loading