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

std: Make file copy ops use zero-copy mechanisms #6516

Merged
merged 5 commits into from
Oct 9, 2020

Conversation

LemonBoy
Copy link
Contributor

@LemonBoy LemonBoy commented Oct 3, 2020

Use copy_file_range on Linux (if available), fcopyfile on Darwin,
sendfile on *BSDs (and on Linux kernels without copy_file_range).

Use copy_file_range on Linux (if available), fcopyfile on Darwin,
sendfile on *BSDs (and on Linux kernels without copy_file_range).
@alexnask alexnask added enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library. labels Oct 3, 2020
This is an optimization as it avoids an extra syscall, but it's also a
workaround for fstat being not available on Windows.
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.

Nice improvement, needs a bit more work before merging. lmk if you want a handoff

lib/std/os.zig Outdated
if (std.Target.current.os.tag == .linux) {
// Try copy_file_range first as that works at the FS level and is the
// most efficient method (if available).
if (has_copy_file_range_syscall.get() != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This should use the target version range to determine which state we are in:

  • the target OS version range guarantees kernel will have copy_file_range. Skip the runtime check.
  • the target OS version range guarantees kernel will not have copy_file_range. Skip the runtime check.
  • the target OS version range spans a kernel version that has the syscall and a version that does not have the syscall. Do the runtime check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, it's enough to preload has_copy_file_range_syscall with 0 if the version check fails.

lib/std/os.zig Outdated

/// Transfer all the data between two file descriptors in the most efficient way.
/// No metadata is transferred over.
pub fn copy_file(fd_in: fd_t, fd_out: fd_t, options: CopyFileOptions) CopyFileError!void {
Copy link
Member

Choose a reason for hiding this comment

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

What is the "posix layer" API function that this represents? It looks like the only one that has such a function is Darwin, in which case this should be called fcopyfile and not have an options parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no POSIX equivalent, we can just move this into fs as a private helper and call it a day.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good 👍

Copy link
Member

Choose a reason for hiding this comment

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

btw if you haven't seen it: #5019

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw if you haven't seen it: #5019

IMO we don't need a posix layer, we only need a std.os that does things (eg. copyFile, writeData) rather than trying to paper over all the differences between different OSs. If one wants a specific posix-specific/Windows-specific function they're more than welcome to do so by explicitly writing it out, but a full-blown Posix compatibility layer is not something that belongs into a stdlib.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that creating a full blown posix compatibility layer is not a goal in and of itself. But it's an implementation detail of providing a std that does things (e.g. std.fs). It's code that we would end up writing - in fact did end up writing - as an implementation detail of the higher level cross platform abstractions. "posix layer" is just a way to think about how it is organized.

lib/std/fs.zig Outdated
@@ -1823,7 +1823,7 @@ pub const Dir = struct {
var atomic_file = try dest_dir.atomicFile(dest_path, .{ .mode = mode });
defer atomic_file.deinit();

try atomic_file.file.writeFileAll(in_file, .{ .in_len = size });
try os.copy_file(in_file.handle, atomic_file.file.handle, .{});
Copy link
Member

Choose a reason for hiding this comment

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

The docs for in_len mention:

    /// If the size of the source file is known, passing the size here will save one syscall.
   in_len: ?u64 = null,

Did we gain a redundant fstat syscall in the case that sendfile has to be used? I know we've had this discussion before, and your position is that it doesn't matter in practice, but this is fundamental to zig's premise as a language and standard library, that it does the "optimal" interaction with the OS. I think it's an important sign of implementation quality for an strace / ProcMon session to contain only necessary syscalls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did we gain a redundant fstat syscall in the case that sendfile has to be used?

No, both copy_file_range and sendfile are asked to read/write as many bytes as possible at each iteration. This saves a stat() (that's done anyway if when the file mode is copied over, I'd expect copyFile to copy the timestamps too but that's a different topic)

@codehz
Copy link
Contributor

codehz commented Oct 5, 2020

What's about CopyFileW for windows?

@LemonBoy
Copy link
Contributor Author

LemonBoy commented Oct 5, 2020

What's about CopyFileW for windows?

It doesn't work on file handles so we're SOL. Is it any faster than a read/write loop?

Now it is a private API.
Also handle short writes in copy_file_range fallback implementation.
lib/std/fs.zig Outdated Show resolved Hide resolved
lib/std/os.zig Outdated
@@ -4978,6 +4979,11 @@ pub const CopyFileRangeError = error{
/// Other systems fall back to calling `pread` / `pwrite`.
///
/// Maximum offsets on Linux are `math.maxInt(i64)`.
var has_copy_file_range_syscall = init: {
const kernel_has_syscall = comptime std.Target.current.os.isAtLeast(.linux, .{ .major = 4, .minor = 5 }) orelse true;
break :init std.atomic.Int(u1).init(@boolToInt(kernel_has_syscall));
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a bool and accesing via @atomicLoad(bool, &has_copy_file_range_syscall) be simpler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I hate builtins and their @ so I'm using the posh abstraction layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was originally going to write that we should create std.atomic.Bool, but I revised my comment after I wrote an implementation, and realised it was no more than a wrapper around the builtins; do you think that would be preferable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my plan (I had std.atomic.Flag) in mind, but I discovered Int(bool) works just fine so... shrug

lib/std/os.zig Outdated
@@ -4978,25 +4979,26 @@ pub const CopyFileRangeError = error{
/// Other systems fall back to calling `pread` / `pwrite`.
///
/// Maximum offsets on Linux are `math.maxInt(i64)`.
var has_copy_file_range_syscall = init: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better name could be may_have_copy_file_range_syscall?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has false positives: it can be set to true even if the kernel doesn't have the syscall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial guess may be wrong, but once we execute copy_file_range the result reflects the kernel characteristics.

lib/std/os.zig Outdated Show resolved Hide resolved
lib/std/os.zig Outdated Show resolved Hide resolved
@daurnimator
Copy link
Contributor

Closes #2563

Comment on lines 26 to 28
pub const copyfile_state_t = *@Type(.Opaque);
pub extern "c" fn copyfile_state_alloc() copyfile_state_t;
pub extern "c" fn copyfile_state_free(state: copyfile_state_t) c_int;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but the alloc/free are now unused and can get the boot before/after merging.

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 the updated patch. It all looks quite clean. I only have a few nitpicks left.

lib/std/fs.zig Outdated Show resolved Hide resolved
lib/std/os.zig Outdated Show resolved Hide resolved
lib/std/os.zig Outdated Show resolved Hide resolved
lib/std/os.zig Outdated Show resolved Hide resolved
@LemonBoy
Copy link
Contributor Author

LemonBoy commented Oct 7, 2020

The more I work on os.zig the more my hate towards it grows, I don't care about this PR anymore so I'm handing it off.

@andrewrk
Copy link
Member

andrewrk commented Oct 7, 2020

OK no problem. Thanks!

Everybody respects your opinion around here, I'm sure if you open a proposal and explain how you want things to be different in a clear way it will be taken seriously.

@andrewrk andrewrk merged commit 76a1954 into ziglang:master Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants