cp: implement copying from streams #7061
Conversation
|
GNU testsuite comparison: |
DaringCuteSeal
force-pushed
the
cp-stream-2
branch
from
3b9a0c3 to
8348260
Compare
|
GNU testsuite comparison: |
DaringCuteSeal
force-pushed
the
cp-stream-2
branch
4 times, most recently
from
9d4bc72 to
ac614da
Compare
|
GNU testsuite comparison: |
DaringCuteSeal
force-pushed
the
cp-stream-2
branch
from
ac614da to
1bf9fad
Compare
|
GNU testsuite comparison: |
| @@ -3437,15 +3437,9 @@ fn test_same_file_force_backup() { | |||
| } | |||
|
|
|||
| /// Test for copying the contents of a FIFO as opposed to the FIFO object itself. | |||
| #[cfg(all(unix, not(target_os = "freebsd"), not(target_os = "openbsd")))] | |||
| #[cfg(unix)] | |||
| @@ -1953,6 +1953,7 @@ fn print_paths(parents: bool, source: &Path, dest: &Path) { | |||
| /// | |||
| /// * `Ok(())` - The file was copied successfully. | |||
| /// * `Err(CopyError)` - An error occurred while copying the file. | |||
| #[allow(clippy::too_many_arguments)] | |||
There was a problem hiding this comment.
it should probably be refactor (this is fine in a separate PR)
There was a problem hiding this comment.
oh nah. I added this for the sake of passing the style check since I added a new argument below there and it triggered a clippy warning. I don't think I can separate it :P
or should I?
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
did you run the benchmarks to make sure it didn't regress ? |
Oh, no, wait you mean regression for the non-special files to make sure I didn't mess that up right? |
|
for some reason my modification actually made it slightly faster in these runs but it's likely to also be influenced by my hard drive and other random factors both are compiled with With multiple files (50 runs, 3x warm-up)
With single file (50 runs, 3x warm-up)
|
Previously, the only stream that cp supports copying are FIFOs. Any other files will be handled using std::io::copy which does not handle special files like block devices or character specials. This commit fixes the previously missing support by using std::io::copy or splice syscall for Linux to handle those cases.
Copying FIFOs now work under FreeBSD and other UNIX platforms.
DaringCuteSeal
force-pushed
the
cp-stream-2
branch
3 times, most recently
from
908e155 to
6402672
Compare
DaringCuteSeal
force-pushed
the
cp-stream-2
branch
from
6402672 to
c6d1923
Compare
|
Could you please just export the hyperfine --export-markdown output ? |
sure |
|
@DaringCuteSeal not what i meant, sorry the goal is to have an easy way to compare results |
wait ha you mean just run im sure the reports above are from hyperfine |
|
sorry, github eat my message: |
ohhh yeah i did see that message but after i re-read everything i can't find that anymore |
|
@DaringCuteSeal btw, do you know why we don't use splice in this case #7092 ? |
splice only works for pipes sources and not regular files for regular files we use
which seems to already use all optimizations possible with that issue in particular it's probably something wrong with our loops, considering the amount of files the user reported copying. |
|
@sylvestre for the benchmark btw |
|
it regressed the performances ?! surprising |
|
actually yeah thats weird i wonder if the order of operation matters or is it the |
|
wait no it actually did regress it :| > hyperfine --warmup 3 --show-output "bash -c '/home/mynameisasecret/cp-new --update=all -r /home/mynameisasecret/test-dir/ /home/mynameisasecret/test-dir-2'" "bash -c '/home/mynameisasecret/cp-old --update=all -r /home/mynameisasecret/test-dir/ /home/mynameisasecret/test-dir-2'" waddaya say then @sylvestre |
|
sorry but what means waddaya ? |
what do you* |
|
maybe run samply to investigate the difference in term of execution |
|
sure thing |
|
hey @sylvestre i tried profiling the also i tried running more hyperfine with ~1.05 difference ratio doesn't sound too bad for me. |
|
yeah, 1.05 is probably noise |
Description
Previously, the only stream that
cpsupports copying are FIFOs. Any other files will be handled usingstd::io::copywhich does not handle unix "special" files, such as block devices and character specials. This PR addresses the previously missing support by usingstd::io::copyor splice syscall (under Linux) provided from thebuf-copymodule from uucore.There are also several minor fixes introduced alongside:
/dev/nullnow works no matter the file path. Previously,cponly checks if the argument passed is a string of "/dev/null".Current Minor Issue
The
copy_streamfrom uucore returns aUResultwhich means that the caller cannot know the kind of error it returns directly. However,cp's stream copy function does not directly return aUResultas of now (it returns astd::io::Error), making the module plug slightly wonky. In this PR, I tried to map the UResult Error into astd::io::Errorwith theOtherkind which doesn't sound right. A better fix can be done in a separate refactor, please let me know if I can (and should) work on that.