Avoid creating a yield ID counter per async writer #2768

glbrntt · 2024-07-05T10:31:22Z

Motivation:

The NIOAsyncWriter uses an atomic to generate yield IDs. Each writer has its own atomic. We can save an allocatiuon per writer but using a shared atomic.

Each load then wrapping increment operation with relaxed ordering takes approx 3ns on my machine. For a UInt64 this would take approx 188 years to wrap around if run in a tight loop.

Modification:

Share a single yield ID counter for NIOAsyncWriter

Result:

Fewer allocations

Motivation: The NIOAsyncWriter uses an atomic to generate yield IDs. Each writer has its own atomic. We can save an allocatiuon per writer but using a shared atomic. Each load then wrapping increment operation with relaxed ordering takes approx 3ns on my machine. For a UInt64 this would take approx 188 years to wrap around if run in a tight loop. Modification: - Share a single yield ID counter for NIOAsyncWriter Result: Fewer allocations

Lukasa · 2024-07-05T12:18:22Z

Sources/NIOCore/AsyncSequences/NIOAsyncWriter.swift

@@ -17,6 +17,9 @@ import DequeModule
 import NIOConcurrencyHelpers
 import _NIODataStructures

+@usableFromInline
+let yieldIDCounter = ManagedAtomic<UInt64>(0)


This would probably be nicer as a static, but this is otherwise reasonable.

Agreed but wasn't sure where to put it. The obvious place is NIOAsyncWriter but that's generic so can't have a static let.

I am not sure if I agree with this move. I understand that we want to reduce allocations but IMO the correct move forward is to replace SwiftAtomics.ManagedAtomic with Synchronisation.Atomic which should avoid the allocation as well since it is ~Copyable. We ought to be able to do this conditionally when the compiler is Swift 6. Though availability might be tricky. If availability blocks us we can move this into a global. I would suggest renaming though to scope it better e.g. let _asyncWriterYieldIDCounter or something like that

I agree re: using Synchronization.Atomic but as noted, the availability makes this difficult. I've renamed the global though.

* 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

glbrntt requested a review from FranzBusch July 5, 2024 10:31

glbrntt added the semver/patch No public API change. label Jul 5, 2024

glbrntt force-pushed the shared-yield-id-generator branch from 46ed6c1 to 921a1ad Compare July 5, 2024 10:37

gjcairo approved these changes Jul 5, 2024

View reviewed changes

Lukasa approved these changes Jul 5, 2024

View reviewed changes

glbrntt added 2 commits July 8, 2024 14:48

better naming

95f909a

Merge branch 'main' into shared-yield-id-generator

0026472

FranzBusch approved these changes Jul 8, 2024

View reviewed changes

Merge branch 'main' into shared-yield-id-generator

2d77b0c

glbrntt enabled auto-merge (squash) July 9, 2024 11:52

glbrntt merged commit abbb144 into apple:main Jul 9, 2024
16 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid creating a yield ID counter per async writer #2768

Avoid creating a yield ID counter per async writer #2768

glbrntt commented Jul 5, 2024

Lukasa Jul 5, 2024

glbrntt Jul 5, 2024

FranzBusch Jul 8, 2024

glbrntt Jul 8, 2024

Avoid creating a yield ID counter per async writer #2768

Avoid creating a yield ID counter per async writer #2768

Conversation

glbrntt commented Jul 5, 2024

Lukasa Jul 5, 2024

Choose a reason for hiding this comment

glbrntt Jul 5, 2024

Choose a reason for hiding this comment

FranzBusch Jul 8, 2024

Choose a reason for hiding this comment

glbrntt Jul 8, 2024

Choose a reason for hiding this comment