uucore: add common splice-write functionality #6964

DaringCuteSeal · 2024-12-17T13:18:51Z

Splice is a Linux-specific syscall that allows direct data copying from one file descriptor to another without user-space buffer. This can be used by several utilities to perform optimized copy under Linux and Android.

sylvestre · 2024-12-17T13:25:58Z

src/uucore/src/lib/features/splice.rs

+/// The `bool` in the result value indicates if we need to fall back to normal
+/// copying or not. False means we don't have to.
+#[inline]
+pub fn write_fast_using_splice<R: Read + AsFd + AsRawFd, S: AsRawFd + AsFd>(


would it be possible to add unit tests in this file ? thanks

sylvestre · 2024-12-17T13:26:11Z

src/uucore/src/lib/features/splice.rs

+#[cfg(any(target_os = "linux", target_os = "android"))]
+use crate::pipes::{pipe, splice, splice_exact};
+
+const SPLICE_SIZE: usize = 1024 * 128;


please document these "magic numbers"

damn not the author of this 😄 hmm

from here src/uu/cat/src/splice.rs ?

yeah. it's taken from there

ok, leave it like this then :)

I will have a look

sylvestre · 2024-12-17T13:26:45Z

src/uucore/src/lib/features/splice.rs

+
+/// Move exactly `num_bytes` bytes from `read_fd` to `write_fd`.
+///
+/// Panics if not enough bytes can be read.


not sure panic is the right thing to do here.
we probably want to return an error

sylvestre · 2024-12-17T13:28:05Z

src/uucore/src/lib/features/splice.rs

+pub fn write_fast_using_splice<R: Read + AsFd + AsRawFd, S: AsRawFd + AsFd>(
+    handle: &R,
+    write_fd: &S,
+) -> nix::Result<(usize, bool)> {


would it be possible to use UError https://docs.rs/uucore/0.0.28/uucore/error/trait.UError.html ?

github-actions · 2024-12-17T13:44:49Z

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

DaringCuteSeal · 2024-12-18T03:22:15Z

huh, i just realized that std::io::copy already uses splice when available, as documented by Rust:

On Linux (including Android), this function uses copy_file_range(2), sendfile(2) or splice(2) syscalls to move data directly between file descriptors if possible.

it is, however, subject to change. should we keep the reimplementation?

sylvestre · 2024-12-18T09:07:02Z

Interesting! did you do some benchmarking ?

DaringCuteSeal · 2024-12-18T09:17:10Z

nope, not yet. that's sounds interesting to do, i will see which one performs better

DaringCuteSeal · 2024-12-18T09:53:04Z

it seems like the reimplementation using splice wins here, at least on my linux machine

i did some tests on install with hyperfine to write from /dev/zero to a file:

dd if=/dev/zero of=/dev/stdout bs=1M count=1000 | ./install /dev/stdin "output_file"

Reimplementation, 100 runs:

Command	Mean [s]	Min [s]	Max [s]	Relative
`bash ./test_reimp.sh`	1.324 ± 0.500	0.828	3.253	1.00

using io::copy, 100 runs:

Command	Mean [s]	Min [s]	Max [s]	Relative
`bash ./test_iocopy.sh`	1.560 ± 0.467	1.173	3.427	1.00

could be related to the chunk size

sylvestre · 2024-12-18T10:16:58Z

you should use hyperfine for benchmarking :)

DaringCuteSeal · 2024-12-18T10:57:27Z

Oh yeah i did, or.. i'm supposed to just pass the commands to hyperfine directly?

DaringCuteSeal · 2024-12-18T11:59:13Z

tried more

reimplementation, 1G chunk, /dev/zero to file (100 runs):

Command	Mean [s]	Min [s]	Max [s]	Relative
`/bin/dd if=/dev/zero of=/dev/stdout bs=1G count=1 \| ./install /dev/stdin "output_file"`	1.413 ± 0.550	0.842	3.417	1.00

io::copy, 1G chunks, /dev/zero to file (100 runs)

Command	Mean [s]	Min [s]	Max [s]	Relative
`/bin/dd if=/dev/zero of=/dev/stdout bs=1G count=1 \| ./install-copy /dev/stdin "output_file"`	1.663 ± 0.452	1.198	3.619	1.00

reimplementation, 1G chunk, file to file (100 runs):

Command	Mean [s]	Min [s]	Max [s]	Relative
`/bin/dd if=test of=/dev/stdout bs=1G count=1 \| ./install /dev/stdin "output"`	1.146 ± 0.291	0.819	2.394	1.00

io::copy, 1G chunk size, file to file (100 runs):

Command	Mean [s]	Min [s]	Max [s]	Relative
`/bin/dd if=test of=/dev/stdout bs=1G count=1 \| ./install-copy /dev/stdin "output"`	1.634 ± 0.527	1.173	4.430	1.00

i took a look at the implementation of io::copy, seems like they used 0x7ffff000 (2,147,479,552 bytes) as the chunk size which is already the maximum.

set that to my splice and..

Command	Mean [s]	Min [s]	Max [s]	Relative
`/bin/dd if=test of=/dev/stdout bs=1G count=1 \| ./install /dev/stdin "output"`	1.207 ± 0.465	0.809	3.685	1.00

it's a biiit slower. But still faster than using io::copy.

i traced the syscalls made for both and both seem to be using splice similarly. i have no idea why our splice copy is faster, i may wanna dig more.

github-actions · 2024-12-18T12:41:33Z

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

sylvestre · 2024-12-19T10:48:06Z

could you please properly rebase your patches? the diff is showing unrelated changes
thanks

DaringCuteSeal · 2024-12-19T10:58:46Z

yeah yeah idk why i messed up the branch

Splice is a Linux-specific syscall that allows direct data copying from one file descriptor to another without user-space buffer. As of now, this is used by `cp`, `cat`, and `install` when compiled for Linux and Android.

DaringCuteSeal · 2024-12-19T11:34:06Z

ok there

github-actions · 2024-12-19T11:59:18Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

sylvestre · 2024-12-19T12:38:32Z

thanks

sylvestre reviewed Dec 17, 2024

View reviewed changes

DaringCuteSeal force-pushed the splice-uucore branch 3 times, most recently from 2bf5075 to 0d92ff6 Compare December 18, 2024 12:11

uucore: add common splice-write functionality

625c49d

Splice is a Linux-specific syscall that allows direct data copying from one file descriptor to another without user-space buffer. As of now, this is used by `cp`, `cat`, and `install` when compiled for Linux and Android.

DaringCuteSeal force-pushed the splice-uucore branch from 0d92ff6 to 625c49d Compare December 19, 2024 11:32

sylvestre merged commit 4341ae3 into uutils:main Dec 19, 2024
62 checks passed

This was referenced Dec 19, 2024

yes: use splice_data functions from uucore #6980

Closed

uucore/buf_copy: bug fixes for compatibility with uutils core utilities #6983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uucore: add common splice-write functionality #6964

uucore: add common splice-write functionality #6964

DaringCuteSeal commented Dec 17, 2024

sylvestre Dec 17, 2024

DaringCuteSeal Dec 17, 2024

sylvestre Dec 17, 2024

DaringCuteSeal Dec 17, 2024

sylvestre Dec 17, 2024 •

edited

Loading

DaringCuteSeal Dec 17, 2024

sylvestre Dec 17, 2024

DaringCuteSeal Dec 17, 2024

sylvestre Dec 17, 2024

sylvestre Dec 17, 2024

github-actions bot commented Dec 17, 2024

DaringCuteSeal commented Dec 18, 2024

sylvestre commented Dec 18, 2024

DaringCuteSeal commented Dec 18, 2024

DaringCuteSeal commented Dec 18, 2024 •

edited

Loading

sylvestre commented Dec 18, 2024

DaringCuteSeal commented Dec 18, 2024

DaringCuteSeal commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

sylvestre commented Dec 19, 2024

DaringCuteSeal commented Dec 19, 2024

DaringCuteSeal commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

sylvestre commented Dec 19, 2024

uucore: add common splice-write functionality #6964

uucore: add common splice-write functionality #6964

Conversation

DaringCuteSeal commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvestre Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2024

DaringCuteSeal commented Dec 18, 2024

sylvestre commented Dec 18, 2024

DaringCuteSeal commented Dec 18, 2024

DaringCuteSeal commented Dec 18, 2024 • edited Loading

sylvestre commented Dec 18, 2024

DaringCuteSeal commented Dec 18, 2024

DaringCuteSeal commented Dec 18, 2024

github-actions bot commented Dec 18, 2024

sylvestre commented Dec 19, 2024

DaringCuteSeal commented Dec 19, 2024

DaringCuteSeal commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

sylvestre commented Dec 19, 2024

sylvestre Dec 17, 2024 •

edited

Loading

DaringCuteSeal commented Dec 18, 2024 •

edited

Loading