src: move cpSync dir copy logic completely to C++ #58624

dario-piotrowicz · 2025-06-07T22:40:11Z

prior to these changes cpSync would copy directories using C++ logic only if the user didn't provide a filtering function to the cpSync call, the changes here make it so that instead C++ is used to copy directories even when a filtering function is provided

Some benchmarking I performed locally:

Note

The perf gain is nice (although not huge), the biggest benefit of this change for me is code improvement, since with these changes we no longer need to have the same exact functionality implemented in both JS and C++, also this should help moving more cpSync code over C++ 🙂

nodejs-github-bot · 2025-06-07T22:41:17Z

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

anonrig · 2025-06-07T23:01:39Z

src/node_file.cc

+      Local<Value> argv[] = {
+          String::NewFromUtf8(
+              env->isolate(), src.c_str(), v8::NewStringType::kNormal)
+              .ToLocalChecked(),


Can we avoid tolocalchecked() calls since they are unsafe. Also you can use ToV8Value method which reduces the code.

Regarding the safeness of the code, I did run this with different filter functions, I also tried not returning anything or throwing within the filter function itself and every case seems totally fine with ToLocalChecked 🤔

The key challenge with ToLocalChecked() is that it causes the node.js process to crash even on otherwise recoverable errors. It really is better to use the ToLocal(...) APIs instead and we've been going through and actively converting away from using ToLocalChecked as much as possible. I agree with @anonrig that we should avoid it here also.

If string creation fails due to external factors or simply due a bug in v8, this will directly crash process

Ok, thanks a lot for the clarification both 🫶, I'll swap this with ToLocal 👍

I hope this works for you? 🙂 🙏

bb2d3c4

src/node_file.cc

anonrig · 2025-06-07T23:02:34Z

src/node_file.cc

+  if (args[7]->IsFunction()) {
+    Local<v8::Function> args_filter_fn = args[7].As<v8::Function>();
+
+    filter_fn = [env, args_filter_fn](std::string src,


If you use std::string view you will improve performance.

great! thanks! 😄

Does this look good? 🙂

c9e25e8

src/node_file.cc

dario-piotrowicz · 2025-06-07T23:49:06Z

I've added semver-major PRs that contain breaking changes and should be released in the next major version. since my implementation causes the filtering function arguments to be slightly different:

(notice the addition of ./ in my version)

I do like the extra ./ as it looks more correct to me, please let me know if you disagree and I'll look into removing that and align better the two implementations (or you do agree with the change but would prefer me to do in a separate semver-major PR and make this a non semver-major one)

src/node_file.cc

jasnell · 2025-06-08T00:18:39Z

src/node_file.cc

+              .ToLocalChecked(),
+          String::NewFromUtf8(
+              env->isolate(), dest.data(), v8::NewStringType::kNormal)
+              .ToLocalChecked()};


One additional difference here that will be problematic is that this is passing the src and dest to the filter callback as a string always. With the currrent implementation, however, if we pass the path in as a Buffer or URL then the filter callback receives the Buffer and URL... I notice that this appears to be undocumented in api docs sadly)... We have to preserve that because this implementation tries to interpret all src and dest paths as UTF8 sequences which they quite likely won't be (the reason we accept Buffer in the first place is that many filenames in older legacy systems use legacy text encodings).

fs.cpSync(Buffer.from('a.js'), Buffer.from('b.js'), { filter(...args) { console.log(...args); return true } });

Ah ok I see

Thanks a lot for pointing this out, I'll address it 🙏

@jasnell Are you sure about this statement?

if we pass the path in as a Buffer or URL then the filter callback receives the Buffer and URL...

Because after giving it a quick try that does not seem to be the case for me 🤔

I've also commented this here: #58627 (review)

@jasnell Given our conversation in #58627 and also #58634, what do you think it means for this PR? Do you think we can proceed with my C++ implementation even though it is not converting the values? (it is not making the issue any worse, I guess? 🤔)

Or should this PR be blocked by #58634 as well? 🤔

(PS: I'd be pretty hesitant addressing #58634 here since this PR is only for cpSync, whilst the issue is both for cp and cpSync 🤔)

I'd like us to see if we can figure out a fix for #58634 first

The reason is simple enough... I'd like to make sure that the fix to the bug doesn't conflict with the changes you're making here... that is, I want to make sure landing this doesn't make fixing the bug more difficult / convoluted / etc or make sure we don't end up having to revert this in part when fixing the issue. Even if we just can get a sense of how we want to fix the bug it should be enough to unblock this.

Ah ok I understand, yeah that totally makes sense to me, thanks for the info 🙏

codecov · 2025-06-08T14:02:52Z

Codecov Report

Attention: Patch coverage is 81.81818% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.63%. Comparing base (bba07d7) to head (be0da14).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_file.cc	76.92%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58624      +/-   ##
==========================================
- Coverage   90.17%   89.63%   -0.55%     
==========================================
  Files         637      637              
  Lines      188023   187995      -28     
  Branches    36887    36591     -296     
==========================================
- Hits       169552   168506    -1046     
- Misses      11223    12166     +943     
- Partials     7248     7323      +75

Files with missing lines	Coverage Δ
lib/internal/fs/cp/cp-sync.js	`47.15% <100.00%> (-9.82%)`	⬇️
src/node_file.cc	`75.87% <76.92%> (+0.02%)`	⬆️

... and 93 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

geeksilva97 · 2025-06-08T17:02:31Z

May I assume this change is for making cpSync 'faster'? If so, wouldn't that require a benchmark CI?

dario-piotrowicz · 2025-06-08T22:00:50Z

May I assume this change is for making cpSync 'faster'? If so, wouldn't that require a benchmark CI?

Hey @geeksilva97 👋

May I assume this change is for making cpSync 'faster'?

Yes, as mentioned in the PR's description 😃

If so, wouldn't that require a benchmark CI?

Sorry, I am not completely sure what you're asking 🙂

Are you asking me to create a new benchmark in file in the benchmark folder such as benchmark/fs
/bench-cpSync.js? 🙂

geeksilva97 · 2025-06-09T12:19:45Z

May I assume this change is for making cpSync 'faster'? If so, wouldn't that require a benchmark CI?

Hey @geeksilva97 👋

May I assume this change is for making cpSync 'faster'?

Yes, as mentioned in the PR's description 😃

If so, wouldn't that require a benchmark CI?

Sorry, I am not completely sure what you're asking 🙂

Are you asking me to create a new benchmark in file in the benchmark folder such as benchmark/fs /bench-cpSync.js? 🙂

Thanks for replying, @dario-piotrowicz . About the benchmark, I was asking if this PR needs a benchmark ci run - that one we add with needs-benchmark-ci tag

dario-piotrowicz · 2025-06-09T18:24:52Z

Thanks for replying, @dario-piotrowicz .

Of course! it's my pleasure 😄

About the benchmark, I was asking if this PR needs a benchmark ci run - that one we add with needs-benchmark-ci tag

Ah ok, sorry I wasn't familiar with that tag 🙂

We could add it but since I don't see any benchmarks in /benchmark that call cpSync with a filtering function I don't think that we would see any relevant result in a CI run? (I am assuming that the tag triggers the run of those benchmarks right?)

But if you want I can add such a benchmark and then the tag 🤔

But either way, it sounds like this PR can be blocked for a while 🥹 (#58624 (comment))

src/node_file.cc

prior to these changes cpSync would copy directories using C++ logic only if the user didn't provide a filtering function to the cpSync call, the changes here make it so that instead C++ is used to copy directories even when a filtering function is provided

use std::optional

use std:string_view

Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>

Co-authored-by: Anna Henningsen <github@addaleax.net>

dario-piotrowicz requested a review from anonrig June 7, 2025 22:40

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 7, 2025

dario-piotrowicz force-pushed the dario/move-cpsync-copyDir-to-cpp-filter branch 2 times, most recently from 7081997 to 055b84b Compare June 7, 2025 22:58

anonrig reviewed Jun 7, 2025

View reviewed changes

dario-piotrowicz added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 7, 2025

anonrig reviewed Jun 7, 2025

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

jasnell reviewed Jun 8, 2025

View reviewed changes

dario-piotrowicz added the blocked PRs that are blocked by other issues or PRs. label Jun 9, 2025

dario-piotrowicz force-pushed the dario/move-cpsync-copyDir-to-cpp-filter branch from f9efe3c to a3b912b Compare June 10, 2025 12:44

addaleax reviewed Jun 12, 2025

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

addaleax reviewed Jun 12, 2025

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

dario-piotrowicz and others added 8 commits June 12, 2025 21:24

fixup! src: move cpSync dir copy logic completely to C++

fb14768

use std::optional

fixup! src: move cpSync dir copy logic completely to C++

8f6e69a

use std:string_view

Update src/node_file.cc

6437abb

Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>

fix cpp formatting

39d2447

fix unqualified-id error

431bcd1

avoid ToLocalChecked calls

ed9a480

(try to) fix windows paths issue

8563b73

dario-piotrowicz added 2 commits June 12, 2025 21:24

fix cpp linting error

4d9cd51

actually fix cpp formatting

d95e989

dario-piotrowicz force-pushed the dario/move-cpsync-copyDir-to-cpp-filter branch from a3b912b to d95e989 Compare June 12, 2025 20:24

Apply suggestions from code review

be0da14

Co-authored-by: Anna Henningsen <github@addaleax.net>

Uh oh!

src: move cpSync dir copy logic completely to C++ #58624

Are you sure you want to change the base?

src: move cpSync dir copy logic completely to C++ #58624

Conversation

dario-piotrowicz commented Jun 7, 2025

Uh oh!

nodejs-github-bot commented Jun 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dario-piotrowicz commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jasnell Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

geeksilva97 commented Jun 8, 2025

Uh oh!

dario-piotrowicz commented Jun 8, 2025

Uh oh!

geeksilva97 commented Jun 9, 2025

Uh oh!

dario-piotrowicz commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dario-piotrowicz commented Jun 7, 2025 •

edited

Loading

jasnell Jun 8, 2025 •

edited

Loading

codecov bot commented Jun 8, 2025 •

edited

Loading