Clone files on Darwin rather than copying them #2823

rnro · 2024-08-02T15:33:52Z

Motivation:

Copying regular files on Darwin was taking more time than it needed to because it was manually opening files and copying over bites rather than making use of the system's ability to create CoW clones.

Modifications:

The primary change is to switch copyItem to be backed by Darwin's copyfile method rather than fcopyfile. fcopyfile ignores the flag we were passing in to indicate that the file should be cloned if possible, whereas copyfile is a higher-level API which takes this into account.

This change exposes a copyfile wrapper method as a new public function.

This change also adds a workaround to a seeming compiler bug on channel options blocking building tests on more recent Xcodes.

Result:

Copying files on Darwin will be faster.

glbrntt

Looks great, thanks for fixing this Rick. The actual fix looks good but I left a couple of nitty things to fix up.

glbrntt · 2024-08-02T15:44:32Z

Tests/NIOPosixTests/StreamChannelsTest.swift

+            let opt = try? receiver.getOption(.socketOption(.so_rcvbuf)).wait()
+            let receiveBufferSize = Int(opt ?? 8192)


Can you do this in separate PR?

glbrntt · 2024-08-02T15:46:42Z

Sources/NIOFileSystem/FileSystem.swift

+            state: nil,
+            flags: flags
+        ).mapError { errno in
+            FileSystemError.fcopyfile(


Can you update this helper? One of the things it'll do is attach a SyscallError to the FileSystemError which will include the name of the syscall, now it will incorrectly be fcopyfile.

Where it doesn't make sense to duplicate the helper I've tended to just pass in the name of the syscall: FileSystemError.copyfile("copyfile", ... )

glbrntt · 2024-08-02T15:48:39Z

Sources/NIOFileSystem/FileSystemError+Syscall.swift

@@ -1016,6 +1016,25 @@ extension FileSystemError {
                Can't copy file from '\(sourcePath)' to '\(destinationPath)', the destination \
                path already exists.
                """
+        case .fileExists:


Can you update the error tests? They check that errnos get mapped to an appropriate file system error code.

glbrntt · 2024-08-02T15:50:02Z

Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift

@@ -400,7 +400,7 @@ internal func libc_remove(
 }

 #if canImport(Darwin)
-/// copyfile(3): Copy a file from one file to another.
+/// copyfile(3): Copy a file from one file to another. (fcopyfile)


nit

Suggested change

/// copyfile(3): Copy a file from one file to another. (fcopyfile)

/// fcopyfile(3): Copy a file from one file to another.

glbrntt · 2024-08-02T15:53:17Z

Sources/NIOFileSystem/Internal/System Calls/Syscall.swift

@@ -313,6 +313,24 @@ public enum Libc {
    }
    #endif

+    #if canImport(Darwin)
+    @_spi(Testing)
+    public static func copyfile(


Can you add a test for this please?

swift-nio/Tests/NIOFileSystemTests/Internal/SyscallTests.swift

Line 376 in ba62457

func test_fcopyfile() throws {

?

Motivation: Copying regular files on Darwin was taking more time than it needed to because it was manually opening files and copying over bites rather than making use of the system's ability to create CoW clones. Modifications: The primary change is to switch `copyItem` to be backed by Darwin's `copyfile` method rather than `fcopyfile`. `fcopyfile` ignores the flag we were passing in to indicate that the file should be cloned if possible, whereas `copyfile` is a higher-level API which takes this into account. This change exposes a `copyfile` wrapper method as a new public function. This change also adds a workaround to a seeming compiler bug on channel options blocking building tests on more recent Xcodes. Result: Copying files on Darwin will be faster.

glbrntt

Great patch, thanks @rnro!

rnro requested a review from glbrntt August 2, 2024 15:33

rnro force-pushed the copy_file_on_darwin_use_clone branch from 5901179 to adeb5b6 Compare August 2, 2024 15:41

glbrntt requested changes Aug 2, 2024

View reviewed changes

rnro added 2 commits August 5, 2024 10:49

add FileSystemError.copyfile

6eb5833

rnro force-pushed the copy_file_on_darwin_use_clone branch from aa683ab to 6eb5833 Compare August 5, 2024 10:26

rnro requested a review from glbrntt August 5, 2024 10:26

glbrntt approved these changes Aug 5, 2024

View reviewed changes

glbrntt merged commit bd12191 into apple:main Aug 5, 2024
27 of 28 checks passed

glbrntt added the 🔨 semver/patch No public API change. label Aug 5, 2024

rnro deleted the copy_file_on_darwin_use_clone branch August 5, 2024 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clone files on Darwin rather than copying them #2823

Clone files on Darwin rather than copying them #2823

rnro commented Aug 2, 2024

glbrntt left a comment

glbrntt Aug 2, 2024

rnro Aug 5, 2024

glbrntt Aug 2, 2024

glbrntt Aug 2, 2024

glbrntt Aug 2, 2024

glbrntt Aug 2, 2024

glbrntt left a comment

		let opt = try? receiver.getOption(.socketOption(.so_rcvbuf)).wait()
		let receiveBufferSize = Int(opt ?? 8192)

	/// copyfile(3): Copy a file from one file to another. (fcopyfile)
	/// fcopyfile(3): Copy a file from one file to another.

Clone files on Darwin rather than copying them #2823

Clone files on Darwin rather than copying them #2823

Conversation

rnro commented Aug 2, 2024

Motivation:

Modifications:

Result:

glbrntt left a comment

Choose a reason for hiding this comment

glbrntt Aug 2, 2024

Choose a reason for hiding this comment

rnro Aug 5, 2024

Choose a reason for hiding this comment

glbrntt Aug 2, 2024

Choose a reason for hiding this comment

glbrntt Aug 2, 2024

Choose a reason for hiding this comment

glbrntt Aug 2, 2024

Choose a reason for hiding this comment

glbrntt Aug 2, 2024

Choose a reason for hiding this comment

glbrntt left a comment

Choose a reason for hiding this comment