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.

Sign up for GitHub

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

Jump to bottom

worker: set up child Isolate inside Worker thread #26011

Closed
wants to merge 7 commits into from
Closed

worker: set up child Isolate inside Worker thread #26011

wants to merge 7 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 8, 2019

worker: pre-allocate thread id

Allocate a thread id before actually creating the Environment instance.

worker: set up child Isolate inside Worker thread

Refs: #24016

test: add Worker + --prof regression test

Fixes: #24016

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2551/pipeline

@nodejs-github-bot 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. labels Feb 8, 2019
@addaleax addaleax mentioned this pull request Feb 8, 2019
Closed
@addaleax
Copy link
Member Author

addaleax commented Feb 8, 2019

@nodejs/workers Sorry the middle commit is such a big change, but if one of you could give this a look, that would be awesome.

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

@addaleax addaleax added worker Issues and PRs related to Worker support. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 8, 2019
@benjamingr
Copy link
Member

(also: the middle commit is the test 😅 )

@addaleax
Copy link
Member Author

addaleax commented Feb 9, 2019

@benjamingr That’s just github showing the commit out-of-order because it orders by author date rather than actual commit order :)

@addaleax
Copy link
Member Author

addaleax commented Feb 9, 2019

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

bnoordhuis
bnoordhuis approved these changes Feb 9, 2019
View reviewed changes
src/env.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
@addaleax
Copy link
Member Author

addaleax commented Feb 9, 2019

@bnoordhuis Thanks, done!

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 9, 2019
bnoordhuis
bnoordhuis approved these changes Feb 9, 2019
View reviewed changes
src/node_worker.cc Outdated Show resolved Hide resolved
@addaleax
Copy link
Member Author

addaleax commented Feb 10, 2019 •
edited
Loading

Regular CI: https://ci.nodejs.org/job/node-test-pull-request/20706/
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/20709/
Again: https://ci.nodejs.org/job/node-test-pull-request/20714/
ARM re-build (had infra failures): https://ci.nodejs.org/job/node-test-commit-arm-fanned/6374/
Again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/6376/
Stress test CI for some platforms, including AIX: https://ci.nodejs.org/job/node-stress-single-test/2155/

@addaleax
Copy link
Member Author

addaleax commented Feb 10, 2019 •
edited
Loading

5th attempt at getting a green ARM fanned CI: https://ci.nodejs.org/job/node-test-commit-arm-fanned/6377/
Resume CI: https://ci.nodejs.org/job/node-test-commit-arm-fanned/6381/

This is frustrating, especially since this is obviously infrastructure-related and the only difference to the last green CI is a comment. :/

@addaleax
Copy link
Member Author

@nodejs/build-infra Can somebody take a look at https://ci.nodejs.org/job/node-test-binary-arm/6092/RUN_SUBSET=2,label=pi1-docker/console?

@addaleax
Copy link
Member Author

ARM fanned again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/6396/

@addaleax
Copy link
Member Author

Landed in e11388b...902c71a

@addaleax addaleax closed this Feb 11, 2019
@addaleax addaleax deleted the worker-on-thread branch February 11, 2019 19:02
pull bot pushed a commit to SimenB/node that referenced this pull request Feb 11, 2019
@addaleax
worker: pre-allocate thread id
58ba8bf 
Allocate a thread id before actually creating the Environment instance.

PR-URL: nodejs#26011
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
pull bot pushed a commit to SimenB/node that referenced this pull request Feb 11, 2019
@addaleax
worker: set up child Isolate inside Worker thread
5bc6e49 
Refs: nodejs#24016

PR-URL: nodejs#26011
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
pull bot pushed a commit to SimenB/node that referenced this pull request Feb 11, 2019
@addaleax
test: add Worker + --prof regression test
902c71a 
Fixes: nodejs#24016

PR-URL: nodejs#26011
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax added a commit that referenced this pull request Feb 13, 2019
@addaleax
worker: pre-allocate thread id
1764aae 
Allocate a thread id before actually creating the Environment instance.

PR-URL: #26011
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax added a commit that referenced this pull request Feb 13, 2019
@addaleax
worker: set up child Isolate inside Worker thread
6c1e928 
Refs: #24016

PR-URL: #26011
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax added a commit that referenced this pull request Feb 13, 2019
@addaleax
test: add Worker + --prof regression test
896962f 
Fixes: #24016

PR-URL: #26011
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@benjamingr benjamingr benjamingr approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @nodejs-github-bot @benjamingr @bnoordhuis @joyeecheung @gireeshpunathil