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.

Sign up for GitHub

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

Jump to bottom

Allow in-place mutation of NIOLoopBoundBox.value #2771

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Allow in-place mutation of NIOLoopBoundBox.value #2771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e7217f
Allow in-place mutation of `NIOLoopBoundBox.value`
dnadoba Jul 7, 2024
4059c21
Fix typos in docs
dnadoba Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Sources/NIOCore/NIOLoopBound.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ public struct NIOLoopBound<Value>: @unchecked Sendable {
self._eventLoop.preconditionInEventLoop()
return self._value
}
set {
_modify {
Copy link
Member

Choose a reason for hiding this comment

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

@Lukasa What's the current position on _modify, is that now allowed? Alternative is obvs to make a withMutating { inout ... }?

Copy link
Member

Choose a reason for hiding this comment

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

We have used _modify and _read in multiple places now for performance. I think at this point it sadly is allowed to be used.

Copy link
Member

Choose a reason for hiding this comment

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

fine by me

Copy link
Member Author

@dnadoba dnadoba Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have completely switched to use _modify by default if the property type can be a CoW type.
Combining this with consume self if the property isn't simply a stored property and I can implement efficient computed properties without worrying about triggering CoW. The CoWValue test helper then makes sure this actually checks out and stays like this.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think that's a good development. There are still open questions about _modify which is why it's underscored. And _modify is probably less benign than other underscored stuff like @inline(__always) or @_cdecl. The precise semantics haven't been settled and things may change.

Anybody aware of a post-2022 effort of getting these accessors specified?

Copy link
Member

Choose a reason for hiding this comment

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

The concern is that it's an underscored (i.e. not ready for prime time) feature whose semantics aren't fully specified.

The standard library is developed and shipped together with the compiler itself which makes it different. Example: Before Swift had a defined memory model, the standard library pretended there to be a defined memory.

Regarding swift-collections: That's up to the Swift collection authors.

Concerns regarding _modify: It's very easy to go wrong. For example if you have this code

_modify {
   print("A")
   yield &self._something
   print("B")
}

then you will not always see B after you saw an A. It depends if there was a throw in the mutation. This can leave your type in an inconsistent (possibly memory-unsafe) state and is arguably hard to see (there's no try or place to catch). Sure, if the yield is the last line in a _modify then it's probably fine.

But fundamentally, we can just offer a func withMutableValue { inout Value ... } which can achieve the same as _modify (worse syntax ofc).

This is just a decision for the NIO team. They maintain the code base, so they need to be comfortable with the amount of undefined, underscored stuff that's in the code base at present. New compiler versions could adjust things for underscored features.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is fair and matches my understanding of the risk. It's a trade off, similar to the use of unsafe or unchecked swift features. The pitch explains this as well. Are there any other known issues today?

possibly memory-unsafe

@weissi is that possible without additional use of unsafe swift?

Agree, thats up to the NIO team.

Copy link
Member

Choose a reason for hiding this comment

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

possibly memory-unsafe
@weissi is that possible without additional use of unsafe swift?

I would assume so because compiler-emitted code could be skipped too (unless it protects against that). The key point is: Without an _modify implementations checked against the current compiler invocations we don't really know.

Copy link
Member

Choose a reason for hiding this comment

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

I am personally okay with using _modify since it is used by the compiler and standard library developers outside the stdlib e.g. swift-collections. It makes the code that developers write fast without them having to discover a modify method. Additionally, we are using more underscored attributes such as @_exported or @_alwaysEmitIntoClient.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're using @_alwaysEmitIntoClient we should definitely rethink that too.

@_exported is underscored, but its behaviour is well understood and safe. _modify is definitely not that.

As you know, the compiler and standard library developers explicitly do not make the same compatibility promises we do. That allows them to freely use these annotations, as they can always resolve issues by dropping all prior Swift versions. We can't do that, so if _modify introduces issues, we'll be forced to fork or revert the change.

self._eventLoop.preconditionInEventLoop()
self._value = newValue
yield &self._value
}
}
}
Expand Down Expand Up @@ -136,9 +136,9 @@ public final class NIOLoopBoundBox<Value>: @unchecked Sendable {
self._eventLoop.preconditionInEventLoop()
return self._value
}
set {
_modify {
self._eventLoop.preconditionInEventLoop()
self._value = newValue
yield &self._value
}
}
}
Expand Down
31 changes: 31 additions & 0 deletions Tests/NIOPosixTests/CoWValue.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the SwiftNIO open source project
//
// Copyright (c) 2024 Apple Inc. and the SwiftNIO project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//

/// A Copy on Write (CoW) type that can be used in tests to assert in-place mutation
struct CoWValue: @unchecked Sendable {
private final class UniquenessIndicator {}

/// This reference is "copied" if not uniquely referenced
private var uniquenessIndicator = UniquenessIndicator()

/// mutates `self` and returns a boolean whether it was mutated in place or not
/// - Returns: true if mutation happened in-place, false if Copy on Write (CoW) was triggered
mutating func mutateInPlace() -> Bool {
guard isKnownUniquelyReferenced(&self.uniquenessIndicator) else {
self.uniquenessIndicator = UniquenessIndicator()
return false
}
return true
}
}
8 changes: 8 additions & 0 deletions Tests/NIOPosixTests/NIOLoopBoundTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ final class NIOLoopBoundTests: XCTestCase {
}.wait())
}

func testInPlaceMutation() {
var loopBound = NIOLoopBound(CoWValue(), eventLoop: loop)
XCTAssertTrue(loopBound.value.mutateInPlace())

let loopBoundBox = NIOLoopBoundBox(CoWValue(), eventLoop: loop)
XCTAssertTrue(loopBoundBox.value.mutateInPlace())
}

// MARK: - Helpers
func sendableBlackhole<S: Sendable>(_ sendableThing: S) {}

Expand Down