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.

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

NIOLoopBound should get unchecked variants that are truly free in release mode #2506

Open
weissi opened this issue Aug 17, 2023 · 5 comments · May be fixed by #2515
Open

NIOLoopBound should get unchecked variants that are truly free in release mode #2506

weissi opened this issue Aug 17, 2023 · 5 comments · May be fixed by #2515
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@weissi
Copy link
Member

weissi commented Aug 17, 2023

NIOLoopBound(Box) is super useful when adopting Sendable but the eventLoop.preconditionInEventLoop() checks everywhere are expensive and -- for repeated accesses -- unnecessary.

There should be unchecked variants of init, get and set value that do eventLoop.assertInEventLoop() instead. Together with @inlinable that should become completely free in release mode (and still check in debug).

Docs:

@weissi weissi added enhancement New feature or request good first issue Good for newcomers labels Aug 17, 2023
@dnadoba
Copy link
Member

dnadoba commented Aug 18, 2023

The other option is to use CurrentEventLoop and friends from #2228 which only call preconditionInEventLoop() once during initialisation.

@weissi
Copy link
Member Author

weissi commented Aug 18, 2023

The other option is to use CurrentEventLoop and friends from #2228 which only call preconditionInEventLoop() once during initialisation.

Yes, good to have multiple options.

@natikgadzhi
Copy link
Contributor

It seems like NIOLoopBoundBox has internal init with an unchecked event loop already. And, NIOLoopBox conforms to @unchecked Sendable. To confirm:

  • We're not talking about that @unchecked Sendable protocol.
  • You suggest adding a version of init and value get and set that would still verify the same event loop, but with an assert, so they might blow up.
  • If the approach below is what you mean, then in release mode, no preconditionInEventLoop() will be invoked. So if something else changes what event loop the code is running on between debug and release, the user might get in trouble. But that's on them, they've used the unchecked variant.

Something like:

// NIOLoopBound

@inlinable
public init(_ value: Value, uncheckedEventLoop: EventLoop) {
    eventLoop.assertInEventLoop()
    self._eventLoop = eventLoop
    self._value = value
}

@inlinable
public var uncheckedValue: Value {
    get {
        self._eventLoop.assertInEventLoop()
        return self._value
    }
    set {
        self._eventLoop.assertInEventLoop()
        self._value = newValue
    }
}

@dnadoba @weissi I've read the proposal in #2228, but I'm not yet experienced enough to quite grasp how I could use it to achieve a similar result.

If the approach above is useful, I'm happy to put this together. If not, let me go work through a few example apps to gain more experience with event loops. Pointers / speed runs towards that particular area are very welcome 🙃

@weissi
Copy link
Member Author

weissi commented Aug 31, 2023

@natikgadzhi yes, thanks, uncheckedValue is exactly what I had in mind.

natikgadzhi added a commit to natikgadzhi/swift-nio that referenced this issue Sep 9, 2023
**Summary**:
This commit adds unchecked version of init and value get+set of
`NIOLoopBound` and `NIOLoopBoundBox`.

**Motivation**:
- Closes apple#2506.

**Changes**:
- Added `init` and `public var uncheckedValue` properties to both
  `NIOLoopBound` and `NIOLoopBoundBox`.

**Review & help**
- [x] Tests pass locally.
- [ ] How should we test that `NIOLoopBound.init(uncheckedEventLoop:)`
  calls `assertInEventLoop`? `EmbeddedEventLoop` seems to be final — how
  do I mock the assert function on in elegantly, without copy and
    pasting all the protocol conformance stuff?
@natikgadzhi natikgadzhi linked a pull request Sep 9, 2023 that will close this issue
2 tasks
@Amarylla923
Copy link

Thank you swift 😊❤️ I 🍏🍎 m glad you are you. Swiftapple. 😊 Good I will look it over and add or git init if it is possible thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants