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

lib,src: iterate module requests of a module wrap in JS #52058

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 12, 2024

Avoid repetitively calling into JS callback from C++ in
ModuleWrap::Link. This removes the convoluted callback style of the
internal ModuleWrap link step.

Refs: #51977 (comment)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/vm

@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. needs-ci PRs that need a full CI run. labels Mar 12, 2024
@legendecas legendecas marked this pull request as ready for review March 12, 2024 18:15
@legendecas legendecas force-pushed the module/link branch 2 times, most recently from 26b9280 to b201582 Compare March 12, 2024 18:18
@legendecas legendecas force-pushed the module/link branch 2 times, most recently from c372b99 to 6daec91 Compare March 13, 2024 07:14
@legendecas legendecas force-pushed the module/link branch 2 times, most recently from a79c903 to 572d6b0 Compare March 19, 2024 03:02
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2024
@nodejs-github-bot
Copy link
Collaborator

src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
src/module_wrap.cc Outdated Show resolved Hide resolved
@legendecas
Copy link
Member Author

Rebased on top of the tip of the main branch.

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

General idea LGTM though this needs a rebase

assert.fail('link callback should not be called');
});
[kLink]() {
/** nothing to do for synthetic modules */
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't need to be solved in this PR but it seems weird that we provide the link method for SyntheticModule at all....

Avoid repetitively calling into JS callback from C++ in
`ModuleWrap::Link`. This removes the convoluted callback style of the
internal `ModuleWrap` link step.
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 16, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member Author

@joyeecheung thanks, rebased and CI is green.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion

ObjectSetPrototypeOf(dependencyJobs, null);

// Specifiers should be aligned with the moduleRequests array in order.
const specifiers = Array(moduleRequests.length);
Copy link
Member

Choose a reason for hiding this comment

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

This leads to holey arrays, it might still be better to just use ArrayPrototyprPush since this is in a hot path (I think ArrayPrototypePush can be optimized properly the last time I checked).

Copy link
Member Author

@legendecas legendecas Apr 24, 2024

Choose a reason for hiding this comment

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

Do you have a figure for it? As far as I can tell with the latest V8 version, using a microbenchmark to allocate a 20-length array with the two patterns, the result is outstanding that pre-allocating could be ~40% faster: https://jsbench.me/9tlvebajbe/1.

Copy link
Member

Choose a reason for hiding this comment

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

The problem with holey arrays come more from the access instead of the creation, since V8 needs to defend against things like this:

Array.prototype[1] = 1;
let a = Array(2);
a[1]; // 1

Copy link
Member

Choose a reason for hiding this comment

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

(Also, it's not blocking, I am fine landing with holey arrays, since it's probably still not that much work compared to all the other things the resolution is doing..)

Copy link
Member Author

@legendecas legendecas Apr 25, 2024

Choose a reason for hiding this comment

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

Thanks for bringing this up. I think in this particular case the array is not accessed repetitively so I'd find pre-allocation would be a good solution here.

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2024
@nodejs-github-bot nodejs-github-bot merged commit d5c7ffd into nodejs:main Apr 25, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d5c7ffd

@legendecas legendecas deleted the module/link branch April 25, 2024 15:49
aduh95 pushed a commit that referenced this pull request Apr 29, 2024
Avoid repetitively calling into JS callback from C++ in
`ModuleWrap::Link`. This removes the convoluted callback style of the
internal `ModuleWrap` link step.

PR-URL: #52058
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jun 18, 2024
Avoid repetitively calling into JS callback from C++ in
`ModuleWrap::Link`. This removes the convoluted callback style of the
internal `ModuleWrap` link step.

PR-URL: nodejs#52058
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Avoid repetitively calling into JS callback from C++ in
`ModuleWrap::Link`. This removes the convoluted callback style of the
internal `ModuleWrap` link step.

PR-URL: nodejs#52058
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@marco-ippolito marco-ippolito added the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants