-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
fs: improve cpSync
performance
#53541
base: main
Are you sure you want to change the base?
Conversation
f7f3d9b
to
aab2a36
Compare
aab2a36
to
31286c9
Compare
31286c9
to
5e52125
Compare
cc @nodejs/performance @nodejs/fs @nodejs/cpp-reviewers |
e3e4afc
to
ea737f7
Compare
cc @nodejs/tsc this pull-request is now a
I appreciate if you could share your comments and thoughts on this. |
ea737f7
to
a859abd
Compare
a859abd
to
4f7d9d4
Compare
I'm okay with those changes as semver-major. I would be -1 on any sort of exception. |
I'm not sure, can't we re-map the thrown errors to the existing codes (e.g. in a catch, even if it's hard/degrates performance in C++ land (why?))? Wouldn't that only hurt the performance in the error case which isn't the interesting one? I'm wondering if our users for APIs like cpSync that are less performance sensitive (since they're blocking) don't care more breaking changes than a perf gain. (Just to be extra explicit - I'm not blocking this PR) |
Wait actually reading the code I think I understand since std::filesystem doesn't distinguish between those cases. |
These specific errors can only be caught if we traverse the filesystem one by one, and make the necessary copy operations. Since we don't do it, there is no way to know if the EEXIST was thrown from a symlink in src, or a symlink in destination that looks into a path in destination folder. The test cases are same, but they throw different errors.
We can catch it, but the problem is, we don't know which file is causing this error. |
Thanks, I'm +1 regarding the change and the breakage since I doubt people using cpSync are checking error codes this granularly. We may want to be prudent and trigger a CITGM run but I don't expect it'll find much. (It should still be semver major though) |
@benjamingr Actually, there is a |
I love systems engineering. We can't use try/catch because we are passing
|
That's for the overload that doesn't contain the error_code - and has the same info as the error_code so if it isn't there it doesn't distinguish between those cases. (Also CI should tell us that std::filesystem::copy gives these errors in a cross-platform friendly way) |
Even with that overloaded function, the following code does not compile due to disabled exception handling @benjamingr
|
@anonrig you misunderstand me, sorry - I mean If you do that, it doesn't throw and returns the error code through the parameter instead. If you remove that parameter it will throw. (There are valid important reasons why exceptions are disabled in the codebase, but feel free to disable that for testing) |
Yes, unfortunately std::error_code does not have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent. Im in favor of this remaining a major change. Great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with green CI. Since the implementation was adjusted, benchmark rerun would be appreciated.
@@ -218,7 +218,7 @@ function nextdir() { | |||
assert.throws( | |||
() => cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true })), | |||
{ | |||
code: 'ERR_FS_CP_EINVAL' | |||
code: 'ERR_FS_CP_EEXIST' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code: 'ERR_FS_CP_EEXIST' | |
code: 'ERR_FS_CP_EINVAL' |
AFAICT if symlink in src points to location in dest, cp
will throw EINVAL
due to presumably attempting of copying directory to its subdirectory. This might be a bug in the subdirectory checking implementation (isSrcSubdir(resolvedSrc, resolvedDest)
).
Directory structure before copying:
├── copy_15
│ └── link -> node/test/.tmp.0/copy_16
├── copy_16
│ └── link -> node/test/.tmp.0/copy_16
Directory structure after performing regular cp -r node/test/.tmp.0/copy_15 node/test/.tmp.0/copy_16
:
├── copy_15
│ └── link -> node/test/.tmp.0/copy_16
├── copy_16
│ ├── copy_15
│ │ └── link -> node/test/.tmp.0/copy_16
│ └── link -> node/test/.tmp.0/copy_16
It's not really related to this particular PR, but i guess creating cyclic symlink like this should be allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is nice enough. More detailed errors could be provided as part of a follow-up PR.
4f7d9d4
to
8b0f25f
Compare
@LiviaMedeiros benchmark ci: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1572/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with these changes as semver major. -1 on back porting.
I'm a little afraid of having a branch with two completely separate implementations of the feature depending on the options passed by the user. This LGTM if the end goal is to get rid of the JS one. |
copy_options options = copy_options::copy_symlinks; | ||
|
||
// When true timestamps from src will be preserved. | ||
if (preserveTimestamps) options |= copy_options::create_hard_links; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct?
afaict, this will create hard links in the destination directory to the original files. so, while it will appear upon inspecting the filesystem that the files have been copied and the timestamps have been preserved, the files will not have actually been copied, just linked as directory entries in the filesystem to the same file.
this means if you change any of the files in the source directory subsequently, the "files" in the destination will also change. i haven't looked at existing code in detail but am pretty sure this is not how it works with this flag.
i am also guessing without checking that this will only work if source and destination are on same filesystem? 🤔
if i am indeed correct, i guess simplest thing to do for now would be to use the slow path if "preserveTimestamps" is set?
from a quick google, i'm not sure there is an efficient way to copy and preserve timestamps without having to touch each file in a syscall and it just doesn't seem that there is an option rn in std::filesystem to emulate the current behaviour. happy to be wrong though - not a C++ expert.
https://en.cppreference.com/w/cpp/filesystem/copy_options
https://en.wikipedia.org/wiki/Hard_link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i put some benches using the test/fixtures/copy directory from node.js and the same options as above here if anyone wants to try them out - should be easy to get them working on linux or macos.
https://github.com/just-js/lo-bench/blob/main/fs/README.md
some things to note:
on linux core i5, using /dev/shm to reduce filesystem/disk overhead
- bun is roughly 4x the performance of current node.js 22.3
- a "zero-overhead" implementation on v8, using lo runtime and v8 fastcalls to call std::filesystem::copy is ~2.5x the performance of current node - can assume this would be the theoretical max of node.js with this change merged i think.
- bun is still roughly 1.5x the optimal v8 implementation using filesystem::copy
on macos m1 mini, using a ram disk
- bun is rougly 2.5x performance of current node.js 22.3
- the "optimal" v8 implementation using fileystem::copy is ~1.3x node.js 22.3
- bun is still roughly 2x better than "optimal" v8 using filesystem::copy
also, for the same workload:
- node does ~90k syscalls
- bun does ~55k
- std::filesystem::copy does ~65k
so, without testing with other directory structures and sizes/depths but assuming we see similar results, we could assert the following:
- this change will still be significantly slower than bun
- there is still no good solution for getting individual file errors back, which seems necessary
- it can't be used when we need to preserve timestamps (see above)
- imo, the existing implementation could probably be changed to not make as many unnecessary syscalls, remain in JS and be within a couple of percentage points of throughput of the bun implementation. this might be a better long term approach?
another couple of things to note
- if we use force: false then the difference between node.js and bun is much smaller
- the bigger/deeper the directory is, the smaller the difference will be between node and bun as the throughput is syscall heavy.
- the difference will probably be less again on an actual filesystem. i just used ramdisk in the benches to eliminate that overhead and get a clearer picture of runtime overhead.
let me know if i got anything wrong - i ran through this pretty quickly, but it should be easy to reproduce those results using the link above. 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
For now, I'll split this pull-request into multiple changes, and try to optimize the existing implementation before moving it into full C++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a different approach: #53614
I'm extremely open to suggestions on improving the benchmarks, but with the current state, here are the results:
local benchmarks
macOS M1 Max
benchmark ci
benchmark ci: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1572/