Skip to content

Commit f776f77

Browse files
dario-piotrowiczdario-piotrowicz
committed
fs: improve cpSync no-filter copyDir performance
move the logic in `cpSync` that copies a directory from src to dest from JavaScript to C++ increasing its performance Note: this improvement is not applied if the filter option is provided, such optimization will be looked into separately
1 parent 19e0d47 commit f776f77

File tree

5 files changed

+222
-9
lines changed

5 files changed

+222
-9
lines changed

‎lib/internal/fs/cp/cp-sync.js‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -133,18 +133,22 @@ function setDestTimestamps(src, dest) {
133133

134134
// TODO(@anonrig): Move this function to C++.
135135
function onDir(srcStat, destStat, src, dest, opts) {
136-
if (!destStat) return mkDirAndCopy(srcStat.mode, src, dest, opts);
136+
if (!destStat) return copyDir(src, dest, opts, true, srcStat.mode);
137137
return copyDir(src, dest, opts);
138138
}
139139

140-
function mkDirAndCopy(srcMode, src, dest, opts) {
141-
mkdirSync(dest);
142-
copyDir(src, dest, opts);
143-
return setDestMode(dest, srcMode);
144-
}
140+
function copyDir(src, dest, opts, mkDir, srcMode) {
141+
if (!opts.filter) {
142+
// the caller didn't provide a js filter function, in this case
143+
// we can run the whole function faster in C++
144+
// TODO(dario-piotrowicz): look into making cpSyncCopyDir also accept the potential filter function
145+
return fsBinding.cpSyncCopyDir(src, dest, opts.force, opts.errorOnExist, opts.verbatimSymlinks, opts.preserveTimestamps);
146+
}
147+
148+
if (mkDir) {
149+
mkdirSync(dest);
150+
}
145151

146-
// TODO(@anonrig): Move this function to C++.
147-
function copyDir(src, dest, opts) {
148152
const dir = opendirSync(src);
149153

150154
try {
@@ -169,6 +173,10 @@ function copyDir(src, dest, opts) {
169173
}
170174
} finally {
171175
dir.closeSync();
176+
177+
if (srcMode !== undefined) {
178+
setDestMode(dest, srcMode);
179+
}
172180
}
173181
}
174182

‎src/node_errors.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
7878
V(ERR_FS_CP_EINVAL, Error) \
7979
V(ERR_FS_CP_DIR_TO_NON_DIR, Error) \
8080
V(ERR_FS_CP_NON_DIR_TO_DIR, Error) \
81+
V(ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY, Error) \
8182
V(ERR_FS_EISDIR, Error) \
8283
V(ERR_FS_CP_SOCKET, Error) \
8384
V(ERR_FS_CP_FIFO_PIPE, Error) \

‎src/node_file.cc‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3417,6 +3417,203 @@ static void CpSyncOverrideFile(const FunctionCallbackInfo<Value>& args) {
34173417
}
34183418
}
34193419

3420+
std::vector<std::string> normalizePathToArray(
3421+
const std::filesystem::path& path) {
3422+
std::vector<std::string> parts;
3423+
std::filesystem::path absPath = std::filesystem::absolute(path);
3424+
for (const auto& part : absPath) {
3425+
if (!part.empty()) parts.push_back(part.string());
3426+
}
3427+
return parts;
3428+
}
3429+
3430+
// Return true if dest is a subdir of src, otherwise false.
3431+
bool isInsideDir(const std::filesystem::path& src,
3432+
const std::filesystem::path& dest) {
3433+
auto srcArr = normalizePathToArray(src);
3434+
auto destArr = normalizePathToArray(dest);
3435+
if (srcArr.size() > destArr.size()) return false;
3436+
return std::equal(srcArr.begin(), srcArr.end(), destArr.begin());
3437+
}
3438+
3439+
static void CpSyncCopyDir(const FunctionCallbackInfo<Value>& args) {
3440+
Environment* env = Environment::GetCurrent(args);
3441+
Isolate* isolate = env->isolate();
3442+
3443+
CHECK_EQ(args.Length(), 6); // src, dest, force, errorOnExist,
3444+
// verbatimSymlinks, preserveTimestamps
3445+
3446+
BufferValue src(isolate, args[0]);
3447+
CHECK_NOT_NULL(*src);
3448+
ToNamespacedPath(env, &src);
3449+
3450+
BufferValue dest(isolate, args[1]);
3451+
CHECK_NOT_NULL(*dest);
3452+
ToNamespacedPath(env, &dest);
3453+
3454+
bool force = args[2]->IsTrue();
3455+
bool error_on_exist = args[3]->IsTrue();
3456+
bool verbatim_symlinks = args[4]->IsTrue();
3457+
bool preserve_timestamps = args[5]->IsTrue();
3458+
3459+
std::error_code error;
3460+
std::filesystem::create_directories(*dest, error);
3461+
if (error) {
3462+
return env->ThrowStdErrException(error, "cp", *dest);
3463+
}
3464+
3465+
auto file_copy_opts = std::filesystem::copy_options::recursive;
3466+
if (force) {
3467+
file_copy_opts |= std::filesystem::copy_options::overwrite_existing;
3468+
} else if (error_on_exist) {
3469+
file_copy_opts |= std::filesystem::copy_options::none;
3470+
} else {
3471+
file_copy_opts |= std::filesystem::copy_options::skip_existing;
3472+
}
3473+
3474+
std::function<bool(std::filesystem::path, std::filesystem::path)>
3475+
copy_dir_contents;
3476+
copy_dir_contents = [verbatim_symlinks,
3477+
&copy_dir_contents,
3478+
&env,
3479+
file_copy_opts,
3480+
preserve_timestamps](std::filesystem::path src,
3481+
std::filesystem::path dest) {
3482+
std::error_code error;
3483+
for (auto dir_entry : std::filesystem::directory_iterator(src)) {
3484+
auto filename = dir_entry.path().filename();
3485+
auto dest_file = dest / filename;
3486+
3487+
if (dir_entry.is_symlink()) {
3488+
auto src_entry_path = std::filesystem::relative(dir_entry.path(), src);
3489+
3490+
if (verbatim_symlinks) {
3491+
std::filesystem::copy_symlink(dir_entry.path(), dest_file, error);
3492+
if (error) {
3493+
env->ThrowStdErrException(error, "cp", dest.c_str());
3494+
return false;
3495+
}
3496+
} else {
3497+
auto target =
3498+
std::filesystem::read_symlink(dir_entry.path().c_str(), error);
3499+
if (error) {
3500+
env->ThrowStdErrException(error, "cp", dest.c_str());
3501+
return false;
3502+
}
3503+
auto target_absolute = std::filesystem::weakly_canonical(
3504+
std::filesystem::absolute(src / target));
3505+
3506+
if (std::filesystem::exists(dest_file)) {
3507+
if (std::filesystem::is_regular_file(dest_file)) {
3508+
env->ThrowStdErrException(
3509+
std::make_error_code(std::errc::file_exists),
3510+
"cp",
3511+
dest_file.c_str());
3512+
return false;
3513+
}
3514+
3515+
auto current_target =
3516+
std::filesystem::read_symlink(dest_file.c_str(), error);
3517+
if (error) {
3518+
env->ThrowStdErrException(error, "cp", dest.c_str());
3519+
return false;
3520+
}
3521+
3522+
if (isInsideDir(target, current_target)) {
3523+
std::string message =
3524+
"Cannot copy %s to a subdirectory of self %s";
3525+
THROW_ERR_FS_CP_EINVAL(
3526+
env, message.c_str(), target.c_str(), current_target.c_str());
3527+
return false;
3528+
}
3529+
3530+
// Prevent copy if src is a subdir of dest since unlinking
3531+
// dest in this case would result in removing src contents
3532+
// and therefore a broken symlink would be created.
3533+
if (std::filesystem::is_directory(dest_file) &&
3534+
isInsideDir(current_target, target)) {
3535+
std::string message = "cannot overwrite %s with %s";
3536+
THROW_ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY(
3537+
env, message.c_str(), current_target.c_str(), target.c_str());
3538+
return false;
3539+
}
3540+
3541+
// symlinks get overridden by cp even if force: false, this is being
3542+
// applied here for backward compatibility, but is it correct? or is
3543+
// it a bug?
3544+
std::filesystem::remove(dest_file, error);
3545+
if (error) {
3546+
env->ThrowStdErrException(error, "cp", dest.c_str());
3547+
return false;
3548+
}
3549+
}
3550+
3551+
if (dir_entry.is_directory()) {
3552+
std::filesystem::create_directory_symlink(
3553+
target_absolute, dest_file, error);
3554+
} else {
3555+
std::filesystem::create_symlink(target_absolute, dest_file, error);
3556+
}
3557+
if (error) {
3558+
env->ThrowStdErrException(error, "cp", dest.c_str());
3559+
return false;
3560+
}
3561+
}
3562+
} else if (dir_entry.is_directory()) {
3563+
auto src_entry_path = std::filesystem::relative(dir_entry.path(), src);
3564+
auto filename = dir_entry.path().filename();
3565+
std::filesystem::create_directory(dest_file);
3566+
auto success = copy_dir_contents(src / filename, dest_file);
3567+
if (!success) {
3568+
return false;
3569+
}
3570+
} else if (dir_entry.is_regular_file()) {
3571+
auto src_entry_path = std::filesystem::relative(dir_entry.path(), src);
3572+
auto dest_entry_path = dest / src_entry_path;
3573+
std::filesystem::copy_file(
3574+
dir_entry.path(), dest_entry_path, file_copy_opts, error);
3575+
if (error) {
3576+
env->ThrowStdErrException(error, "cp", dest.c_str());
3577+
return false;
3578+
}
3579+
3580+
if (preserve_timestamps) {
3581+
uv_fs_t req;
3582+
auto cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
3583+
int result =
3584+
uv_fs_stat(nullptr, &req, dir_entry.path().c_str(), nullptr);
3585+
if (is_uv_error(result)) {
3586+
env->ThrowUVException(
3587+
result, "stat", nullptr, dir_entry.path().c_str());
3588+
return false;
3589+
}
3590+
3591+
const uv_stat_t* const s = static_cast<const uv_stat_t*>(req.ptr);
3592+
const double source_atime =
3593+
s->st_atim.tv_sec + s->st_atim.tv_nsec / 1e9;
3594+
const double source_mtime =
3595+
s->st_mtim.tv_sec + s->st_mtim.tv_nsec / 1e9;
3596+
3597+
int utime_result = uv_fs_utime(nullptr,
3598+
&req,
3599+
dest_entry_path.c_str(),
3600+
source_atime,
3601+
source_mtime,
3602+
nullptr);
3603+
if (is_uv_error(utime_result)) {
3604+
env->ThrowUVException(
3605+
utime_result, "utime", nullptr, dest_entry_path.c_str());
3606+
return false;
3607+
}
3608+
}
3609+
}
3610+
}
3611+
return true;
3612+
};
3613+
3614+
copy_dir_contents(std::filesystem::path(*src), std::filesystem::path(*dest));
3615+
}
3616+
34203617
BindingData::FilePathIsFileReturnType BindingData::FilePathIsFile(
34213618
Environment* env, const std::string& file_path) {
34223619
THROW_IF_INSUFFICIENT_PERMISSIONS(
@@ -3757,6 +3954,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
37573954

37583955
SetMethod(isolate, target, "cpSyncCheckPaths", CpSyncCheckPaths);
37593956
SetMethod(isolate, target, "cpSyncOverrideFile", CpSyncOverrideFile);
3957+
SetMethod(isolate, target, "cpSyncCopyDir", CpSyncCopyDir);
37603958

37613959
StatWatcher::CreatePerIsolateProperties(isolate_data, target);
37623960
BindingData::CreatePerIsolateProperties(isolate_data, target);
@@ -3870,6 +4068,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
38704068

38714069
registry->Register(CpSyncCheckPaths);
38724070
registry->Register(CpSyncOverrideFile);
4071+
registry->Register(CpSyncCopyDir);
38734072

38744073
registry->Register(Chmod);
38754074
registry->Register(FChmod);

‎test/parallel/test-fs-cp.mjs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,7 +447,7 @@ if (!isWindows && !isInsideDirWithUnusualChars) {
447447
force: false,
448448
recursive: true,
449449
}),
450-
{ code: 'ERR_FS_CP_EEXIST' }
450+
{ code: 'EEXIST' }
451451
);
452452
}
453453

@@ -493,6 +493,9 @@ if (!isWindows && !isInsideDirWithUnusualChars) {
493493
const dest = nextdir();
494494
mkdirSync(dest, mustNotMutateObjectDeep({ recursive: true }));
495495
writeFileSync(join(src, 'foo.txt'), 'foo', mustNotMutateObjectDeep({ mode: 0o444 }));
496+
// small wait to make sure that destStat.mtime.getTime() would produce a time
497+
// different from srcStat.mtime.getTime() if preserveTimestamps wasn't set to true
498+
await setTimeout(5);
496499
cpSync(src, dest, mustNotMutateObjectDeep({ preserveTimestamps: true, recursive: true }));
497500
assertDirEquivalent(src, dest);
498501
const srcStat = lstatSync(join(src, 'foo.txt'));

‎typings/internalBinding/fs.d.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ declare namespace InternalFSBinding {
7878

7979
function cpSyncCheckPaths(src: StringOrBuffer, dest: StringOrBuffer, dereference: boolean, recursive: boolean): void;
8080
function cpSyncOverrideFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, preserveTimestamps: boolean): void;
81+
function cpSyncCopyDir(src: StringOrBuffer, dest: StringOrBuffer, force: boolean, errorOnExist: boolean, verbatimSymlinks: boolean, dereference: boolean): void;
8182

8283
function fchmod(fd: number, mode: number, req: FSReqCallback): void;
8384
function fchmod(fd: number, mode: number): void;
@@ -262,6 +263,7 @@ export interface FsBinding {
262263
copyFile: typeof InternalFSBinding.copyFile;
263264
cpSyncCheckPaths: typeof InternalFSBinding.cpSyncCheckPaths;
264265
cpSyncOverrideFile: typeof InternalFSBinding.cpSyncOverrideFile;
266+
cpSyncCopyDir: typeof InternalFSBinding.cpSyncCopyDir;
265267
fchmod: typeof InternalFSBinding.fchmod;
266268
fchown: typeof InternalFSBinding.fchown;
267269
fdatasync: typeof InternalFSBinding.fdatasync;

0 commit comments

Comments
 (0)