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

src: port bootstrap/cache.js to C++ #27046

Closed
wants to merge 2 commits into from
Closed

src: port bootstrap/cache.js to C++ #27046

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 2, 2019 •
edited
Loading

This allows us to query the categories of modules in C++
so we can implement the code cache generator in C++ that
does not depend on a Node.js binary.

Ref: #21563

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3137
@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 Apr 2, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22104/
@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 2, 2019 •
edited
Loading

cc @nodejs/process @nodejs/build-files

PTAL, I am looking into rewriting tools/generate_code_cache.js into a binary target (mkcodecahe) that only depends on the Node.js library instead of the executable - it's probably less error-prone to configure GYP to complete the two-pass build process that way. This is one of the items listed out in #21563
bnoordhuis
bnoordhuis reviewed Apr 2, 2019
View reviewed changes
src/node_native_module.cc Outdated Show resolved Hide resolved
src/node_native_module.cc Outdated Show resolved Hide resolved
src/node_native_module.cc Outdated Show resolved Hide resolved
src/node_native_module.cc
OneByteString(isolate, "canBeRequired"),
ToJsSet(context, can_be_required))
.FromJust();
info.GetReturnValue().Set(result);
Copy link
Member

Choose a reason for hiding this comment

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

Consider caching result as an Environment field or compute it eagerly inNativeModuleLoader::Initialize().

It feels kind of wasteful to recompute it every time and it breaks object identity (although that's more of an academic issue since it's only used internally.)
Copy link
Member Author

@joyeecheung joyeecheung Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's actually kind of wasteful to compute this eagerly since this is only used in the code cache generator and the tests/benchmarks. Normal use cases do not need this at all, and the code that do use this only compute it once per process.
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22109/
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22110/
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22114/
@Trott
Copy link
Member

Trott commented Apr 2, 2019

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

Linux will fail again because it got the bad ubuntu18-04 host again. I've taken it offline and will restart CI again. Windows already failed again. 😞
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22115/
@joyeecheung joyeecheung mentioned this pull request Apr 2, 2019
Closed
addaleax
addaleax approved these changes Apr 2, 2019
View reviewed changes
src/node_native_module.cc Outdated Show resolved Hide resolved
@refack refack mentioned this pull request Apr 2, 2019
Closed
benjamingr
benjamingr approved these changes Apr 3, 2019
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

lgtm overall though I haven't worked on this code :]
@joyeecheung
src: port bootstrap/cache.js to C++
9da6a33 
This allows us to query the categories of modules in C++
so we can implement the code cache generator in C++ that
does not depend on a Node.js binary.
@joyeecheung
fixup! src: port bootstrap/cache.js to C++
a7f8857
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22182/
@joyeecheung
Copy link
Member Author

Landed in e1d55a0
@joyeecheung joyeecheung closed this Apr 4, 2019
joyeecheung added a commit that referenced this pull request Apr 4, 2019
@joyeecheung
src: port bootstrap/cache.js to C++
e1d55a0 
This allows us to query the categories of modules in C++
so we can implement the code cache generator in C++ that
does not depend on a Node.js binary.

PR-URL: #27046
Refs: #21563
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 4, 2019
@joyeecheung @BethGriggs
src: port bootstrap/cache.js to C++
c1cbc1e 
This allows us to query the categories of modules in C++
so we can implement the code cache generator in C++ that
does not depend on a Node.js binary.

PR-URL: #27046
Refs: #21563
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Apr 23, 2019
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
6 participants
@joyeecheung @nodejs-github-bot @Trott @bnoordhuis @addaleax @benjamingr