Adopt `NIOThrowingAsyncSequenceProducer` #2879

rnro · 2024-09-10T14:23:49Z

Motivation:

Adopt NIOThrowingAsyncSequenceProducer in NIOFileSystem to reduce code duplication.

Modifications:

Adopt NIOThrowingAsyncSequenceProducer in NIOFileSystem DirectoryEntryProducer and FileChunkProducer

Result:

No functional changes. Internal changes reduce code duplication.

FranzBusch

In general this looks good to me. Have you run some benchmarks between the two to make sure we still have the same performance?

glbrntt

This looks good! I left a couple of small comments which need addressing.

glbrntt · 2024-09-11T13:05:24Z

Sources/NIOFileSystem/DirectoryEntries.swift

@@ -135,52 +136,85 @@ extension DirectoryEntries.Batched.AsyncIterator: Sendable {}
 // MARK: - Internal

 @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
-extension BufferedStream where Element == [DirectoryEntry] {
+extension NIOThrowingAsyncSequenceProducer where Element == [DirectoryEntry], Failure == Error,


nit

Suggested change

extension NIOThrowingAsyncSequenceProducer where Element == [DirectoryEntry], Failure == Error,

extension NIOThrowingAsyncSequenceProducer where Element == [DirectoryEntry], Failure == (any Error),

glbrntt · 2024-09-11T13:12:28Z

Sources/NIOFileSystem/DirectoryEntries.swift

        switch self.state {
        case let .idle(handle, _):
            return handle.threadPool
        case let .open(threadPool, _, _):
            return threadPool
+        case .openPausedProducing(let threadPool, let source, let array):
+            self.state = .modifying


.modifying isn't required here because we aren't modifying any of the state held in the openPausedProducing case

glbrntt · 2024-09-11T13:12:48Z

Sources/NIOFileSystem/DirectoryEntries.swift

+    internal mutating func pauseProducing() {
+        switch self.state {
+        case .open(let nIOThreadPool, let source, let array):
+            self.state = .modifying


same here, no need to do .modifying

glbrntt · 2024-09-11T13:13:15Z

Sources/NIOFileSystem/DirectoryEntries.swift

        case .done:
            return nil
        case .modifying:
            fatalError()
        }
    }

+    internal mutating func pauseProducing() {
+        switch self.state {
+        case .open(let nIOThreadPool, let source, let array):


nit: threadPool

glbrntt · 2024-09-11T13:14:39Z

Sources/NIOFileSystem/FileChunks.swift

+    init(range: FileChunks.ChunkRange, handle: SystemFileHandle, length: Int64) {
+


nit: no blank lines at the start of funcs

Suggested change

init(range: FileChunks.ChunkRange, handle: SystemFileHandle, length: Int64) {

init(range: FileChunks.ChunkRange, handle: SystemFileHandle, length: Int64) {

glbrntt · 2024-09-11T13:15:35Z

Sources/NIOFileSystem/FileChunks.swift

+        let state: ProducerState = switch range {
+        case .entireFile:
+                .init(handle: handle, range: nil)
+        case .partial(let partialRange):
+                .init(handle: handle, range: partialRange)
+        }


Hmm, I don't think our minimum Swift version is new enough to support this? Also the indentation is way off for the body of each case.

(Also I really don't like this syntax but maybe I'm just a curmudgeon...)

glbrntt · 2024-09-11T13:26:58Z

Sources/NIOFileSystem/FileChunks.swift

+
+@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
+extension ProducerState.Producing {
+    mutating func updateRange(count: Int) -> Bool {


It's not obvious how the range is updated without looking at the implementation, also the return value is quite misleading: if updateRange(count: blah) { ... } looks a whole lot like "if the range was updated then ...".

Would probably be clearer if you call this didReadBytes(_ count: Int) and documented the return value (you could use an enum for the return type but that's probably overkill)

glbrntt · 2024-09-11T13:29:30Z

In general this looks good to me. Have you run some benchmarks between the two to make sure we still have the same performance?

I would be surprised if there was a substantial difference. You'd think the syscalls would dominate here but it's worth doing some validation nonetheless.

glbrntt

Looks good modulo some failing CI!

glbrntt · 2024-09-13T09:01:51Z

Sources/NIOFileSystem/FileChunks.swift

+        case .produceMore:
+            self.produceMore()
+        case .stopProducing:
+            self.state.withLockedValue { state in state.pauseProducing()}


Suggested change

self.state.withLockedValue { state in state.pauseProducing()}

self.state.withLockedValue { state in state.pauseProducing() }

Motivation: Adopt `NIOThrowingAsyncSequenceProducer` in NIOFileSystem to reduce code duplication. Modifications: Adopt `NIOThrowingAsyncSequenceProducer` in NIOFileSystem `DirectoryEntryProducer` and `FileChunkProducer` Result: No functional changes. Internal changes reduce code duplication.

rnro · 2024-09-13T13:18:19Z

Performance-wise there doesn’t seem to be much of a difference.

I tested listing the files on disk, exercising the DirectoryEntries path. I listed a directory I happened to have with 772492 files in it making up 23GiB.

For the existing code:
3 runs of: 27.51s, 26.53s, 26.34s
With an average of: 26.8s

For the new code:
3 runs of: 26.08s, 27.71s, 27.35s
With an average of: 27.05s

### Motivation: Adopt `NIOThrowingAsyncSequenceProducer` in NIOFileSystem to reduce code duplication. ### Modifications: Adopt `NIOThrowingAsyncSequenceProducer` in NIOFileSystem `DirectoryEntryProducer` and `FileChunkProducer` ### Result: No functional changes. Internal changes reduce code duplication.

finagolfin · 2024-09-17T13:35:33Z

This pull consistently causes hanging FileHandle tests in my Android CI, that runs them in an Android x86_64 emulator on both linux and macOS built with 5.10, 6.0, and trunk 6.1. I can reproduce locally on Android 13 AArch64 with a recently-built trunk 6.1 toolchain from the Sep. 4 source snapshot and the Termux 5.10.1 toolchain.

I'll see if I can track it down and file an issue.

This reverts commit 282f593.

This reverts commit 282f593. ### Motivation: Repeated testing in linux containers has surfaced issues with the implementation which require some rethinking of the code, let's revert it for now and fix it up. ### Modifications: Revert the adoption commit. ### Result: The change is backed out.

### Motivation: Adopt `NIOThrowingAsyncSequenceProducer` in NIOFileSystem to reduce code duplication. ### Modifications: Adopt `NIOThrowingAsyncSequenceProducer` in NIOFileSystem `DirectoryEntryProducer` and `FileChunkProducer`. This change was previously merged and then backed out due to issues (#2879). The original change is in the first commit, the second commit contains additional changes. In the original adoption of `NIOThrowingAsyncSequenceProducer` the code did not deal with backpressure being applied very well, in some cases causes hangs. A key bug was that it was not protected against re-entrant calls to `produceMore`. Such calls may happen e.g. when the producer has been asked to pause producing and then resume again before the initial read had completed. This resulted in overlapping reads and re-ordered data. This change introduces a new activity state which is protected by a lock and keeps track of if we are in the critical section to serialize access. `DirectoryEntryProducer` performs most of its logic within a lock and so doesn't seem to be impacted in the same way. ### Result: No functional changes. Internal changes reduce code duplication.

rnro requested a review from glbrntt September 10, 2024 14:23

FranzBusch added the 🔨 semver/patch No public API change. label Sep 11, 2024

FranzBusch reviewed Sep 11, 2024

View reviewed changes

glbrntt requested changes Sep 11, 2024

View reviewed changes

rnro force-pushed the adopt_NIOThrowingAsyncSequenceProducer branch 4 times, most recently from 7d74670 to 41fe2de Compare September 12, 2024 15:58

glbrntt approved these changes Sep 13, 2024

View reviewed changes

rnro force-pushed the adopt_NIOThrowingAsyncSequenceProducer branch from 41fe2de to b61146f Compare September 13, 2024 10:18

rnro added 2 commits September 13, 2024 11:48

review comments

eac6c2a

rnro force-pushed the adopt_NIOThrowingAsyncSequenceProducer branch from b61146f to eac6c2a Compare September 13, 2024 10:48

FranzBusch approved these changes Sep 13, 2024

View reviewed changes

rnro merged commit 282f593 into apple:main Sep 13, 2024
27 of 29 checks passed

rnro deleted the adopt_NIOThrowingAsyncSequenceProducer branch September 13, 2024 13:21

finagolfin mentioned this pull request Sep 18, 2024

NIOThrowingAsyncSequenceProducer gets the FileHandle tests to hang on linux/Android #2887

Closed

rnro added a commit to rnro/swift-nio that referenced this pull request Sep 20, 2024

Revert "Adopt NIOThrowingAsyncSequenceProducer (apple#2879)"

dedbc6a

This reverts commit 282f593.

rnro mentioned this pull request Oct 11, 2024

Adopt NIOThrowingAsyncSequenceProducer 2nd try #2917

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt `NIOThrowingAsyncSequenceProducer` #2879

Adopt `NIOThrowingAsyncSequenceProducer` #2879

rnro commented Sep 10, 2024

FranzBusch left a comment

glbrntt left a comment

glbrntt Sep 11, 2024

glbrntt Sep 11, 2024

glbrntt Sep 11, 2024

glbrntt Sep 11, 2024

glbrntt Sep 11, 2024

glbrntt Sep 11, 2024

glbrntt Sep 11, 2024

glbrntt Sep 11, 2024

glbrntt commented Sep 11, 2024

glbrntt left a comment

glbrntt Sep 13, 2024

rnro commented Sep 13, 2024

finagolfin commented Sep 17, 2024

	extension NIOThrowingAsyncSequenceProducer where Element == [DirectoryEntry], Failure == Error,
	extension NIOThrowingAsyncSequenceProducer where Element == [DirectoryEntry], Failure == (any Error),

		init(range: FileChunks.ChunkRange, handle: SystemFileHandle, length: Int64) {

	self.state.withLockedValue { state in state.pauseProducing()}
	self.state.withLockedValue { state in state.pauseProducing() }

Adopt NIOThrowingAsyncSequenceProducer #2879

Adopt NIOThrowingAsyncSequenceProducer #2879

Conversation

rnro commented Sep 10, 2024

Motivation:

Modifications:

Result:

FranzBusch left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glbrntt commented Sep 11, 2024

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnro commented Sep 13, 2024

finagolfin commented Sep 17, 2024

Adopt `NIOThrowingAsyncSequenceProducer` #2879

Adopt `NIOThrowingAsyncSequenceProducer` #2879