Skip to content
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

os.copy_file_range: save a syscall for most operations #12491

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

motiejus
Copy link
Contributor

Currenty copy_file_range always uses at least two syscalls:

  1. As many as it needs to do the initial copy (always 1 during my
    testing)
  2. The last one is always when offset is the size of the file.

The second syscall is used to detect the terminating condition. However,
because we do a stat for other reasons, we know the size of the file,
and we can skip the syscall.

Sparse files: since copy_file_range expands holes of sparse files, I
conclude that this layer was not intended to work with sparse files. In
other words, this commit does not make it worse for sparse file society.

Test program

const std = @import("std");

pub fn main() !void {
    const arg1 = std.mem.span(std.os.argv[1]);
    const arg2 = std.mem.span(std.os.argv[2]);
    try std.fs.cwd().copyFile(arg1, std.fs.cwd(), arg2, .{});
}

Test output (current master)

Observe two copy_file_range syscalls: one with 209 bytes, one with
zero:

$ zig build-exe cp.zig
$ strace ./cp ./cp.zig ./cp2.zig |& grep copy_file_range
copy_file_range(3, [0], 5, [0], 4294967295, 0) = 209
copy_file_range(3, [209], 5, [209], 4294967295, 0) = 0
$

Test output (this diff)

Observe a single copy_file_range syscall with 209 bytes:

$ /code/zig/build/zig build-exe cp.zig
$ strace ./cp ./cp.zig ./cp2.zig |& grep copy_file_range
copy_file_range(3, [0], 5, [0], 4294967295, 0) = 209
$

@motiejus motiejus force-pushed the copy_file_range_syscalls branch 2 times, most recently from 49669ee to fe1391d Compare August 23, 2022 15:04
@andrewrk andrewrk force-pushed the copy_file_range_syscalls branch from fe1391d to 90ce446 Compare September 1, 2022 01:49
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience on this review. With this one thing addressed, this can be merged-

@motiejus motiejus force-pushed the copy_file_range_syscalls branch 2 times, most recently from c5d644e to 742b816 Compare October 16, 2022 09:45
Currenty copy_file_range always uses at least two syscalls:

1. As many as it needs to do the initial copy (always 1 during my
   testing)
2. The last one is always when offset is the size of the file.

The second syscall is used to detect the terminating condition. However,
because we do a stat for other reasons, we know the size of the file,
and we can skip the syscall.

Sparse files: since copy_file_range expands holes of sparse files, I
conclude that this layer was not intended to work with sparse files. In
other words, this commit does not make it worse for sparse file society.

Test program
------------

    const std = @import("std");

    pub fn main() !void {
        const arg1 = std.mem.span(std.os.argv[1]);
        const arg2 = std.mem.span(std.os.argv[2]);
        try std.fs.cwd().copyFile(arg1, std.fs.cwd(), arg2, .{});
    }

Test output (current master)
----------------------------

Observe two `copy_file_range` syscalls: one with 209 bytes, one with
zero:

    $ zig build-exe cp.zig
    $ strace ./cp ./cp.zig ./cp2.zig |& grep copy_file_range
    copy_file_range(3, [0], 5, [0], 4294967295, 0) = 209
    copy_file_range(3, [209], 5, [209], 4294967295, 0) = 0
    $

Test output (this diff)
-----------------------

Observe a single `copy_file_range` syscall with 209 bytes:

    $ /code/zig/build/zig build-exe cp.zig
    $ strace ./cp ./cp.zig ./cp2.zig |& grep copy_file_range
    copy_file_range(3, [0], 5, [0], 4294967295, 0) = 209
    $
@motiejus motiejus force-pushed the copy_file_range_syscalls branch from 742b816 to 5c76d4e Compare October 16, 2022 09:46
@motiejus motiejus requested a review from andrewrk October 16, 2022 09:46
@andrewrk andrewrk merged commit fd10baf into ziglang:master Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants