nodejs
fs: improve `cpSync` dest overriding performance
move the logic in cpSync that overrides existing dest files from JavaScript to C++ increasing its performance
This is the type of perf improvement that this change makes:
Your benchmark needs options.force in order to trigger the path you're updating, isn't it? Or do you have force: true as the default value.
Your benchmark needs options.force in order to trigger the path you're updating, isn't it? Or do you have force: true as the default value.
Yes, force: true is the default :slightly_smiling_face:
Codecov Report
Attention: Patch coverage is 31.37255% with 35 lines in your changes missing coverage. Please review.
Project coverage is 90.23%. Comparing base (
73e7bd1) to head (33cc599). Report is 33 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/node_file.cc | 27.90% | 20 Missing and 11 partials :warning: |
| src/env-inl.h | 0.00% | 4 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #58160 +/- ##
=======================================
Coverage 90.22% 90.23%
=======================================
Files 633 633
Lines 186871 186925 +54
Branches 36685 36703 +18
=======================================
+ Hits 168612 168667 +55
- Misses 11042 11054 +12
+ Partials 7217 7204 -13
| Files with missing lines | Coverage Ξ | |
|---|---|---|
| lib/internal/fs/cp/cp-sync.js | 92.13% <100.00%> (-0.11%) |
:arrow_down: |
| src/env.h | 98.14% <ΓΈ> (ΓΈ) |
|
| src/env-inl.h | 93.39% <0.00%> (-0.81%) |
:arrow_down: |
| src/node_file.cc | 76.12% <27.90%> (-0.87%) |
:arrow_down: |
: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.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Could we get another benchmark that only removed existing files? I guess it will also be faster, it is just helpful to know by how much.
Sorry could you clarify what you mean? :pray:
This code, as far as my understanding goes, is only effecting cpSync when it is overriding the destination file, no other cp operation should see any difference here :thinking:
Windows error
src\node_file.cc(40,10): fatal error : 'utime.h' file not found [D:\a\node\node\libnode.vcxproj]
Commit Queue failed
- Loading data for nodejs/node/pull/58160 β Done loading data for nodejs/node/pull/58160 ----------------------------------- PR info ------------------------------------ Title fs: improve `cpSync` dest overriding performance (#58160) Author Dario Piotrowicz <[email protected]> (@dario-piotrowicz) Branch dario-piotrowicz:dario/move-cpsync-mayCopyFile-logic-to-cpp -> nodejs:main Labels c++, fs, author ready, needs-ci Commits 13 - fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - Apply suggestions from code review - fix and add OnScopeLeave calls - throw unlink errno error to match existing node behavior - use none flag instead of unnecessary skip_existing - format cpp code - fix EPERM throwing code - improve std code and properly throw std errors Committers 2 - Dario Piotrowicz <[email protected]> - Dario Piotrowicz <[email protected]> PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> -------------------------------------------------------------------------------- βΉ This PR was created on Sun, 04 May 2025 13:29:18 GMT β Approvals: 2 β - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/58160#pullrequestreview-2831887442 β - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/58160#pullrequestreview-2834980041 β Last GitHub CI successful βΉ Last Full PR CI on 2025-05-15T13:26:47Z: https://ci.nodejs.org/job/node-test-pull-request/66807/ - Querying data for job/node-test-pull-request/66807/ β Last Jenkins CI successful -------------------------------------------------------------------------------- β No git cherry-pick in progress β No git am in progress β No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD β origin/main is now up-to-date - Downloading patch for 58160 From https://github.com/nodejs/node * branch refs/pull/58160/merge -> FETCH_HEAD β Fetched commits as 73e7bd1c0e3e..aa6904c57c53 -------------------------------------------------------------------------------- [main 03f5bdcaef] fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sat May 3 22:07:30 2025 +0100 2 files changed, 57 insertions(+), 7 deletions(-) Auto-merging typings/internalBinding/fs.d.ts [main 9cd67427ca] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 14:48:16 2025 +0100 1 file changed, 2 insertions(+) [main 64bf218fd5] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 14:49:33 2025 +0100 1 file changed, 7 insertions(+), 8 deletions(-) [main 73f56127f4] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 14:59:48 2025 +0100 1 file changed, 8 insertions(+), 6 deletions(-) [main 536ca733a5] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 15:15:27 2025 +0100 1 file changed, 10 insertions(+), 2 deletions(-) [main 0162c0be3a] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 17:02:19 2025 +0100 1 file changed, 18 insertions(+), 9 deletions(-) [main 9eb0b5b406] Apply suggestions from code review Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 22:37:51 2025 +0100 1 file changed, 2 insertions(+), 1 deletion(-) [main f0fd441958] fix and add OnScopeLeave calls Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 22:40:41 2025 +0100 1 file changed, 3 insertions(+), 1 deletion(-) [main ee4c46bc86] throw unlink errno error to match existing node behavior Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 23:50:58 2025 +0100 1 file changed, 5 insertions(+), 1 deletion(-) [main 38125556f5] use none flag instead of unnecessary skip_existing Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 23:56:38 2025 +0100 1 file changed, 1 insertion(+), 1 deletion(-) [main 0a47832894] format cpp code Author: Dario Piotrowicz <[email protected]> Date: Tue May 6 00:07:54 2025 +0100 1 file changed, 3 insertions(+), 5 deletions(-) [main 1d7e3ec434] fix EPERM throwing code Author: Dario Piotrowicz <[email protected]> Date: Sat May 10 16:05:29 2025 +0100 1 file changed, 1 insertion(+), 3 deletions(-) [main 36f35e51b0] improve std code and properly throw std errors Author: Dario Piotrowicz <[email protected]> Date: Tue May 13 00:51:54 2025 +0100 3 files changed, 16 insertions(+), 11 deletions(-) β Patches applied There are 13 commits in the PR. Attempting autorebase. Rebasing (2/21) Rebasing (3/21) Rebasing (4/21) Rebasing (5/21) Rebasing (6/21) Rebasing (7/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- fs: improve `cpSync` dest overriding performancehttps://github.com/nodejs/node/actions/runs/15098587944move the logic in
cpSyncthat overrides existing dest files from JavaScript to C++ increasing its performancePR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 386b8c5406] fs: improve
cpSyncdest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sat May 3 22:07:30 2025 +0100 3 files changed, 77 insertions(+), 7 deletions(-) Rebasing (8/21) Rebasing (9/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- Apply suggestions from code reviewCo-authored-by: Yagiz Nizipli <[email protected]> PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 29802ab0c5] Apply suggestions from code review Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 22:37:51 2025 +0100 1 file changed, 2 insertions(+), 1 deletion(-) Rebasing (10/21) Rebasing (11/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- fix and add OnScopeLeave calls
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 7bafd2d16c] fix and add OnScopeLeave calls Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 22:40:41 2025 +0100 1 file changed, 3 insertions(+), 1 deletion(-) Rebasing (12/21) Rebasing (13/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- throw unlink errno error to match existing node behavior
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 37bfce81ed] throw unlink errno error to match existing node behavior Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 23:50:58 2025 +0100 1 file changed, 5 insertions(+), 1 deletion(-) Rebasing (14/21) Rebasing (15/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- use none flag instead of unnecessary skip_existing
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 7655329128] use none flag instead of unnecessary skip_existing Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 23:56:38 2025 +0100 1 file changed, 1 insertion(+), 1 deletion(-) Rebasing (16/21) Rebasing (17/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- format cpp code
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD af1d9a0ce9] format cpp code Author: Dario Piotrowicz <[email protected]> Date: Tue May 6 00:07:54 2025 +0100 1 file changed, 3 insertions(+), 5 deletions(-) Rebasing (18/21) Rebasing (19/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- fix EPERM throwing code
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 6c0327719a] fix EPERM throwing code Author: Dario Piotrowicz <[email protected]> Date: Sat May 10 16:05:29 2025 +0100 1 file changed, 1 insertion(+), 3 deletions(-) Rebasing (20/21) Rebasing (21/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- improve std code and properly throw std errors
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD a2bee3eb2c] improve std code and properly throw std errors Author: Dario Piotrowicz <[email protected]> Date: Tue May 13 00:51:54 2025 +0100 3 files changed, 16 insertions(+), 11 deletions(-) Successfully rebased and updated refs/heads/main.
βΉ Add
commit-queue-squashlabel to land the PR as one commit, orcommit-queue-rebaseto land as separate commits.
Commit Queue failed
- Loading data for nodejs/node/pull/58160 β Done loading data for nodejs/node/pull/58160 ----------------------------------- PR info ------------------------------------ Title fs: improve `cpSync` dest overriding performance (#58160) Author Dario Piotrowicz <[email protected]> (@dario-piotrowicz) Branch dario-piotrowicz:dario/move-cpsync-mayCopyFile-logic-to-cpp -> nodejs:main Labels c++, fs, author ready, needs-ci Commits 13 - fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - fixup! fs: improve `cpSync` dest overriding performance - Apply suggestions from code review - fix and add OnScopeLeave calls - throw unlink errno error to match existing node behavior - use none flag instead of unnecessary skip_existing - format cpp code - fix EPERM throwing code - improve std code and properly throw std errors Committers 1 - Dario Piotrowicz <[email protected]> PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> -------------------------------------------------------------------------------- βΉ This PR was created on Sun, 04 May 2025 13:29:18 GMT β Approvals: 2 β - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/58160#pullrequestreview-2831887442 β - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/58160#pullrequestreview-2849079402 β Last GitHub CI successful βΉ Last Full PR CI on 2025-05-23T10:46:19Z: https://ci.nodejs.org/job/node-test-pull-request/67024/ - Querying data for job/node-test-pull-request/67024/ β Last Jenkins CI successful -------------------------------------------------------------------------------- β No git cherry-pick in progress β No git am in progress β No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD β origin/main is now up-to-date - Downloading patch for 58160 From https://github.com/nodejs/node * branch refs/pull/58160/merge -> FETCH_HEAD β Fetched commits as d96d57d5a7cf..33cc59938e61 -------------------------------------------------------------------------------- [main a8bbced268] fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sat May 3 22:07:30 2025 +0100 2 files changed, 57 insertions(+), 7 deletions(-) [main bb41b9b9aa] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 14:48:16 2025 +0100 1 file changed, 2 insertions(+) [main 18a8622eb3] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 14:49:33 2025 +0100 1 file changed, 7 insertions(+), 8 deletions(-) [main f5ca508add] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 14:59:48 2025 +0100 1 file changed, 8 insertions(+), 6 deletions(-) [main 877ac242c3] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 15:15:27 2025 +0100 1 file changed, 10 insertions(+), 2 deletions(-) [main 292a0c3f11] fixup! fs: improve `cpSync` dest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sun May 4 17:02:19 2025 +0100 1 file changed, 18 insertions(+), 9 deletions(-) [main dcaa5749e9] Apply suggestions from code review Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 22:37:51 2025 +0100 1 file changed, 2 insertions(+), 1 deletion(-) [main e348fc4712] fix and add OnScopeLeave calls Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 22:40:41 2025 +0100 1 file changed, 3 insertions(+), 1 deletion(-) [main bb5f0e9836] throw unlink errno error to match existing node behavior Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 23:50:58 2025 +0100 1 file changed, 5 insertions(+), 1 deletion(-) [main 062e05d994] use none flag instead of unnecessary skip_existing Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 23:56:38 2025 +0100 1 file changed, 1 insertion(+), 1 deletion(-) [main 0b4702dd57] format cpp code Author: Dario Piotrowicz <[email protected]> Date: Tue May 6 00:07:54 2025 +0100 1 file changed, 3 insertions(+), 5 deletions(-) [main 519ec6acc7] fix EPERM throwing code Author: Dario Piotrowicz <[email protected]> Date: Sat May 10 16:05:29 2025 +0100 1 file changed, 1 insertion(+), 3 deletions(-) [main a67da1b60a] improve std code and properly throw std errors Author: Dario Piotrowicz <[email protected]> Date: Tue May 13 00:51:54 2025 +0100 3 files changed, 16 insertions(+), 11 deletions(-) β Patches applied There are 13 commits in the PR. Attempting autorebase. Rebasing (2/21) Rebasing (3/21) Rebasing (4/21) Rebasing (5/21) Rebasing (6/21) Rebasing (7/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- fs: improve `cpSync` dest overriding performancehttps://github.com/nodejs/node/actions/runs/15209909689move the logic in
cpSyncthat overrides existing dest files from JavaScript to C++ increasing its performancePR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD b6efda350e] fs: improve
cpSyncdest overriding performance Author: Dario Piotrowicz <[email protected]> Date: Sat May 3 22:07:30 2025 +0100 3 files changed, 77 insertions(+), 7 deletions(-) Rebasing (8/21) Rebasing (9/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- Apply suggestions from code reviewCo-authored-by: Yagiz Nizipli <[email protected]> PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD e519c037e1] Apply suggestions from code review Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 22:37:51 2025 +0100 1 file changed, 2 insertions(+), 1 deletion(-) Rebasing (10/21) Rebasing (11/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- fix and add OnScopeLeave calls
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD adcd89e344] fix and add OnScopeLeave calls Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 22:40:41 2025 +0100 1 file changed, 3 insertions(+), 1 deletion(-) Rebasing (12/21) Rebasing (13/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- throw unlink errno error to match existing node behavior
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD f4581fce42] throw unlink errno error to match existing node behavior Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 23:50:58 2025 +0100 1 file changed, 5 insertions(+), 1 deletion(-) Rebasing (14/21) Rebasing (15/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- use none flag instead of unnecessary skip_existing
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 9f34424581] use none flag instead of unnecessary skip_existing Author: Dario Piotrowicz <[email protected]> Date: Mon May 5 23:56:38 2025 +0100 1 file changed, 1 insertion(+), 1 deletion(-) Rebasing (16/21) Rebasing (17/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- format cpp code
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 8ee34143ef] format cpp code Author: Dario Piotrowicz <[email protected]> Date: Tue May 6 00:07:54 2025 +0100 1 file changed, 3 insertions(+), 5 deletions(-) Rebasing (18/21) Rebasing (19/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- fix EPERM throwing code
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 579ce500b3] fix EPERM throwing code Author: Dario Piotrowicz <[email protected]> Date: Sat May 10 16:05:29 2025 +0100 1 file changed, 1 insertion(+), 3 deletions(-) Rebasing (20/21) Rebasing (21/21) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- improve std code and properly throw std errors
PR-URL: https://github.com/nodejs/node/pull/58160 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
[detached HEAD 41506c49f0] improve std code and properly throw std errors Author: Dario Piotrowicz <[email protected]> Date: Tue May 13 00:51:54 2025 +0100 3 files changed, 16 insertions(+), 11 deletions(-) Successfully rebased and updated refs/heads/main.
βΉ Add
commit-queue-squashlabel to land the PR as one commit, orcommit-queue-rebaseto land as separate commits.