Skip to content

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Apr 2, 2019

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

@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

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 2, 2019

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

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

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@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

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 :]

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.
@nodejs-github-bot
Copy link
Collaborator

@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
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 4, 2019
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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.

6 participants