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

[dario-piotrowicz]

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++ :slightly_smiling_face:

[nodejs-github-bot]

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

[dario-piotrowicz]

I've added https://github.com/nodejs/node/labels/semver-major 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)

[codecov[bot]]

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 :warning:

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%)	:arrow_down:
src/node_file.cc	75.87% <76.92%> (+0.02%)	:arrow_up:

... and 93 files with indirect coverage changes

:rocket: New features to boost your workflow:

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

[geeksilva97]

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

[dario-piotrowicz]

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

Hey @geeksilva97 :wave:

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

Yes, as mentioned in the PR's description :smiley:

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

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

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

[geeksilva97]

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]

Thanks for replying, @dario-piotrowicz .

Of course! it's my pleasure :smile:

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 :slightly_smiling_face:

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 :thinking:

But either way, it sounds like this PR can be blocked for a while :face_holding_back_tears: (https://github.com/nodejs/node/pull/58624#discussion_r2135813234)