src,lib: make JSTransferables based on private symbols #47956

legendecas · 2023-05-11T06:32:01Z

src: distinguish HTML transferable and cloneable
The HTML structured serialize algorithm treats transferable and
serializable as two different bits. A web platform interface can be
both transferable and serializable.

Splits BaseObject::TransferMode to be able to compose the two bits
and distinguishes the transferable and cloneable.

deps: V8: cherry-pick cf13b9b46572
Original commit message:

[objects] Determine host objects with serializer delegate

Enable custom host objects filter with ValueSerializer::Delegate
when ValueSerializer::Delegate::HasCustomHostObject returns true.

This allows the embedder to serialize JavaScript implemented host
objects.

Bug: v8:11927
Change-Id: I70e7aa70b10dc1053c113d521479cbe746febc7e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4385565
Commit-Queue: Chengzhong Wu (legendecas) <[email protected]>
Reviewed-by: Camillo Bruni <[email protected]>
Cr-Commit-Position: refs/heads/main@{[#87055](https://github.com/legendecas/node/issues/87055)}

Refs: v8/v8@cf13b9b

src,lib: make JSTransferables based on private symbols
Serializes and transfers platform objects implemented as a JS class
based on private symbols instead of V8 object internal slots. This
avoids the need to alter the prototype chains and mixins to make the
JS class to be transferable.

The performance improvements on the platform object creation (e.g. web streams) can be observable as:

                                                      confidence improvement accuracy (*)   (**)  (***)
webstreams/creation.js kind='ReadableStream' n=50000         ***    137.35 %       ±1.72% ±2.31% ±3.02%
webstreams/creation.js kind='TransformStream' n=50000        ***     93.73 %       ±0.97% ±1.30% ±1.69%
webstreams/creation.js kind='WritableStream' n=50000         ***    128.99 %       ±1.69% ±2.27% ±2.99%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 3 comparisons, you can thus expect the following amount of false-positive results:
  0.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.03 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

Refs: #47497 (comment)

nodejs-github-bot · 2023-05-11T06:32:06Z

Review requested:

@nodejs/crypto
@nodejs/v8-update

legendecas · 2023-05-11T06:39:18Z

Since this is a new feature, this PR can wait until the V8 change gets merged with the v8 currency updates, instead of landing the cherry-picked V8 commit.

lib/internal/webstreams/transfer.js

lib/internal/worker/js_transferable.js

src/node_messaging.cc

anonrig · 2023-05-11T14:35:30Z

src/node_messaging.cc

+  bool has_transfer_mode;
+  if (!object->HasPrivate(context, env->transfer_mode_private_symbol())
+           .To(&has_transfer_mode)) {
+    return Nothing<bool>();


Do we need to handle Nothing as well? Or can we just assume it is not transferable or transferable? This would reduce the complexity of the implementation below

I dug a bit into the private symbol implementation. I believe we can safely assume that accessing the private symbol data properties should not throw and we can remove the maybe handle checks here.

Thanks for the idea.

src/crypto/crypto_keys.h

jasnell · 2023-05-29T19:29:37Z

oooo... yes! thank you for doing this.

The HTML structured serialize algorithm treats transferable and serializable as two different bits. A web platform interface can be both transferable and serializable. Splits BaseObject::TransferMode to be able to compose the two bits and distinguishes the transferable and cloneable.

Original commit message: [objects] Determine host objects with serializer delegate Enable custom host objects filter with ValueSerializer::Delegate when ValueSerializer::Delegate::HasCustomHostObject returns true. This allows the embedder to serialize JavaScript implemented host objects. Bug: v8:11927 Change-Id: I70e7aa70b10dc1053c113d521479cbe746febc7e Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4385565 Commit-Queue: Chengzhong Wu (legendecas) <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/main@{#87055} Refs: v8/v8@cf13b9b

Serializes and transfers platform objects implemented as a JS class based on private symbols instead of V8 object internal slots. This avoids the need to alter the prototype chains and mixins to make the JS class to be transferable.

legendecas · 2023-07-05T03:56:14Z

@anonrig @jasnell thanks for the review! sorry for taking a long time to pick this up. I've updated the PR to address the review comments. Would you mind taking a look again? Thank you!

mcollina

rubberstamp LGTM

This is amazing, thanks!

nodejs-github-bot · 2023-07-05T11:12:53Z

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

nodejs-github-bot · 2023-07-06T03:42:26Z

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

nodejs-github-bot · 2023-07-07T06:42:27Z

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

nodejs-github-bot · 2023-07-07T09:30:00Z

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

nodejs-github-bot · 2023-07-07T16:51:22Z

Commit Queue failed

- Loading data for nodejs/node/pull/47956
✔  Done loading data for nodejs/node/pull/47956
----------------------------------- PR info ------------------------------------
Title      src,lib: make JSTransferables based on private symbols (#47956)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     legendecas:webidl/jstransferable -> nodejs:main
Labels     lib / src, needs-ci
Commits    5
 - src: distinguish HTML transferable and cloneable
 - deps: V8: cherry-pick cf13b9b46572
 - src,lib: make JSTransferables based on private symbols
 - fixup! src,lib: make JSTransferables based on private symbols
 - fixup! deps: V8: cherry-pick cf13b9b46572
Committers 1
 - Chengzhong Wu 
PR-URL: https://github.com/nodejs/node/pull/47956
Refs: https://github.com/v8/v8/commit/cf13b9b46572a9824d2d632abdd48c56161ace02
Reviewed-By: Matteo Collina 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Daeyeon Jeong 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47956
Refs: https://github.com/v8/v8/commit/cf13b9b46572a9824d2d632abdd48c56161ace02
Reviewed-By: Matteo Collina 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Daeyeon Jeong 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 11 May 2023 06:32:01 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/47956#pullrequestreview-1514024315
   ✔  - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/47956#pullrequestreview-1514347888
   ✔  - Daeyeon Jeong (@daeyeon): https://github.com/nodejs/node/pull/47956#pullrequestreview-1519291904
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-07-07T09:30:00Z: https://ci.nodejs.org/job/node-test-pull-request/52645/
- Querying data for job/node-test-pull-request/52645/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 47956
From https://github.com/nodejs/node
 * branch                  refs/pull/47956/merge -> FETCH_HEAD
✔  Fetched commits as 3205b1936a5c..9558dfbc1604
--------------------------------------------------------------------------------
Auto-merging src/node_file.cc
[main 1db5272fe6] src: distinguish HTML transferable and cloneable
 Author: legendecas 
 Date: Mon Apr 10 02:24:06 2023 +0800
 10 files changed, 104 insertions(+), 94 deletions(-)
[main 7a5bc6c681] deps: V8: cherry-pick cf13b9b46572
 Author: Chengzhong Wu 
 Date: Thu Apr 13 15:22:38 2023 +0800
 5 files changed, 110 insertions(+), 3 deletions(-)
[main 81dabd1439] src,lib: make JSTransferables based on private symbols
 Author: Chengzhong Wu 
 Date: Sat May 6 17:00:50 2023 +0800
 23 files changed, 342 insertions(+), 232 deletions(-)
 rename test/parallel/{test-messaging-maketransferable.js => test-messaging-marktransfermode.js} (82%)
[main d5adc625d1] fixup! src,lib: make JSTransferables based on private symbols
 Author: Chengzhong Wu 
 Date: Wed Jul 5 17:09:27 2023 +0800
 1 file changed, 4 insertions(+), 4 deletions(-)
[main 847a70f995] fixup! deps: V8: cherry-pick cf13b9b46572
 Author: Chengzhong Wu 
 Date: Thu Jul 6 17:11:22 2023 +0800
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 5 commits in the PR. Attempting autorebase.
Rebasing (2/8)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

src: distinguish HTML transferable and cloneable
The HTML structured serialize algorithm treats transferable and

serializable as two different bits. A web platform interface can be

both transferable and serializable.
Splits BaseObject::TransferMode to be able to compose the two bits

and distinguishes the transferable and cloneable.
PR-URL: #47956

Refs: v8/v8@cf13b9b

Reviewed-By: Matteo Collina [email protected]

Reviewed-By: Yagiz Nizipli [email protected]

Reviewed-By: Daeyeon Jeong [email protected]
[detached HEAD 178290c101] src: distinguish HTML transferable and cloneable

Author: legendecas [email protected]

Date: Mon Apr 10 02:24:06 2023 +0800

10 files changed, 104 insertions(+), 94 deletions(-)

Rebasing (3/8)

Rebasing (4/8)

Rebasing (5/8)
Executing: git node land --amend --yes

⚠  Found Refs: v8/v8@cf13b9b, skipping..

--------------------------------- New Message ----------------------------------

deps: V8: cherry-pick cf13b9b46572
Original commit message:
[objects] Determine host objects with serializer delegate

Enable custom host objects filter with ValueSerializer::Delegate
when ValueSerializer::Delegate::HasCustomHostObject returns true.

This allows the embedder to serialize JavaScript implemented host
objects.

Bug: v8:11927
Change-Id: I70e7aa70b10dc1053c113d521479cbe746febc7e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4385565
Commit-Queue: Chengzhong Wu (legendecas) <[email protected]>
Reviewed-by: Camillo Bruni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#87055}

Refs: v8/v8@cf13b9b

PR-URL: #47956

Reviewed-By: Matteo Collina [email protected]

Reviewed-By: Yagiz Nizipli [email protected]

Reviewed-By: Daeyeon Jeong [email protected]
[detached HEAD a9dc0d8373] deps: V8: cherry-pick cf13b9b46572

Author: Chengzhong Wu [email protected]

Date: Thu Apr 13 15:22:38 2023 +0800

6 files changed, 111 insertions(+), 4 deletions(-)

Rebasing (6/8)

Rebasing (7/8)

Rebasing (8/8)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

src,lib: make JSTransferables based on private symbols
Serializes and transfers platform objects implemented as a JS class

based on private symbols instead of V8 object internal slots. This

avoids the need to alter the prototype chains and mixins to make the

JS class to be transferable.
PR-URL: #47956

Refs: v8/v8@cf13b9b

Reviewed-By: Matteo Collina [email protected]

Reviewed-By: Yagiz Nizipli [email protected]

Reviewed-By: Daeyeon Jeong [email protected]
[detached HEAD c383a46279] src,lib: make JSTransferables based on private symbols

Author: Chengzhong Wu [email protected]

Date: Sat May 6 17:00:50 2023 +0800

23 files changed, 342 insertions(+), 232 deletions(-)

rename test/parallel/{test-messaging-maketransferable.js => test-messaging-marktransfermode.js} (82%)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/5488694803

nodejs-github-bot · 2023-07-07T17:00:02Z

Landed in 38dee8a

legendecas · 2023-07-12T03:09:56Z

@anonrig I think it might be better to land PRs that contain V8 cherry-picks as separate commits with commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. instead. However, I failed to find a written doc for this pattern.

anonrig · 2023-07-12T04:39:49Z

You’re absolutely right. This was an error. Sorry for the trouble caused!

juanarbol · 2023-07-13T16:29:18Z

This depends on #47604 (it needs the transfer_unsupported_type_str()) method

The HTML structured serialize algorithm treats transferable and serializable as two different bits. A web platform interface can be both transferable and serializable. Splits BaseObject::TransferMode to be able to compose the two bits and distinguishes the transferable and cloneable. PR-URL: nodejs#47956 Refs: v8/v8@cf13b9b Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]>

legendecas · 2023-12-19T07:50:26Z

I think this might be valuable to backport to LTS lines. This PR is not marked as semver-major PRs that contain breaking changes and should be released in the next major version. . I can create backport PRs.

@juanarbol @ruyadorno what do you think, may I remove the dont-land-on-x labels?

nodejs-github-bot added 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 May 11, 2023

legendecas mentioned this pull request May 11, 2023

lib: mark URL/URLSearchParams as uncloneable and untransferable #47497

Merged

anonrig reviewed May 11, 2023

View reviewed changes

jasnell reviewed May 29, 2023

View reviewed changes

src/crypto/crypto_keys.h Outdated Show resolved Hide resolved

legendecas and others added 3 commits July 5, 2023 11:04

legendecas force-pushed the webidl/jstransferable branch from ca73f52 to b61b4c9 Compare July 5, 2023 03:55

legendecas mentioned this pull request Jul 5, 2023

fetch() performance parity nodejs/undici#1203

Closed

mcollina approved these changes Jul 5, 2023

View reviewed changes

fixup! src,lib: make JSTransferables based on private symbols

161ce38

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2023

anonrig approved these changes Jul 5, 2023

View reviewed changes

ignoramous mentioned this pull request Jul 5, 2023

Performance of WHATWG ReadableStream.read() nodejs/performance#82

Open

github-actions bot mentioned this pull request Jul 6, 2023

CI Reliability 2023-07-06 nodejs/reliability#609

Open

17 tasks

fixup! deps: V8: cherry-pick cf13b9b46572

9558dfb

github-actions bot mentioned this pull request Jul 7, 2023

CI Reliability 2023-07-07 nodejs/reliability#610

Open

20 tasks

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2023

daeyeon approved these changes Jul 7, 2023

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 7, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 7, 2023

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

nodejs-github-bot merged commit 38dee8a into nodejs:main Jul 7, 2023

This was referenced Jul 8, 2023

CI Reliability 2023-07-08 nodejs/reliability#611

Open

CI Reliability 2023-07-09 nodejs/reliability#612

Open

CI Reliability 2023-07-10 nodejs/reliability#613

Open

legendecas deleted the webidl/jstransferable branch July 12, 2023 02:56

legendecas mentioned this pull request Jul 13, 2023

doc: expand on squashing and rebasing to land a PR #48751

Merged

juanarbol added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Jul 13, 2023

ruyadorno added dont-land-on-v16.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Sep 11, 2023

RafaelGSS mentioned this pull request Oct 9, 2023

2023-10-17, Version 21.0.0 (Current) #49870

Merged

targos mentioned this pull request May 1, 2024

streams: fixes for webstreams #51168

Merged

richardlau mentioned this pull request Jun 7, 2024

Unable to deserialize cloned data #49844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,lib: make JSTransferables based on private symbols #47956

src,lib: make JSTransferables based on private symbols #47956

legendecas commented May 11, 2023

nodejs-github-bot commented May 11, 2023

legendecas commented May 11, 2023

anonrig May 11, 2023

legendecas Jul 5, 2023

jasnell commented May 29, 2023

legendecas commented Jul 5, 2023

mcollina left a comment

nodejs-github-bot commented Jul 5, 2023

nodejs-github-bot commented Jul 6, 2023

nodejs-github-bot commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

PR-URL: #47956
Refs: v8/v8@`cf13b9b`
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Daeyeon Jeong [email protected]

Refs: v8/v8@`cf13b9b`
PR-URL: #47956
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Daeyeon Jeong [email protected]

PR-URL: #47956
Refs: v8/v8@`cf13b9b`
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Daeyeon Jeong [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Jul 7, 2023

legendecas commented Jul 12, 2023 •

edited

Loading

anonrig commented Jul 12, 2023 •

edited

Loading

juanarbol commented Jul 13, 2023 •

edited

Loading

legendecas commented Dec 19, 2023 •

edited

Loading

src,lib: make JSTransferables based on private symbols #47956

src,lib: make JSTransferables based on private symbols #47956

Conversation

legendecas commented May 11, 2023

nodejs-github-bot commented May 11, 2023

legendecas commented May 11, 2023

anonrig May 11, 2023

Choose a reason for hiding this comment

legendecas Jul 5, 2023

Choose a reason for hiding this comment

jasnell commented May 29, 2023

legendecas commented Jul 5, 2023

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 5, 2023

nodejs-github-bot commented Jul 6, 2023

nodejs-github-bot commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

PR-URL: #47956 Refs: v8/v8@cf13b9b Reviewed-By: Matteo Collina [email protected] Reviewed-By: Yagiz Nizipli [email protected] Reviewed-By: Daeyeon Jeong [email protected]

Refs: v8/v8@cf13b9b PR-URL: #47956 Reviewed-By: Matteo Collina [email protected] Reviewed-By: Yagiz Nizipli [email protected] Reviewed-By: Daeyeon Jeong [email protected]

PR-URL: #47956 Refs: v8/v8@cf13b9b Reviewed-By: Matteo Collina [email protected] Reviewed-By: Yagiz Nizipli [email protected] Reviewed-By: Daeyeon Jeong [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Jul 7, 2023

legendecas commented Jul 12, 2023 • edited Loading

anonrig commented Jul 12, 2023 • edited Loading

juanarbol commented Jul 13, 2023 • edited Loading

legendecas commented Dec 19, 2023 • edited Loading

PR-URL: #47956
Refs: v8/v8@`cf13b9b`
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Daeyeon Jeong [email protected]

Refs: v8/v8@`cf13b9b`
PR-URL: #47956
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Daeyeon Jeong [email protected]

PR-URL: #47956
Refs: v8/v8@`cf13b9b`
Reviewed-By: Matteo Collina [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: Daeyeon Jeong [email protected]

legendecas commented Jul 12, 2023 •

edited

Loading

anonrig commented Jul 12, 2023 •

edited

Loading

juanarbol commented Jul 13, 2023 •

edited

Loading

legendecas commented Dec 19, 2023 •

edited

Loading