Add parallel removal of items #3008

mimischi · 2024-11-29T17:09:56Z

Introduce parallel removal of items, instead of only doing a sequential removal (and discovery) one-by-one

Motivation:

#2933 mentioned this function call appears to be slow. If we attempt to delete a directory with lots of subdirectories, we end up discovering each subdirectory one by one on a single thread.

Modifications:

This PR refactors the logic to remove items. Instead of sequentially discovering and deleting items one by one, we traverse all directories up to a maximum concurrency, and issue file deletions in parallel within each directory.

Result:

The new concurrent implementation was measured to be ~1.4x faster than FileManager, ~1.5x faster than rm -rf and ~3x faster than the current sequential implementation on main. This new concurrent implementation does use the most CPU out of all the other approaches.

Results are broken down into two sections. We delete a copy of the swift-nio repository, where the .build/ folder has grown to a large size, providing lots of additional files and subdirectories.

We produce a single CLI using --configration=release that can switch between using the .sequential mode (what is currently available on the main branch), .parallel (the new concurrent implementation in this PR) and filemanager (uses Foundation.FileManager as a comparison).

Resolves #2933

Normal size

Description: The swift-nio library as found when building the CLI used for benchmarking.
Benchmark run with: 3579 directories, 11657 files

$ hyperfine --runs 10 --warmup 2 --prepare "rm -rf /tmp/swift-nio && cp -r $(pwd) /tmp/swift-nio" ".build/release/NIOTestCLI --sequential /tmp/swift-nio" ".build/release/NIOTestCLI --parallel /tmp/swift-nio" ".build/release/NIOTestCLI --filemanager /tmp/swift-nio" "rm -rf /tmp/swift-nio"
Benchmark 1: .build/release/NIOTestCLI --sequential /tmp/swift-nio
  Time (mean ± σ):      2.712 s ±  0.053 s    [User: 0.526 s, System: 8.632 s]
  Range (min … max):    2.625 s …  2.781 s    10 runs

Benchmark 2: .build/release/NIOTestCLI --parallel /tmp/swift-nio
  Time (mean ± σ):     896.7 ms ±  25.6 ms    [User: 302.9 ms, System: 5313.5 ms]
  Range (min … max):   853.2 ms … 942.1 ms    10 runs

Benchmark 3: .build/release/NIOTestCLI --filemanager /tmp/swift-nio
  Time (mean ± σ):      1.337 s ±  0.076 s    [User: 0.022 s, System: 1.253 s]
  Range (min … max):    1.252 s …  1.508 s    10 runs

Benchmark 4: rm -rf /tmp/swift-nio
  Time (mean ± σ):      1.483 s ±  0.037 s    [User: 0.015 s, System: 1.416 s]
  Range (min … max):    1.413 s …  1.541 s    10 runs

Summary
  .build/release/NIOTestCLI --parallel /tmp/swift-nio ran
    1.49 ± 0.10 times faster than .build/release/NIOTestCLI --filemanager /tmp/swift-nio
    1.65 ± 0.06 times faster than rm -rf /tmp/swift-nio
    3.02 ± 0.10 times faster than .build/release/NIOTestCLI --sequential /tmp/swift-nio

Large size

Description: The swift-nio library as found after re-running the .github workflow many times locally. I'm unable to reproduce this directory size, but have benchmarked against it.
Benchmark run with: 15260 directories, 46842 files

$ hyperfine --runs 10 --warmup 2 --prepare "rm -rf /tmp/swift-nio && cp -r $(pwd) /tmp/swift-nio" ".build/release/NIOTestCLI --sequential /tmp/swift-nio" ".build/release/NIOTestCLI --parallel /tmp/swift-nio" ".build/release/NIOTestCLI --filemanager /tmp/swift-nio" "rm -rf /tmp/swift-nio"
Benchmark 1: .build/release/NIOTestCLI --sequential /tmp/swift-nio
  Time (mean ± σ):     12.082 s ±  0.354 s    [User: 2.180 s, System: 41.547 s]
  Range (min … max):   11.717 s … 12.923 s    10 runs

Benchmark 2: .build/release/NIOTestCLI --parallel /tmp/swift-nio
  Time (mean ± σ):      4.770 s ±  0.102 s    [User: 1.357 s, System: 25.785 s]
  Range (min … max):    4.570 s …  4.883 s    10 runs

Benchmark 3: .build/release/NIOTestCLI --filemanager /tmp/swift-nio
  Time (mean ± σ):      6.503 s ±  0.115 s    [User: 0.081 s, System: 5.427 s]
  Range (min … max):    6.262 s …  6.687 s    10 runs

Benchmark 4: rm -rf /tmp/swift-nio
  Time (mean ± σ):      7.056 s ±  0.246 s    [User: 0.064 s, System: 6.119 s]
  Range (min … max):    6.435 s …  7.233 s    10 runs

Summary
  .build/release/NIOTestCLI --parallel /tmp/swift-nio ran
    1.36 ± 0.04 times faster than .build/release/NIOTestCLI --filemanager /tmp/swift-nio
    1.48 ± 0.06 times faster than rm -rf /tmp/swift-nio
    2.53 ± 0.09 times faster than .build/release/NIOTestCLI --sequential /tmp/swift-nio

mimischi · 2024-12-01T20:08:34Z

I'll need a hand with the API breakage check. I can't reproduce the result locally, and I'm not entirely clear how to prevent the breakage reported in CI—would we not add a new removeItem method with overloaded arguments to FileSystemProtocol.swift to prevent this?

API breakage: func FileSystemProtocol.removeItem(at:strategy:recursively:) has been added as a protocol requirement

glbrntt

This looks great so far, I left a couple of notes inline.

Sources/NIOFileSystem/Internal/ParallelRemoval.swift

Sources/NIOFileSystem/FileSystemProtocol.swift

Sources/NIOFileSystem/Internal/Concurrency Primitives/TokenBucket.swift

Sources/NIOFileSystem/Internal/ParallelRemoval.swift

This PR refactors the logic to remove items. Instead of sequentially discovering and deleting items one by one, we concurrently traverse all directories that we find and then sequentially issue file deletions. This new deletion logic is currently unbounded, and will spawn a bunch of tasks no matter the underlying system. Benchmark using `--configration=release` for both the `main` and `parallel-removal` branches: ``` $ rm -rf .build && git switch main && swift build --configuration=release && cp .build/release/NIOTestCLI NIOTestCLI_main [...] Building for production... [134/134] Linking NIOPerformanceTester Build complete! (32.65s) $ rm -rf .build && git switch parallel-removal && swift build --configuration=release [...] Building for production... [134/134] Linking NIOPerformanceTester Build complete! (32.19s) $ hyperfine --max-runs 10 --prepare "rm -rf /tmp/swift-nio && cp -r $(pwd) /tmp/swift-nio" "./NIOTestCLI_main /tmp/swift-nio" ".build/release/NIOTestCLI /tmp/swift-nio" "rm -rf /tmp/swift-nio" Benchmark 1: ./NIOTestCLI_main /tmp/swift-nio Time (mean ± σ): 3.019 s ± 0.032 s [User: 0.601 s, System: 11.123 s] Range (min … max): 2.985 s … 3.072 s 10 runs Benchmark 2: .build/release/NIOTestCLI /tmp/swift-nio Time (mean ± σ): 2.295 s ± 0.065 s [User: 0.673 s, System: 7.677 s] Range (min … max): 2.212 s … 2.370 s 10 runs Benchmark 3: rm -rf /tmp/swift-nio Time (mean ± σ): 1.517 s ± 0.037 s [User: 0.015 s, System: 1.442 s] Range (min … max): 1.480 s … 1.590 s 10 runs Summary rm -rf /tmp/swift-nio ran 1.51 ± 0.06 times faster than .build/release/NIOTestCLI /tmp/swift-nio 1.99 ± 0.05 times faster than ./NIOTestCLI_main /tmp/swift-nio ``` Resolves apple#2933

We don't need to do any state tracking, if we rely on a simple depth-first search. Each DFS is kicked off on a separate task, and we await all tasks within that tree before we delete the given root. This makes things faster than `rm -rf`.

1. Do not rely on Atomics to sum up deleted files. We can return values from the TaskGroup instead. 2. Close the directory handle as quickly as possible, to free up capacity.

Adds `maxDescriptor` as an associated value to `parallel`, so we can rate limit the number of open file descriptors (directories we are scanning at the same time)

Now rate-limits the number of open directories to `maxDescriptors`.

Attempting to not break the API

Turns out you cannot run the breakage check on macOS. After all, the easiest thing to do was to go into the container and run the check manually.

mimischi · 2024-12-04T13:43:27Z

Updated the PRs description with the latest benchmarks

Sources/NIOFileSystem/IOStrategy.swift

Sources/NIOFileSystem/Internal/ParallelRemoval.swift

mimischi · 2024-12-04T17:01:37Z

Sources/NIOFileSystem/Internal/ParallelRemoval.swift

+        // Discover current directory and find all files/directories. Free up
+        // the handle as fast as possible.
+        let (directoriesToRecurseInto, itemsToDelete) = try await bucket.withToken {
+            try await self.withDirectoryHandle(atPath: path) { directory in


We could actually re-use part of removeItemSequentially here. The entire try away self.withDirectoryHandle {} piece can be made the same, if we go for changing when items are deleted (either in parallel in separate tasks, or still sequentially but before we dive into the next subdirectory)

I don't mind either way to be honest.

I went ahead and refactored it in 338ff35

Sources/NIOFileSystem/Internal/Concurrency Primitives/TokenBucket.swift

glbrntt

This is looking really good so far!

Sources/NIOFileSystem/Internal/Concurrency Primitives/TokenBucket.swift

glbrntt · 2024-12-06T09:54:08Z

Sources/NIOFileSystem/Internal/Concurrency Primitives/TokenBucket.swift

+        self.lock.lock()
+        defer { self.lock.unlock() }


You should be able to use withLock here, although you should return the waiter from the withLock call and then resume it.

Got that to work. Not convinced it's more readable than this simple two-liner though, but happy to adapt if that's the way.

Sources/NIOFileSystem/Internal/ParallelRemoval.swift

Sources/NIOFileSystem/Internal/Concurrency Primitives/TokenBucket.swift

Sources/NIOFileSystem/Internal/ParallelRemoval.swift

Tests/NIOFileSystemIntegrationTests/FileHandleTests.swift

Make them rely on the default behavior instead.

glbrntt

One nit but otherwise this looks great!

glbrntt · 2024-12-09T09:17:36Z

Sources/NIOFileSystem/Internal/ParallelRemoval.swift

+        // Discover current directory and find all files/directories. Free up
+        // the handle as fast as possible.
+        let (directoriesToRecurseInto, itemsToDelete) = try await bucket.withToken {
+            try await self.withDirectoryHandle(atPath: path) { directory in


I don't mind either way to be honest.

Sources/NIOFileSystem/Internal/Concurrency Primitives/TokenBucket.swift

Refactor `removeItemSequentially` to use `_collectItemsInDirectory`. Also use the new function in `ParallelRemoval.swift`

We are passing it around different concurrency contexts, and need to mark it as `Sendable` for when we enable stricter Swift 6 checks.

glbrntt · 2024-12-10T10:52:44Z

Sources/NIOFileSystem/FileSystem.swift

-        at path: FilePath
-    ) async throws -> Int {
-        var (subdirectories, filesRemoved) = try await self.withDirectoryHandle(
+    func _collectItemsInDirectory(at path: FilePath) async throws -> ([FilePath], [FilePath]) {


Can you add labels to the tuples? It's too easy to mix them up otherwise.

Actually, do we even need to split them? We could just return the directory entries and the caller can then iterate over them and do as they need. This would avoid allocating an array unnecessarily as well (not that I think it'll have any meaningful impact on perf).

I can get behind adding labels. If we only return a single array, then the callers would need to perform the sorting themselves, and thus replicate the bulk of what this utility does. I'm not wed to this existing, but I think if we have it then might as well use it to remove some level of duplication?

Had a quick discussion offline. Reverted the addition of _collectItemsInDirectory in e9df08e

This reverts commit 338ff35.

glbrntt

This LGTM, thanks @mimischi!

glbrntt · 2024-12-10T15:00:00Z

API break is expected; we are changing the requirement to add the strategy

3 breaking changes detected in _NIOFileSystem:
  💔 API breakage: func FileSystemProtocol.removeItem(at:strategy:recursively:) has been added as a protocol requirement

These fall out of that:

  💔 API breakage: func FileSystem.removeItem(at:recursively:) has parameter 1 type change from Swift.Bool to _NIOFileSystem.RemovalStrategy
  💔 API breakage: func FileSystem.removeItem(at:recursively:) has been renamed to func removeItem(at:strategy:recursively:)

mimischi marked this pull request as draft November 29, 2024 17:10

mimischi mentioned this pull request Nov 29, 2024

removeItem performance #2933

Closed

glbrntt requested changes Dec 2, 2024

View reviewed changes

mimischi commented Dec 3, 2024

View reviewed changes

Sources/NIOFileSystem/Internal/Concurrency Primitives/TokenBucket.swift Outdated Show resolved Hide resolved

mimischi commented Dec 3, 2024

View reviewed changes

Sources/NIOFileSystem/Internal/ParallelRemoval.swift Outdated Show resolved Hide resolved

mimischi added 15 commits December 4, 2024 11:33

Format files

ac5fe9f

Simplify structure by relying on dfs

4db67c0

We don't need to do any state tracking, if we rely on a simple depth-first search. Each DFS is kicked off on a separate task, and we await all tasks within that tree before we delete the given root. This makes things faster than `rm -rf`.

Add documentation to new code and rename functions

09d11a9

Attempt not to break API

861c188

Fix documentation check, and some indents

1d60824

Refactor for a cleaner structure

a97776b

1. Do not rely on Atomics to sum up deleted files. We can return values from the TaskGroup instead. 2. Close the directory handle as quickly as possible, to free up capacity.

Add TokenBucket adaptation from SwiftPM

1a2f27e

Add maxDescriptors to RemovalStrategy.swift

2a1d510

Adds `maxDescriptor` as an associated value to `parallel`, so we can rate limit the number of open file descriptors (directories we are scanning at the same time)

Update tests

5d7a178

Update ParallelRemoval to use TokenBucket

7a91ae6

Now rate-limits the number of open directories to `maxDescriptors`.

Add FileSystemProtocol method to not break API

1b010e5

Attempting to not break the API

Actually stop breaking the API

b336a44

Turns out you cannot run the breakage check on macOS. After all, the easiest thing to do was to go into the container and run the check manually.

Format file

0a83116

Refactor IO strategies to share commonalities

4390a8e

mimischi force-pushed the parallel-removal branch from 3311c18 to 4390a8e Compare December 4, 2024 11:35

mimischi requested a review from glbrntt December 4, 2024 11:38

Fix typo in comment on function signature

f00e17a

mimischi commented Dec 4, 2024

View reviewed changes

Sources/NIOFileSystem/IOStrategy.swift Show resolved Hide resolved

mimischi commented Dec 4, 2024

View reviewed changes

Sources/NIOFileSystem/Internal/ParallelRemoval.swift Outdated Show resolved Hide resolved

mimischi commented Dec 4, 2024

View reviewed changes

mimischi marked this pull request as ready for review December 5, 2024 16:42

mimischi commented Dec 5, 2024

View reviewed changes

Sources/NIOFileSystem/Internal/Concurrency Primitives/TokenBucket.swift Outdated Show resolved Hide resolved

glbrntt requested changes Dec 6, 2024

View reviewed changes

mimischi added 5 commits December 6, 2024 15:55

Address reviews for TokenBucket.swift

1e4d5b5

Address ParallelRemoval.swift review comments

dbd3ded

Address IOStrategy.swift reviews

5b536ca

Remove unnecessary implementations

3d990b1

Remove new arguments to tests function calls

9146b91

Make them rely on the default behavior instead.

mimischi requested a review from glbrntt December 6, 2024 16:38

glbrntt reviewed Dec 9, 2024

View reviewed changes

mimischi added 2 commits December 9, 2024 11:17

Re-use directory discovery logic

338ff35

Refactor `removeItemSequentially` to use `_collectItemsInDirectory`. Also use the new function in `ParallelRemoval.swift`

Makr TokenBucket as @unchecked Sendable

7ed6d7d

We are passing it around different concurrency contexts, and need to mark it as `Sendable` for when we enable stricter Swift 6 checks.

mimischi requested a review from glbrntt December 9, 2024 11:39

glbrntt reviewed Dec 10, 2024

View reviewed changes

Revert "Re-use directory discovery logic"

e9df08e

This reverts commit 338ff35.

glbrntt approved these changes Dec 10, 2024

View reviewed changes

Merge branch 'main' into parallel-removal

e98388f

glbrntt added the 🆕 semver/minor Adds new public API. label Dec 10, 2024

glbrntt merged commit 29c65ed into apple:main Dec 10, 2024
31 of 36 checks passed

mimischi deleted the parallel-removal branch December 11, 2024 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parallel removal of items #3008

Add parallel removal of items #3008

mimischi commented Nov 29, 2024 •

edited

Loading

mimischi commented Dec 1, 2024

glbrntt left a comment

mimischi commented Dec 4, 2024

mimischi Dec 4, 2024 •

edited

Loading

glbrntt Dec 9, 2024

mimischi Dec 9, 2024

glbrntt left a comment

glbrntt Dec 6, 2024

mimischi Dec 6, 2024

glbrntt left a comment

glbrntt Dec 9, 2024

glbrntt Dec 10, 2024

glbrntt Dec 10, 2024

mimischi Dec 10, 2024

mimischi Dec 10, 2024

glbrntt left a comment

glbrntt commented Dec 10, 2024

Add parallel removal of items #3008

Add parallel removal of items #3008

Conversation

mimischi commented Nov 29, 2024 • edited Loading

Motivation:

Modifications:

Result:

Normal size

Large size

mimischi commented Dec 1, 2024

glbrntt left a comment

Choose a reason for hiding this comment

mimischi commented Dec 4, 2024

mimischi Dec 4, 2024 • edited Loading

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

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

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

glbrntt commented Dec 10, 2024

mimischi commented Nov 29, 2024 •

edited

Loading

mimischi Dec 4, 2024 •

edited

Loading