Skip to content

src: use dedicated routine to compile function for builtin CJS loader - #52016

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
joyeecheung:cjs-compile
Mar 11, 2024
Merged

src: use dedicated routine to compile function for builtin CJS loader#52016
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
joyeecheung:cjs-compile

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Mar 8, 2024•
edited
Loading

Copy link
Copy Markdown
Member

So that we can use it to handle code caching in a central place.

Needed by #47472, split out from #51977

Drive-by: use per-isolate persistent strings for the parameters and mark GetHostDefinedOptions() static since it's only used in one compilation unit

Refs: #47472

@nodejs-github-bot

nodejs-github-bot commented Mar 8, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/single-executable

@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@nodejs-github-botnodejs-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. needs-ci PRs that need a full CI run. labels Mar 8, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
joyeecheung
joyeecheung commented Mar 8, 2024
View reviewed changes
Comment threadsrc/node_contextify.cc Outdated

@joyeecheungjoyeecheungMar 8, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Interestingly, in my prototype for #47472, enabling eager compilation doesn't speed up the loading of the various CLI tools I found in the code base - actually, it's slower than the default (lazy compilation). But since #51672 showed that eager compilation of essential internals could at least speed up our core startup, I think this really depends on the usage pattern of the code being compiled, so I left a TODO for future investigation (also I think technically users can try eager compilation themselves using --no-lazy)

@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@nodejs-github-bot

nodejs-github-bot commented Mar 8, 2024

Copy link
Copy Markdown
Collaborator

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

@joyeecheung
src: use dedicated routine to compile function for builtin CJS loader
362df16
So that we can use it to handle code caching in a central place.
Drive-by: use per-isolate persistent strings for the parameters
and mark GetHostDefinedOptions() since it's only used in one
compilation unit
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 8, 2024
@nodejs-github-bot

nodejs-github-bot commented Mar 8, 2024

Copy link
Copy Markdown
Collaborator

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

GeoffreyBooth
GeoffreyBooth approved these changes Mar 9, 2024
View reviewed changes

@GeoffreyBoothGeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn’t need any tests?

@joyeecheung

joyeecheung commented Mar 9, 2024

Copy link
Copy Markdown
MemberAuthor

It's just a refactoring, if it doesn't break any tests, I think that's enough (and if it doesn't work there are bound to be test failure..)

@nodejs-github-bot

nodejs-github-bot commented Mar 10, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Mar 10, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

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

@joyeecheungjoyeecheung 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-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 11, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 11, 2024
@nodejs-github-bot
nodejs-github-bot merged commit bec9b5f into nodejs:mainMar 11, 2024
@nodejs-github-bot

nodejs-github-bot commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

Landed in bec9b5f

@RafaelGSSRafaelGSS mentioned this pull request Apr 15, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@joyeecheung@marco-ippolito
src: use dedicated routine to compile function for builtin CJS loader
c550ef1
So that we can use it to handle code caching in a central place.
Drive-by: use per-isolate persistent strings for the parameters
and mark GetHostDefinedOptions() since it's only used in one
compilation unit
PR-URL: #52016
Refs: #47472
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@marco-ippolitomarco-ippolito mentioned this pull request May 2, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
@joyeecheung@marco-ippolito
src: use dedicated routine to compile function for builtin CJS loader
e161e62
So that we can use it to handle code caching in a central place.
Drive-by: use per-isolate persistent strings for the parameters
and mark GetHostDefinedOptions() since it's only used in one
compilation unit
PR-URL: #52016
Refs: #47472
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
codebytere added a commit to electron/electron that referenced this pull request May 8, 2024
@codebytere
src: use dedicated routine to compile function for builtin CJS loader
c343fdd
nodejs/node#52016
jkleinsc pushed a commit to electron/electron that referenced this pull request May 13, 2024
@electron-roller@codebytere@patchup
chore: bump node to v20.13.1 (main) (#42088)
653d0f0
* chore: bump node in DEPS to v20.13.0
* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL
nodejs/node#52217
* test: skip test for dynamically linked OpenSSL
nodejs/node#52542
* lib, url: add a `windows` option to path parsing
nodejs/node#52509
* src: use dedicated routine to compile function for builtin CJS loader
nodejs/node#52016
* test: mark test as flaky
nodejs/node#52671
* build,tools: add test-ubsan ci
nodejs/node#46297
* src: preload function for Environment
nodejs/node#51539
* chore: fixup patch indices
* deps: update c-ares to 1.28.1
nodejs/node#52285
* chore: handle updated filenames
- nodejs/node#51999
- nodejs/node#51927
* chore: bump node in DEPS to v20.13.1
* events: extract addAbortListener for safe internal use
nodejs/node#52081
* module: print location of unsettled top-level await in entry points
nodejs/node#51999
* fs: add stacktrace to fs/promises
nodejs/node#49849
* chore: update patches
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@codebytere
src: use dedicated routine to compile function for builtin CJS loader
da84019
nodejs/node#52016
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@codebytere
src: use dedicated routine to compile function for builtin CJS loader
65c01d3
nodejs/node#52016
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@electron-roller@codebytere
chore: bump node to v20.14.0 (31-x-y) (#42295)
9a87938
* chore: bump node in DEPS to v20.13.1
* chore: bump node in DEPS to v20.14.0
* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL
nodejs/node#52217
* test: skip test for dynamically linked OpenSSL
nodejs/node#52542
* lib, url: add a `windows` option to path parsing
nodejs/node#52509
* src: use dedicated routine to compile function for builtin CJS loader
nodejs/node#52016
* test: mark test as flaky
nodejs/node#52671
* build,tools: add test-ubsan ci
nodejs/node#46297
* src: preload function for Environment
nodejs/node#51539
* chore: fixup patch indices
* deps: update c-ares to 1.28.1
nodejs/node#52285
* chore: handle updated filenames
* events: extract addAbortListener for safe internal use
nodejs/node#52081
* module: print location of unsettled top-level await in entry points
nodejs/node#51999
* fs: add stacktrace to fs/promises
nodejs/node#49849
* chore: fixup patch indices
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2024
@electron-roller@zcbenz
@codebytere@patchup
chore: bump node to v20.14.0 (30-x-y) (#42294)
3be3381
* chore: bump node in DEPS to v20.13.1
* chore: bump node in DEPS to v20.14.0
* chore: update build_add_gn_build_files.patch
* chore: update patches
* chore: update patches
* build: encode non-ASCII Latin1 characters as one byte in JS2C
nodejs/node#51605
* crypto: use EVP_MD_fetch and cache EVP_MD for hashes
nodejs/node#51034
* chore: update filenames.json
* chore: update patches
* src: support configurable snapshot
nodejs/node#50453
* test: remove test-domain-error-types flaky designation
nodejs/node#51717
* src: avoid draining platform tasks at FreeEnvironment
nodejs/node#51290
* chore: fix accidentally deleted v8 dep
* lib: define FormData and fetch etc. in the built-in snapshot
nodejs/node#51598
* chore: remove stray log
* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL
nodejs/node#52217
* test: skip test for dynamically linked OpenSSL
nodejs/node#52542
* lib, url: add a `windows` option to path parsing
nodejs/node#52509
* src: use dedicated routine to compile function for builtin CJS loader
nodejs/node#52016
* test: mark test as flaky
nodejs/node#52671
* build,tools: add test-ubsan ci
nodejs/node#46297
* src: preload function for Environment
nodejs/node#51539
* deps: update c-ares to 1.28.1
nodejs/node#52285
* chore: fixup
* events: extract addAbortListener for safe internal use
nodejs/node#52081
* module: print location of unsettled top-level await in entry points
nodejs/node#51999
* fs: add stacktrace to fs/promises
nodejs/node#49849
* chore: fixup indices
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Cheng <zcbenz@gmail.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GeoffreyBoothGeoffreyBoothGeoffreyBooth approved these changes
@aduh95aduh95aduh95 approved these changes
+1 more reviewer
@aymen94aymen94aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@joyeecheung@nodejs-github-bot@GeoffreyBooth@aymen94@aduh95