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

joyeecheung · 2024-10-05T20:00:08Z

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 · 2024-10-05T20:06:24Z

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

github-actions · 2024-10-05T20:19:51Z

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 · 2024-10-05T21:09:08Z

Interesting

src/node_util.cc

anonrig · 2024-10-06T03:19:41Z

src/node_util.cc

@@ -292,10 +292,47 @@ static void GetCallSite(const FunctionCallbackInfo<Value>& args) {

    callsite_objects.push_back(obj);
  }
+}
+
+static void IsInsideNodeModules(const FunctionCallbackInfo<Value>& args) {


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

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.

nodejs-github-bot · 2024-10-06T03:30:32Z

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

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 · 2024-10-07T13:48:12Z

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

anonrig · 2024-10-07T13:48:55Z

Linter is complaining @joyeecheung

nodejs-github-bot · 2024-10-07T13:53:38Z

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

nodejs-github-bot · 2024-10-07T14:55:02Z

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

codecov · 2024-10-07T16:04:29Z

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 40 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

nodejs-github-bot · 2024-10-08T10:19:47Z

Landed in 1435338

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. PR-URL: nodejs#55286 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

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. PR-URL: #55286 Backport-PR-URL: #55217 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Refs: #52697

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 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2024

joyeecheung force-pushed the node-modules-check branch from 5273982 to d0d6f97 Compare October 5, 2024 20:10

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

anonrig approved these changes Oct 6, 2024

View reviewed changes

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 bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2024

joyeecheung force-pushed the node-modules-check branch from d0d6f97 to eb8eba8 Compare October 7, 2024 13:46

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024

anonrig approved these changes Oct 7, 2024

View reviewed changes

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

fixup! src: implement IsInsideNodeModules() in C++

c5cfa9c

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024

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

joyeecheung mentioned this pull request Oct 7, 2024

Tracking issue: require(esm) #52697

Open

8 tasks

anonrig approved these changes Oct 7, 2024

View reviewed changes

anonrig requested a review from jasnell October 7, 2024 18:45

legendecas approved these changes Oct 8, 2024

View reviewed changes

joyeecheung 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. labels Oct 8, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 8, 2024

nodejs-github-bot merged commit 1435338 into nodejs:main Oct 8, 2024
63 checks passed

aduh95 added the backported-to-v22.x PRs backported to the v22.x-staging branch. label Oct 12, 2024

aduh95 mentioned this pull request Oct 14, 2024

2024-10-16, Version 23.0.0 (Current) #55338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joyeecheung commented Oct 5, 2024 •

edited

Loading

joyeecheung commented Oct 5, 2024

github-actions bot commented Oct 5, 2024

juanarbol commented Oct 5, 2024

anonrig Oct 6, 2024

joyeecheung Oct 7, 2024

nodejs-github-bot commented Oct 6, 2024

joyeecheung commented Oct 7, 2024

anonrig commented Oct 7, 2024

nodejs-github-bot commented Oct 7, 2024

nodejs-github-bot commented Oct 7, 2024

codecov bot commented Oct 7, 2024 •

edited

Loading

nodejs-github-bot commented Oct 8, 2024

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

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

Conversation

joyeecheung commented Oct 5, 2024 • edited Loading

joyeecheung commented Oct 5, 2024

github-actions bot commented Oct 5, 2024

juanarbol commented Oct 5, 2024

anonrig Oct 6, 2024

Choose a reason for hiding this comment

joyeecheung Oct 7, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 6, 2024

joyeecheung commented Oct 7, 2024

anonrig commented Oct 7, 2024

nodejs-github-bot commented Oct 7, 2024

nodejs-github-bot commented Oct 7, 2024

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Oct 8, 2024

joyeecheung commented Oct 5, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading