Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: support V8 experimental shared values in messaging #47706

Merged
merged 5 commits into from
May 9, 2023

Conversation

syg
Copy link
Contributor

@syg syg commented Apr 24, 2023

This adds the missing integration support for the V8 experimental shared structs feature (--harmony-struct). There is no observable difference to node if --harmony-struct is not passed. The flag is false by default.

This experimental feature is for proving out shared memory in JS. The standards side is tracked by https://github.com/tc39/proposal-structs, though the current experiment, for the sake of ease of experimentation, does not include syntax. The features in experiment is documented here.

I also intentionally did not add any tests because the features are experimental and volatile. Here is an illustrative example though:

'use strict';
const { Worker, isMainThread, parentPort } = require('worker_threads');

if (isMainThread) {
  let Box = new SharedStructType(['payload']);
  let b = new Box();
  b.payload = "hello shared memory world";

  let worker = new Worker(__filename);
  worker.on('message', (message) => {
    console.log(b.payload);
    console.log(message === b);
  });

  worker.postMessage(b);
} else {
  parentPort.on('message', (message) => {
    // Simple echo.
    parentPort.postMessage(message);
  });
}

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 24, 2023
@syg
Copy link
Contributor Author

syg commented Apr 24, 2023

cc @rbuckton, I had meant to do this a while back but totally forgot. Once this merges then TSC can start playing around with the dev trial as well.

@debadree25 debadree25 added the worker Issues and PRs related to Worker support. label Apr 25, 2023
@debadree25
Copy link
Member

cc @nodejs/workers

@nodejs-github-bot
Copy link
Collaborator

@syg
Copy link
Contributor Author

syg commented Apr 26, 2023

@joyeecheung I don't know nodejs very well. Is the CI saying this PR is causing some test failures? I find that rather surprising.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member

joyeecheung commented Apr 26, 2023

I don't know nodejs very well. Is the CI saying this PR is causing some test failures? I find that rather surprising.

The test failures were probably just unmarked flakes. https://ci.nodejs.org/job/node-test-pull-request/51472/ passed (with marked flakes).

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding a test would be valuable unless you are really certain that somebody will remember to add one once the feature's no longer experimental :)

@syg
Copy link
Contributor Author

syg commented Apr 27, 2023

I think adding a test would be valuable unless you are really certain that somebody will remember to add one once the feature's no longer experimental :)

Reasonable request, though I still prefer to not add a node test. The dev trial API surface is under enough flux that when I change things on the V8 side for the sake of experimentation I don't want to be blocked on breaking nodejs tests on the V8 infra. Whatever test we add now will be highly likely to be invalid once the feature is no longer experimental anyway, because the experimental dev trial doesn't use syntax to declare shared structs.

OTOH I'm all for adding "examples" that aren't run on the CI and block them but can be run by hand. WDYT?

Edit: I added a test.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@syg
Copy link
Contributor Author

syg commented May 5, 2023

@joyeecheung I still don't know how to read the CI reports. :) Does this look good to land?

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
@syg
Copy link
Contributor Author

syg commented May 8, 2023

Good to merge?

@joyeecheung 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. and removed 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 May 8, 2023
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member

Need to re-run the CI until it's green

@gengjiawen gengjiawen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2023
@nodejs-github-bot 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 May 9, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/47706
✔  Done loading data for nodejs/node/pull/47706
----------------------------------- PR info ------------------------------------
Title      src: support V8 experimental shared values in messaging (#47706)
Author     Shu-yu Guo  (@syg)
Branch     syg:shared-value-conveyor -> nodejs:main
Labels     c++, worker, needs-ci
Commits    5
 - src: support V8 experimental shared values in messaging
 - Add a test
 - Fix linter errors
 - Update test/parallel/test-experimental-shared-value-conveyor.js
 - Quell linter
Committers 2
 - Shu-yu Guo 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/47706
Reviewed-By: Joyee Cheung 
Reviewed-By: Anna Henningsen 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47706
Reviewed-By: Joyee Cheung 
Reviewed-By: Anna Henningsen 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 24 Apr 2023 19:05:11 GMT
   ✔  Approvals: 2
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/47706#pullrequestreview-1415757197
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/47706#pullrequestreview-1404880614
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-05-08T16:44:58Z: https://ci.nodejs.org/job/node-test-pull-request/51702/
- Querying data for job/node-test-pull-request/51702/
   ✔  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 47706
From https://github.com/nodejs/node
 * branch                  refs/pull/47706/merge -> FETCH_HEAD
✔  Fetched commits as 0b3fcfcf351f..a0946e915825
--------------------------------------------------------------------------------
Auto-merging src/node_messaging.cc
[main e400a95a4e] src: support V8 experimental shared values in messaging
 Author: Shu-yu Guo 
 Date: Mon Apr 24 11:15:07 2023 -0700
 2 files changed, 31 insertions(+), 4 deletions(-)
[main 620a4f5057] Add a test
 Author: Shu-yu Guo 
 Date: Tue May 2 13:49:57 2023 -0700
 1 file changed, 23 insertions(+)
 create mode 100644 test/parallel/test-experimental-shared-value-conveyor.js
[main 806c2bce27] Fix linter errors
 Author: Shu-yu Guo 
 Date: Tue May 2 14:14:40 2023 -0700
 1 file changed, 2 insertions(+), 2 deletions(-)
[main d4b2962cae] Update test/parallel/test-experimental-shared-value-conveyor.js
 Author: Shu-yu Guo 
 Date: Fri May 5 17:09:16 2023 -0400
 1 file changed, 3 insertions(+), 1 deletion(-)
[main 641c3af3b9] Quell linter
 Author: Shu-yu Guo 
 Date: Fri May 5 15:05:18 2023 -0700
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 5 commits in the PR. Attempting autorebase.
Rebasing (2/10)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: support V8 experimental shared values in messaging

PR-URL: #47706
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net

[detached HEAD 6a7fbf90f2] src: support V8 experimental shared values in messaging
Author: Shu-yu Guo syg@chromium.org
Date: Mon Apr 24 11:15:07 2023 -0700
2 files changed, 31 insertions(+), 4 deletions(-)
Rebasing (3/10)
Rebasing (4/10)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Add a test

PR-URL: #47706
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net

[detached HEAD 80840620cd] Add a test
Author: Shu-yu Guo syg@chromium.org
Date: Tue May 2 13:49:57 2023 -0700
1 file changed, 23 insertions(+)
create mode 100644 test/parallel/test-experimental-shared-value-conveyor.js
Rebasing (5/10)
Rebasing (6/10)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Fix linter errors

PR-URL: #47706
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net

[detached HEAD 5e3d362c49] Fix linter errors
Author: Shu-yu Guo syg@chromium.org
Date: Tue May 2 14:14:40 2023 -0700
1 file changed, 2 insertions(+), 2 deletions(-)
Rebasing (7/10)
Rebasing (8/10)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update test/parallel/test-experimental-shared-value-conveyor.js

Co-authored-by: Joyee Cheung joyeec9h3@gmail.com
PR-URL: #47706
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net

[detached HEAD f884cd13a6] Update test/parallel/test-experimental-shared-value-conveyor.js
Author: Shu-yu Guo syg@chromium.org
Date: Fri May 5 17:09:16 2023 -0400
1 file changed, 3 insertions(+), 1 deletion(-)
Rebasing (9/10)
Rebasing (10/10)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Quell linter

PR-URL: #47706
Reviewed-By: Joyee Cheung joyeec9h3@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net

[detached HEAD 2ad3799a03] Quell linter
Author: Shu-yu Guo syg@chromium.org
Date: Fri May 5 15:05:18 2023 -0700
1 file changed, 1 insertion(+), 1 deletion(-)

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/4923112701

@gengjiawen gengjiawen added commit-queue-failed An error occurred while landing this 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. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 9, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2023
@nodejs-github-bot nodejs-github-bot merged commit b2f6eed into nodejs:main May 9, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in b2f6eed

targos pushed a commit that referenced this pull request May 12, 2023
PR-URL: #47706
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants