Skip to content

src: implement IsInsideNodeModules() in C++#55286

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
joyeecheung:node-modules-check
Oct 8, 2024
Merged

src: implement IsInsideNodeModules() in C++#55286
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
joyeecheung:node-modules-check

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Oct 5, 2024

Copy link
Copy Markdown
Member

This previously compiles a script and run it in a new context to avoid global pollution, which is more complex than necessary and can be too slow for it to be reused in other cases. The new implementation just checks the frames in C++ which is safe from global pollution, faster and simpler.

The previous implementation also had a bug when the call site is in a ESM, because ESM have URLs as their script names, which don't start with '/' or '\' and will be skipped. The new implementation removes the skipping to fix it for ESM.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Oct 5, 2024
@joyeecheung

Copy link
Copy Markdown
Member Author

The performance doesn't really matter all that much for new Buffer since it's deprecated, so I didn't add a benchmark. But since I plan to reuse IsInsideNodeModules() for require(esm) warning check when it comes from the parent-less require() built for imported CJS in #55217, I did some local benchmarking with this script, and it is faster than the original version:

const array = [];

console.time('new Buffer');
for (let i = 0; i < 10000; ++i) {
  array.push(new Buffer(10));
}
console.timeEnd('new Buffer');
❯ node node_modules/new-buffer.js
new Buffer: 52.04ms
❯ ../node/node node_modules/new-buffer.js
new Buffer: 26.604ms

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2024
@github-actions github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Oct 5, 2024
@github-actions

github-actions Bot commented Oct 5, 2024

Copy link
Copy Markdown
Contributor
Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/11196042300

@juanarbol

Copy link
Copy Markdown
Member

Interesting

Comment thread src/node_util.cc Outdated
Comment thread src/node_util.cc
}
}

static void IsInsideNodeModules(const FunctionCallbackInfo<Value>& args) {

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.

We can add V8 fast api to this to speed things up.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't know if it's safe to iterate over the stack in the fast calls - probably safer to leave it to a follow up to avoid regressions if it's not and isn't reproducible from the tests.

@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Oct 6, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

This previously compiles a script and run it in a new context
to avoid global pollution, which is more complex than necessary
and can be too slow for it to be reused in other cases. The
new implementation just checks the frames in C++ which is safe
from global pollution, faster and simpler.

The previous implementation also had a bug when the call site
is in a ESM, because ESM have URLs as their script names,
which don't start with '/' or '\' and will be skipped. The new
implementation removes the skipping to fix it for ESM.
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@joyeecheung

Copy link
Copy Markdown
Member Author

Rebased to resolve the merge conflict which caused the CI failures. PTAL again @anonrig

@anonrig

anonrig commented Oct 7, 2024

Copy link
Copy Markdown
Member

Linter is complaining @joyeecheung

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@joyeecheung joyeecheung mentioned this pull request Oct 7, 2024
9 tasks
@codecov

codecov Bot commented Oct 7, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (deb5eff) to head (c5cfa9c).
Report is 836 commits behind head on main.

Files with missing lines Patch % Lines
src/node_util.cc 70.00% 2 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55286      +/-   ##
==========================================
- Coverage   88.41%   88.40%   -0.02%     
==========================================
  Files         652      652              
  Lines      186756   186759       +3     
  Branches    36100    36106       +6     
==========================================
- Hits       165126   165109      -17     
- Misses      14906    14915       +9     
- Partials     6724     6735      +11     
Files with missing lines Coverage Δ
lib/buffer.js 100.00% <100.00%> (ø)
lib/internal/util.js 96.96% <ø> (-0.10%) ⬇️
src/node_util.cc 85.76% <70.00%> (-2.06%) ⬇️

... and 22 files with indirect coverage changes

@joyeecheung

Copy link
Copy Markdown
Member Author

Backport in #56927

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported-to-v22.x PRs backported to the v22.x-staging branch. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants