Skip to content

fs: improve cpSync dest overriding performance #58160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented May 4, 2025

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:
Screenshot at 2025-05-13 01-47-25

@dario-piotrowicz dario-piotrowicz requested a review from anonrig May 4, 2025 13:29
@nodejs-github-bot 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 May 4, 2025
@anonrig
Copy link
Member

anonrig commented May 4, 2025

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.

@dario-piotrowicz
Copy link
Member Author

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 🙂

Copy link

codecov bot commented May 4, 2025

Codecov Report

Attention: Patch coverage is 31.37255% with 35 lines in your changes missing coverage. Please review.

Project coverage is 90.19%. Comparing base (cfd2021) to head (aa6904c).
Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 27.90% 20 Missing and 11 partials ⚠️
src/env-inl.h 0.00% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #58160    +/-   ##
========================================
  Coverage   90.18%   90.19%            
========================================
  Files         631      633     +2     
  Lines      186693   186880   +187     
  Branches    36665    36690    +25     
========================================
+ Hits       168378   168563   +185     
+ Misses      11121    11097    -24     
- Partials     7194     7220    +26     
Files with missing lines Coverage Δ
lib/internal/fs/cp/cp-sync.js 92.13% <100.00%> (-0.11%) ⬇️
src/env.h 98.14% <ø> (ø)
src/env-inl.h 93.39% <0.00%> (-0.81%) ⬇️
src/node_file.cc 76.07% <27.90%> (-0.87%) ⬇️

... and 34 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.

BridgeAR

This comment was marked as resolved.

@dario-piotrowicz
Copy link
Member Author

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? 🙏

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 🤔

@BridgeAR
Copy link
Member

BridgeAR commented May 4, 2025

You are absolutely correct! I misread. Please ignore my comment :)

@dario-piotrowicz
Copy link
Member Author

Ah ok no worries 😄👍

@anonrig
Copy link
Member

anonrig commented May 5, 2025

Windows error

src\node_file.cc(40,10): fatal  error : 'utime.h' file not found [D:\a\node\node\libnode.vcxproj]

@dario-piotrowicz dario-piotrowicz force-pushed the dario/move-cpsync-mayCopyFile-logic-to-cpp branch from 4a5cad0 to 2828919 Compare May 10, 2025 10:36
@dario-piotrowicz dario-piotrowicz force-pushed the dario/move-cpsync-mayCopyFile-logic-to-cpp branch from 6956e4b to 07c6f8e Compare May 12, 2025 12:33
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 13, 2025
@dario-piotrowicz dario-piotrowicz added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 13, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 15, 2025
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants