perf(ext/node): move most fs.cp logic to rust#32580
Conversation
kajukitli
left a comment
There was a problem hiding this comment.
impressive perf improvements — 2x faster on linux, 4x on macos is significant
a few observations:
the good:
- batching sync syscalls in
spawn_blockingis the right approach - error type mapping via
CpErrorenum is clean - keeping the filter callback in JS (since it's user-provided) makes sense
questions/concerns:
todo!()inJoinErrorhandling — line ~62 in fs.rs:
if err.is_cancelled() {
todo!("async tasks must not be cancelled")
}this will panic if a task is cancelled. is that intentional? might want a proper error type instead.
unwrap_or_default()on path strings — several places like:
resolved_src_buf.to_str().unwrap_or_default().to_string()if a path contains invalid UTF-8, this silently becomes empty string. might be worth logging or erroring instead.
-
symlink race condition — in
cp_create_symlink, if dest exists it's removed then symlink is created. between these two ops another process could create something at that path. the original JS had the same issue, so not a regression, just noting it. -
#[property]on errors — theNodeFsErrorstruct uses#[property]attributes which is cool. is that a deno_error macro? might want to document it since it's not standard thiserror. -
missing
cp_syncoptimization — this PR only optimizes the async path. the sync version incp_sync.tsstill uses the old JS implementation. intentional? might be worth a follow-up.
minor:
- the
CpErrorenum'skind(),message(), andpath()methods all match on every variant — could use a macro to reduce repetition
overall this looks good, the benchmark numbers speak for themselves.
8bb9e4d to
88ecff3
Compare
|
I also realized that running Is it okay if I adjust the Lines 16 to 17 in 532b307 |
We've parsed the options before using the `validateCpOptions`
…ons (#32927) ## Summary - Fix `node:fs.cp` failing with `ENOENT` when copying between allowed directories while read permissions outside those directories are ignored (`"ignore": true` in deno.json permissions config) - The `check_parent_paths` walk-up now handles `NotFound` from permission checks (ignored denials) the same way it handles `NotFound` from `stat` by stopping the ancestor traversal **Root cause**: When `fs.cp` was moved from JS to Rust (#32580, #32687), the parent path validation started propagating ignored-permission `ENOENT` as a hard error instead of treating it as a stop signal. The old JS code's `safeStatFn` caught `NotFound` and returned `undefined`, gracefully stopping the walk. The Rust code's `check_cp_path()?` propagated it before reaching the stat call's own `NotFound` handler. Closes #32923 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The idea is to move most of the logic from the JS side to Rust without changing the behavior. Not all functions are moved, for example since users can pass a function to filter which files get copied, it's much easier to just
awaitit on the JS side. I also grouped somesyncsyscalls together and execute them in a tokio blocking thread, that in theory should be more efficient than scheduling many smallasyncsyscalls.Performance insights based on my benchmark:
Linux
Deno 2.7.4
Deno (This PR)
Node.js 25.8.0
Bun 1.3.10
macOS
Deno 2.7.4
Deno (This PR)
Node.js 25.8.0