uucore: add common splice-write functionality by DaringCuteSeal · Pull Request #6964 · uutils/coreutils

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

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

moonfruit mentioned this pull request Jan 20, 2025

uutils-selected 0.0.29 moonfruit/homebrew-tap#200

Closed

Uh oh!

Conversation

DaringCuteSeal commented Dec 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvestre Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 17, 2024

Uh oh!

DaringCuteSeal commented Dec 18, 2024

Uh oh!

sylvestre commented Dec 18, 2024

Uh oh!

DaringCuteSeal commented Dec 18, 2024

Uh oh!

DaringCuteSeal commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sylvestre commented Dec 18, 2024

Uh oh!

DaringCuteSeal commented Dec 18, 2024

Uh oh!

DaringCuteSeal commented Dec 18, 2024

Uh oh!

github-actions bot commented Dec 18, 2024

Uh oh!

sylvestre commented Dec 19, 2024

Uh oh!

DaringCuteSeal commented Dec 19, 2024

Uh oh!

DaringCuteSeal commented Dec 19, 2024

Uh oh!

github-actions bot commented Dec 19, 2024

Uh oh!

sylvestre commented Dec 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sylvestre Dec 17, 2024 •

edited

Loading

DaringCuteSeal commented Dec 18, 2024 •

edited

Loading