FileSystem.copyItem can parallelise directory copy #2806

UncleMattHope · 2024-07-24T18:26:09Z

FileSystem.copyItem defaults to perform directory copies in parallel

Motivation:

Using some limited parallelism can improved the speed of the copy for very limited increase in cpu use.

In circumstances where the source and/or destination are on a high latency device, such as a network file system,
the resulting speed up can be even more significant.
Providing conservative defaults, while allowing users the ability to control it directly if desired, provides for a faster default and access to user targeted gains in performance.
The use of parallel by default is considered in keeping with the swift-nio model of concurrent capabilities.

The parallelism used is restricted because the underlying limited resource for this is highly likely to be limited system file descriptors, rather than the limits of the underlying executor itself, especially on embedded devices with significantly more limited concurrent descriptors.

Modifications:

All copyItem overloads except copyItem(sourcePath:destinationPath:shouldProceedAfterError:shouldCopyFile)
can, and likely will, use concurrent copies when copying directories.

A Newly type CopyStrategy controls the behaviour, allowing:

parallel(maxDescriptors: Int)
- Parallel limited to a maximum number of concurrently open file descriptors.
sequential
- The previous behaviour, still asynchronous but only one operation at a time
platformDefault
- Whatever the API determines the default should be from the prior options

The cancellation semantics of copyItem were previously unclear/undocumented.
These are now stated and cancellation is tested.

There were some aspects of naming/types on this API which were inconsistent/unclear. Those have been altered to aid clarity.

Less duplication with the protocol docs
Consistent use of item not file
Use DirectoryEntry for the source in both shouldXxx callbacks

NIOFileSystem package status remains unchanged, it is still _NIOFileSystem with the semantics that entails.

Result:

For any use of the following functions on FileSystemProtocol, when backed by FileSystem:

`copyItem(*)`:

The shouldCopyItem check happens before the internal FileSystem "do I know how to copy this sort of FileType" check. This makes the code a little simpler, but the desire is to allow consuming code to filter out anything unexpected if they like without needing to trap errors.

`copyItem(sourcePath:destinationPath)`:

Depending on the platform this will become parallel, users on MacOS, Linux and Windows will have up to 4 concurrent file system operations at once.
Empirical testing on a MacBook pro with a significant dense directory hierarchy of 12 GiB had performance improvements of ~50% reduction in elapsed time for CPU usage increases below the noise floor of the testing

`copyItem(sourcePath:destinationPath:shouldProceedAfterError:shouldCopyFile)`:

When migrating from this to copyItem(sourcePath:destinationPath:copyStrategy:shouldProceedAfterError:shouldCopyItem)care should be taken to ensure the should callbacks are safe to use in different concurrency domains, or that CopyStrategy.sequential is explicitly stated.
The backward compatibility shim provided does this by default.

Configurable via CopyStrategy. - Default on known platforms is to be parallel. Cancellation semantics documented and tested. Backward compatibility shim for old copyItem. Fixed inconsistency with use of DirectoryEntry for the source on shouldXxx callbacks Some comment improvements: - Less duplication with the protocol - Consistent use of `item` not 'file' - Tweak a comment on default threads in thread pool that was wrong and confused me This has defaults for concurrency based on limited empirical testing on MacOS assumed to likely apply to other similar targets. Embedded platforms have more conservative limits. Unanticipated platforms remain sequential. Semantic Changes copyItem(*): shouldCopyItem check happens before the FileSystem type check. This makes the code a little simpler, and also allows consuming code to filter out anything unexpected if they like without needing to trap errors. copyItem(sourcePath:destinationPath): Depending on the platform it will become parallel When migrating from shouldProceedAfterError/shouldCopyFile to shouldProceedAfterError/shouldCopyItem care should be taken to ensure they are safe to use in different concurrency domains, or that CopyStrategy.sequential is explicitly stated. The backward compatibility shim provided does this be default.

glbrntt · 2024-07-25T10:06:50Z

Sources/NIOFileSystem/CopyStrategy.swift

+//===----------------------------------------------------------------------===//
+
+/// How to perform copies. Currently only relevant to directory level copies when using
+/// ``FileSystemProtocol.copyItem``


I think this needs to be (the docs CI will likely complain at you anyway):

Suggested change

/// ``FileSystemProtocol.copyItem``

/// ``FileSystemProtocol/copyItem``

Sources/NIOFileSystem/CopyStrategy.swift

Sources/NIOFileSystem/FileSystem.swift

glbrntt · 2024-07-25T12:16:49Z

Sources/NIOFileSystem/FileSystemProtocol.swift

+ ///
+ /// #### Errors
+ ///
+ /// No errors should be throw by implementors without first calling ``shouldProceedAfterError``,


Suggested change

/// No errors should be throw by implementors without first calling ``shouldProceedAfterError``,

/// No errors should be thrown by implementors without first calling ``shouldProceedAfterError``,

Sources/NIOFileSystem/Internal/ParallelDirCopy.swift

glbrntt · 2024-07-25T12:32:41Z

Sources/NIOFileSystem/CopyStrategy.swift

+ ///
+ /// This is primarily exposed for testing to ensure use of the parallel paths (which are more complex) while keeping actual
+ /// parallelism to minimal levels to make debugging simpler.
+ public static let minimalParallel: Self = .init(.parallel(minDescriptorsAllowed))


Is this useful outside of our own tests?

I found writing tests using .parallel(maxDescriptors:2) to be painful since that call declares throws, necessitating lots of cascading the throwing capability or wrapping it in an ugly try/catch.

It seemed reasonable that other people testing code using this might want to make their tests follow the parallel path via .platformDefault, but have the option to switch to the limited parallelism easily without having to mess about at the CopyStrategy declaration point in their tests.

Happy to hide it and just push it into our tests if you prefer.

I don't feel too strongly here but my lean is to hide it from the public API, as you note using it outside of testing is an anti-pattern, and I think it's unlikely that users will also need this in their tests. If they do they can try! parallel(maxDescriptors: 2) (which should be safe per our docs).

I agree with this. I don't see why any user would want to set the minimum parallelism

cool, I'll move it to the testing

Get fixes to the workflow check scripts

pull in docs-check fix for local testing

Co-authored-by: George Barnett <gbarnett@apple.com>

FranzBusch · 2024-07-26T10:59:02Z

Sources/NIOFileSystem/CopyStrategy.swift

+ ///
+ /// This is primarily exposed for testing to ensure use of the parallel paths (which are more complex) while keeping actual
+ /// parallelism to minimal levels to make debugging simpler.
+ public static let minimalParallel: Self = .init(.parallel(minDescriptorsAllowed))


I agree with this. I don't see why any user would want to set the minimum parallelism

Sources/NIOFileSystem/FileSystem.swift

Sources/NIOFileSystem/Internal/ParallelDirCopy.swift

FranzBusch · 2024-07-26T11:04:57Z

Sources/NIOFileSystem/Internal/ParallelDirCopy.swift

+ // We haven't started monitoring for task completion, so inProgress is 'worst case'.
+ var inProgress = 0
+ while inProgress <= maxConcurrentOperations {
+ try Task.checkCancellation()


We are calling iter.next() on the next line which will check for cancellation when needed

FranzBusch · 2024-07-26T11:05:09Z

Sources/NIOFileSystem/Internal/ParallelDirCopy.swift

+ while let _ = try await taskGroup.next() {
+ var keepConsuming = true
+ while keepConsuming {
+ try Task.checkCancellation()


Co-authored-by: Franz Busch <privat@franz-busch.de>

glbrntt

This LGTM, thanks @UncleMattHope!

UncleMattHope · 2024-07-26T17:59:51Z

Sources/NIOFileSystem/Internal/ParallelDirCopy.swift

+ inProgress += 1
+ }
+ } else {
+ // We completed things before we hit the limit


This was a bug, that made my cancellation test flaky if it happened to trigger during this phase.
I have a fix for that and will push shortly.

The cancellation propagates as the sequence cleanly terminating, so our task just completes early, rather than resulting an a cancellation.
simply inserting a try Task.checkCancellation() here is sufficient, but I have also streamlined the test side to be faster too

These made the cancellation test flaky

UncleMattHope · 2024-07-29T09:14:40Z

Confirmation of acceptance of API break in _NIOFileSystem

5 breaking changes detected in _NIOFileSystem:
💔 API breakage: func FileSystem.copyItem(at:to:shouldProceedAfterError:shouldCopyFile:) has parameter 2 type change from (_NIOFileSystem.DirectoryEntry, any Swift.Error) async throws -> Swift.Void to _NIOFileSystem.CopyStrategy
💔 API breakage: func FileSystem.copyItem(at:to:shouldProceedAfterError:shouldCopyFile:) has parameter 3 type change from (SystemPackage.FilePath, SystemPackage.FilePath) async -> Swift.Bool to (_NIOFileSystem.DirectoryEntry, any Swift.Error) async throws -> Swift.Void
💔 API breakage: func FileSystemProtocol.copyItem(at:to:strategy:shouldProceedAfterError:shouldCopyItem:) has been added as a protocol requirement
💔 API breakage: func FileSystem.copyItem(at:to:shouldProceedAfterError:shouldCopyFile:) has been renamed to func copyItem(at:to:strategy:shouldProceedAfterError:shouldCopyItem:)
💔 API breakage: func FileSystemProtocol.copyItem(at:to:) has been renamed to func copyItem(at:to:strategy:)

Since NIOFileSystem is still in leading underscore mode this is deemed acceptable (confirmed in discussions offline)

In most cases the backward compatibility shim provided in FileSystemProtocol will cover it.
In cases where a user is actively using the callbacks or direct usage of FileSystem becoming aware of the semantic changes is desirable considering the cost to implement full backward compatibility

FilesystemProtocol/copyItem has an overload providing the default CopyStrategy Motivation: Based on user feedback from #2806 This makes it easier to use, especially when copying files such that it is irrelevant. Modifications: Add overload providing (at:to:shouldProceedAfterError:shouldCopyItem:) with CopyStrategy.platformDefault as the strategy. Fix a cancellation bug which resulted in a test being flaky. In addition made the test cover that case reliably to prevent it coming back. Result: A simpler and more robust to change API will exist for copyItem()

UncleMattHope added 3 commits July 24, 2024 18:09

apply swift format rules

777c945

ignore visionOS for now

28ed84b

glbrntt requested changes Jul 25, 2024

View reviewed changes

UncleMattHope and others added 7 commits July 25, 2024 16:50

fixup my muscle memory for crefs to DocC form

5b60ed2

Merge branch 'main' into parallelise_directory_copies

f1ee5a5

Get fixes to the workflow check scripts

Merge branch 'main' into parallelise_directory_copies

cfb8ea1

pull in docs-check fix for local testing

fix all DocC errors

9b59c77

Apply suggestions from code review

93fb46d

Co-authored-by: George Barnett <gbarnett@apple.com>

fix naming

3c59120

shim compat for linux testing on 5.8

5bd6192

FranzBusch reviewed Jul 26, 2024

View reviewed changes

UncleMattHope and others added 4 commits July 26, 2024 14:22

Apply suggestions from code review

747cf80

Co-authored-by: Franz Busch <privat@franz-busch.de>

more code review fixups

277763d

confirm to formatting rules after rename

2f4d689

minor format fix

00a069a

glbrntt approved these changes Jul 26, 2024

View reviewed changes

UncleMattHope commented Jul 26, 2024

View reviewed changes

Fix nasty cancellation semantics in early phase

c9b6f55

These made the cancellation test flaky

glbrntt approved these changes Jul 29, 2024

View reviewed changes

Merge branch 'main' into parallelise_directory_copies

1509601

glbrntt merged commit 022ee13 into apple:main Jul 29, 2024
26 of 28 checks passed

UncleMattHope mentioned this pull request Jul 30, 2024

Provide a default CopyStrategy overload for copyItem. #2818

Merged

ayush1794 added the semver/minor Adds new public API. label Aug 5, 2024

FranzBusch mentioned this pull request Oct 17, 2024

removeItem performance #2933

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FileSystem.copyItem can parallelise directory copy #2806

FileSystem.copyItem can parallelise directory copy #2806

UncleMattHope commented Jul 24, 2024

glbrntt Jul 25, 2024

glbrntt Jul 25, 2024

glbrntt Jul 25, 2024

UncleMattHope Jul 26, 2024

glbrntt Jul 26, 2024

FranzBusch Jul 26, 2024

UncleMattHope Jul 26, 2024

FranzBusch Jul 26, 2024

FranzBusch Jul 26, 2024

FranzBusch Jul 26, 2024

glbrntt left a comment

UncleMattHope Jul 26, 2024

UncleMattHope commented Jul 29, 2024

	/// ``FileSystemProtocol.copyItem``
	/// ``FileSystemProtocol/copyItem``

	/// No errors should be throw by implementors without first calling ``shouldProceedAfterError``,
	/// No errors should be thrown by implementors without first calling ``shouldProceedAfterError``,

FileSystem.copyItem can parallelise directory copy #2806

FileSystem.copyItem can parallelise directory copy #2806

Conversation

UncleMattHope commented Jul 24, 2024

Motivation:

Modifications:

Result:

copyItem(*):

copyItem(sourcePath:destinationPath):

copyItem(sourcePath:destinationPath:shouldProceedAfterError:shouldCopyFile):

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

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

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UncleMattHope commented Jul 29, 2024

`copyItem(*)`:

`copyItem(sourcePath:destinationPath)`:

`copyItem(sourcePath:destinationPath:shouldProceedAfterError:shouldCopyFile)`: