GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: use dedicated routine to compile function for builtin CJS loader by joyeecheung · Pull Request #52016 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Mar 8, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, benjamingr, and tonivj5 reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@nodejs-github-bot

nodejs-github-bot commented Mar 8, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 8, 2024
@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 involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Mar 8, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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 thread src/node_contextify.cc Outdated

@joyeecheung joyeecheung Mar 8, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@joyeecheung
joyeecheung force-pushed the cjs-compile branch from d0296c0 to 02b7b4d Compare March 8, 2024 18:34
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 8, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@joyeecheung
joyeecheung force-pushed the cjs-compile branch from 02b7b4d to 362df16 Compare March 8, 2024 19:54
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 8, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 approved these changes Mar 8, 2024
View reviewed changes
GeoffreyBooth
GeoffreyBooth approved these changes Mar 9, 2024
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@joyeecheung

joyeecheung commented Mar 9, 2024

Copy link
Copy Markdown
Member Author

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..)
GeoffreyBooth reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aymen94
aymen94 approved these changes Mar 11, 2024
View reviewed changes
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Mar 11, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Mar 11, 2024
@nodejs-github-bot
nodejs-github-bot merged commit bec9b5f into nodejs:main Mar 11, 2024
@nodejs-github-bot

nodejs-github-bot commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

Landed in bec9b5f
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS mentioned this pull request Apr 15, 2024
2024-04-24, Version 22.0.0 (Current) #52505
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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request May 2, 2024
v20.13.0 proposal #52793
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 <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@aduh95 aduh95 aduh95 approved these changes
+1 more reviewer
@aymen94 aymen94 aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@joyeecheung @nodejs-github-bot @GeoffreyBooth @aymen94 @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.