src: implement structuredClone in native #50330

joyeecheung · 2023-10-22T19:04:56Z

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%

nodejs-github-bot · 2023-10-22T19:05:01Z

Review requested:

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

nodejs-github-bot · 2023-10-22T19:10:28Z

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

KhafraDev · 2023-10-22T20:05:59Z

will this fix #49940 too?

mcollina

lgtm

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

benjamingr · 2023-10-23T20:19:48Z

@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

src/node_messaging.cc

joyeecheung · 2023-10-24T10:16:31Z

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

joyeecheung · 2023-10-24T12:48:41Z

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.

nodejs-github-bot · 2023-10-24T12:54:47Z

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

joyeecheung · 2023-10-24T12:57:54Z

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.

joyeecheung · 2023-10-24T13:24:57Z

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.

nodejs-github-bot · 2023-10-24T13:33:25Z

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.

aduh95

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.

lib/.eslintrc.yaml

nodejs-github-bot · 2023-10-25T11:37:30Z

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

nodejs-github-bot · 2023-10-25T12:00:12Z

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

nodejs-github-bot · 2023-10-25T15:04:26Z

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

nodejs-github-bot · 2023-10-25T17:45:13Z

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: nodejs#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>

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>

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>

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

src/node_messaging.cc Show resolved Hide resolved

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

aduh95 reviewed Oct 25, 2023

View reviewed changes

lib/.eslintrc.yaml Outdated Show resolved Hide resolved

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

fixup! fixup! fixup! src: implement structuredClone in native

0a090f5

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
56 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: implement structuredClone in native #50330

src: implement structuredClone in native #50330

joyeecheung commented Oct 22, 2023 •

edited

Loading

nodejs-github-bot commented Oct 22, 2023

nodejs-github-bot commented Oct 22, 2023

KhafraDev commented Oct 22, 2023

mcollina left a comment

benjamingr commented Oct 23, 2023

joyeecheung commented Oct 24, 2023 •

edited

Loading

joyeecheung commented Oct 24, 2023 •

edited

Loading

nodejs-github-bot commented Oct 24, 2023

joyeecheung commented Oct 24, 2023

joyeecheung commented Oct 24, 2023

nodejs-github-bot commented Oct 24, 2023

aduh95 left a comment

nodejs-github-bot commented Oct 25, 2023

nodejs-github-bot commented Oct 25, 2023

nodejs-github-bot commented Oct 25, 2023

nodejs-github-bot commented Oct 25, 2023

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

src: implement structuredClone in native #50330

src: implement structuredClone in native #50330

Conversation

joyeecheung commented Oct 22, 2023 • edited Loading

nodejs-github-bot commented Oct 22, 2023

nodejs-github-bot commented Oct 22, 2023

KhafraDev commented Oct 22, 2023

mcollina left a comment

Choose a reason for hiding this comment

benjamingr commented Oct 23, 2023

joyeecheung commented Oct 24, 2023 • edited Loading

joyeecheung commented Oct 24, 2023 • edited Loading

nodejs-github-bot commented Oct 24, 2023

joyeecheung commented Oct 24, 2023

joyeecheung commented Oct 24, 2023

nodejs-github-bot commented Oct 24, 2023

aduh95 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 25, 2023

nodejs-github-bot commented Oct 25, 2023

nodejs-github-bot commented Oct 25, 2023

nodejs-github-bot commented Oct 25, 2023

joyeecheung commented Oct 22, 2023 •

edited

Loading

joyeecheung commented Oct 24, 2023 •

edited

Loading

joyeecheung commented Oct 24, 2023 •

edited

Loading