ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out) #2791

weissi · 2024-07-17T08:32:19Z

Motivation:

Since version 1.0.0, SwiftNIO has provided the 'type-assisted' ChannelHandler concept with the self.(un)wrap(In|Out)bound(In|Out) helpers. Up until we got Self to work in all situations (I believe Swift 5.3 or so), this was the best syntax.

Unfortunately, this syntax does come at a cost:

If self is only retained to wrap/unwrap something in an @escaping closure (such as a future callback), there is needless ARC
If self is the only thing and only retained to wrap/unwrap there might even be an unnecessary allocation
Under strict concurrency, capturing self: ChannelHandler actually leads to the user needlessly having to NIOLoopBound(self, eventLoop: context.eventLoop) or similar

Modifications:

Provide static versions of (un)wrap(In|Out)bound(In|Out) in ChannelHandlers such that a user can write for example Self.unwrapInboundOut(foo).

Result:

Less ARC traffic
Fewer allocations
Strict Concurrency becomes easier
Easier job for the optimiser

Lukasa · 2024-07-17T11:06:47Z

Sources/NIOCore/TypeAssistedChannelHandler.swift

@@ -30,6 +30,11 @@ extension _EmittingChannelHandler {
    public func wrapOutboundOut(_ value: OutboundOut) -> NIOAny {
        return NIOAny(value)
    }
+
+    @inlinable
+    public static func wrapOutboundOut(_ value: OutboundOut) -> NIOAny {


Can we think of a reason not to deprecate the non-static versions?

I think it would be annoying to users having to change all of those :|.

What we should definitely remove though is the customisation point on those. There is only one correct implementation so I'd argue we could even remove those without a SemVer major break.

While we can, I don't think it helps us much, and will cause mayhem for those few users who are using library evolution mode even though we told them not to.

It's definitely annoying to users, but otherwise the odds that anyone will get the perf win is pretty low. self. is going to be muscle memory for most, and already written for the rest.

Lukasa · 2024-07-17T11:12:03Z

@swift-server-bot test perf please

swift-server-bot · 2024-07-17T11:12:21Z

!!! Couldn't read commit file !!!

Lukasa · 2024-07-17T11:13:54Z

Can't say I'm surprised that that's brokne.

weissi · 2024-07-17T14:07:54Z

@swift-server-bot test this please

FranzBusch

The code change looks good to me! We could potentially write an ARC benchmark for this but our experience with ARC benchmarks were pretty flaky so far.

weissi · 2024-07-18T10:21:30Z

The code change looks good to me! We could potentially write an ARC benchmark for this but our experience with ARC benchmarks were pretty flaky so far.

Indeed. Also, the risk here is 0. We all know that Self.something is better or equal to self.something, in any place.

* apple/main: (26 commits) [GHA] Only format Swift files that are in Git index (apple#2797) Ignore format commit from git blame (apple#2796) Adopt swift-format (apple#2794) Disable warnings as errors on Swift 6 and main (apple#2793) ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out) (apple#2791) Add manual control to NIOLockedValueBox (apple#2786) [GHA] Cxx interoperability compatibility and integration tests check (apple#2790) [GHA] Introduce reusable matrix workflow (apple#2789) Fix benchmark thresholds update script (apple#2783) [GHA] Broken symlink and format check (apple#2787) [GHA] Add license header check (apple#2781) Improved documentation for HTTP Parts to clarify how often each part is received (apple#2775) [GHA] Download the scripts to make workflow reusable (apple#2785) Combine the two NIOAsyncChannel channel handlers (apple#2779) [GHA] Benchmark job (apple#2780) [GHA] Move docs check to script (apple#2776) Add benchmark for creating NIOAsyncChannel (apple#2774) Avoid creating a yield ID counter per async writer (apple#2768) [GHA] Unacceptable language check (apple#2766) Allow in-place mutation of `NIOLoopBoundBox.value` (apple#2771) ... # Conflicts: # Sources/NIOPosix/BSDSocketAPICommon.swift # Sources/NIOPosix/GetaddrinfoResolver.swift # Sources/NIOPosix/HappyEyeballs.swift

weissi requested a review from FranzBusch July 17, 2024 08:32

ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out)

e9f2faa

weissi force-pushed the jw-static-wrap branch from d6a0a8e to e9f2faa Compare July 17, 2024 08:34

Lukasa added the 🆕 semver/minor Adds new public API. label Jul 17, 2024

Lukasa reviewed Jul 17, 2024

View reviewed changes

FranzBusch approved these changes Jul 18, 2024

View reviewed changes

Merge branch 'main' into jw-static-wrap

5b56715

weissi enabled auto-merge (squash) July 18, 2024 10:21

weissi merged commit 7948ed2 into apple:main Jul 18, 2024
28 of 35 checks passed

weissi deleted the jw-static-wrap branch July 18, 2024 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out) #2791

ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out) #2791

weissi commented Jul 17, 2024 •

edited

Loading

Lukasa Jul 17, 2024

weissi Jul 17, 2024

Lukasa Jul 17, 2024

weissi Jul 17, 2024

Lukasa commented Jul 17, 2024

swift-server-bot commented Jul 17, 2024

Lukasa commented Jul 17, 2024

weissi commented Jul 17, 2024

FranzBusch left a comment

weissi commented Jul 18, 2024

ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out) #2791

ChannelHandler: provide static (un)wrap(In|Out)bound(In|Out) #2791

Conversation

weissi commented Jul 17, 2024 • edited Loading

Motivation:

Modifications:

Result:

Lukasa Jul 17, 2024

Choose a reason for hiding this comment

weissi Jul 17, 2024

Choose a reason for hiding this comment

Lukasa Jul 17, 2024

Choose a reason for hiding this comment

weissi Jul 17, 2024

Choose a reason for hiding this comment

Lukasa commented Jul 17, 2024

swift-server-bot commented Jul 17, 2024

Lukasa commented Jul 17, 2024

weissi commented Jul 17, 2024

FranzBusch left a comment

Choose a reason for hiding this comment

weissi commented Jul 18, 2024

weissi commented Jul 17, 2024 •

edited

Loading