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

tools: implement mkcodecache as an executable #27135

Closed
wants to merge 0 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 8, 2019

This patch splits NativeModuleLoader into two parts - a singleton
that only relies on v8 and node::Mutex and a proxy class for
the singleton (NativeModuleEnv) that provides limited access to
the singleton as well as C++ bindings for the Node.js binary.
A mkcodecache executable is then built on top of the singleton.
This makes it possible to build a Node.js binary with embedded
code cache without building itself using the code cache stub -
the cache is now initialized by NativeModuleEnv instead which
can be refactored out of the mkcodecache dependencies.

Refs: #21563

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@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 8, 2019
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

The idea is that we can decompose mkcodecache out of the node binary so that it's easier to complete the build integration in GYP.
In the case of cross-compilation, the next step is now only to make sure that mkcodecache can run on the simulator, instead of making sure the entire Node.js binary can run on the simulator which would be more difficult since it contains much more platform-dependent code. (See #22934 (comment))

@refack
Copy link
Contributor

refack commented Apr 8, 2019

Test balloon based on #27108
f96ef8276e6a5cfaa000ecf4a5ea69d4aa8da5f0...bb0ffb2fcbc16f7b62f4b907263eaf591f1c90e8
https://ci.nodejs.org/job/node-test-commit/27604/

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 9, 2019

@nodejs/build-files @nodejs/process CI is green. can I have some reviews please?

@joyeecheung joyeecheung added the review wanted PRs that need reviews. label Apr 9, 2019
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

I'm not sure the split is worth the complexity...

src/node_native_module_env.h Outdated Show resolved Hide resolved
src/node_native_module_env.cc Outdated Show resolved Hide resolved
src/node_native_module_env.cc Outdated Show resolved Hide resolved
src/node_native_module_env.cc Outdated Show resolved Hide resolved
src/node_native_module.cc Outdated Show resolved Hide resolved
src/node_native_module_env.cc Outdated Show resolved Hide resolved
src/node_native_module.h Outdated Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 9, 2019

I'm not sure the split is worth the complexity...

I believe it's worth it, even not for the GYP changes - this is what I would've done in hindsight, see the previous comments in NativeModuleLoader:

// This class should not depend on a particular isolate, context, or
// environment. Rather it should take them as arguments when necessary.
// The instances of this class are per-process.

It is now no longer aware of Environment but delegate that to NativeModuleEnv, so the actual dependency of NativeModuleLoader shrinks to util-inl.h and node_mutex.h, and env.h (which basically force you to link to most files) is no longer relevant.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Resolved some comments.

src/node_native_module.cc Outdated Show resolved Hide resolved
src/node_native_module_env.cc Outdated Show resolved Hide resolved
src/node_native_module_env.h Outdated Show resolved Hide resolved
src/node_native_module_env.cc Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Apr 9, 2019

I'm not sure the split is worth the complexity...

I believe it's worth it, even not for the GYP changes - this is what I would've done in hindsight, see the previous comments in NativeModuleLoader:

I have an idea I hope will simplify this. I'll try to right a suggestion.

Anyway, my comments are non-blocking.

@joyeecheung
Copy link
Member Author

I've split the refactoring into the first commit, and submitted it as #27160

@refack
Copy link
Contributor

refack commented Apr 9, 2019

I took the rest into #27161
Minimal change-set needed is 4a2a646

@joyeecheung
Copy link
Member Author

Rebased after #27160 landed, and made the progress output optional depending on NODE_DEBUG=mkcodecache so it gets less noisy when being built by default.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

CI is green. Can I have some reviews please? @nodejs/process @nodejs/build-files

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 16, 2019

Pinging for reviews..from the GitHub suggestions, @refack and maybe @danbev

I guess this may land as part of #27161 but then it'll be me being the sole reviewer of my own patch which is kind of odd.

@refack
Copy link
Contributor

refack commented Apr 16, 2019

I guess this may land as part of #27161 but then it'll be me being the sole reviewer of my own patch which is kind of odd.

Well I guess we cross approved each other's bits.

@refack
Copy link
Contributor

refack commented Apr 16, 2019

Superseded by #27161

@refack refack closed this Apr 16, 2019
@refack refack removed the review wanted PRs that need reviews. label Apr 16, 2019
@refack refack added the landed label Apr 16, 2019
@refack
Copy link
Contributor

refack commented Apr 16, 2019

landed in 4fd7193 (as part of #27161)

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants