### Motivation:
PR #3297 is changing `inEventLoop` and `inEventLoop` is very much on the
perf path. So let's add a benchmark
### Modifications:
- Add `(!)inEventLoop` benchmark
- Fix a guaranteed deadlock in `TCPThroughputBenchmark` to make the
benchmarks runnable
### Result:
- More benchmarks
- Fewer hangs
### Motivation:
As outlined in this
[issue](https://github.com/apple/swift-nio/issues/2930) by @weissi, the
current performance of calling `description` on `HTTPHeaders` is
undermined by the dynamism of Array.
### Modifications:
The proposed solution replaces the implementation of `description` to
iterate over the items and print them out manually. This provides a
faster solution since we bypass the cost of calling into `description`
of an Array.
### Result:
A more performant implementation of `HTTPHeaders` description.
---------
Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
The ChannelInvoker protocols are an awkward beast. They aren't really
something that people can do generic programming against. Instead, they
were designed to do API sharing. Of course, they didn't do that very
well, and the strict concurrency checking world has revealed this.
Much of the API surface on ChannelInvoker is confused. There are
NIOAnys, which aren't Sendable. We allow sending user events without
requiring Sendable. And our two main conforming types are
ChannelPipeline and ChannelHandlerContext, two types with wildly
differing thread-safety semantics.
This PR aims to clean that up.
Modifications:
- Deprecated all API surface on ChannelInvoker protocols that uses
NIOAny.
ChannelInvoker has to be assumed to be a cross-thread protocol,
and that requires that it only use Sendable types. NIOAny isn't,
so these methods are no longer sound.
- Re-add non-deprecated versions on ChannelHandlerContext.
While it's not safe to use the NIOAny methods on Channel or
ChannelPipeline, it's totally safe to use them on
ChannelHandlerContext. So we keep those available and
undeprecated.
- Provide typed generic replacements on ChannelPipeline and on Channel
To replace the NIOAny methods on ChannelPipeline and Channel
we can use some typed generic ones instead. These are not
defined on ChannelInvoker, as the methods are useless on
ChannelHandlerContext. This begins the acknowledgement that
ChannelHandlerContext should not have conformed to these
protocols at all.
- Add Sendable constraints to the user event witnesses on ChannelInvoker
Again, these were missing, but must be there for Channel and
ChannelPipeline.
- Provide non-Sendable overloads on ChannelHandlerContext
ChannelHandlerContext is thread-bound, and so may safely pass
non-Sendable user events.
Result:
One step closer to strict concurrency cleanliness for NIOCore.
Motivation:
Since Swift 5.5 and [SE-0299](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0299-extend-generic-static-member-lookup.md) it is possible to add static members to protocols which are discoverable through the shorthand dot syntax.
This change reduces type repetition and improves call-site legibility.
Modifications:
Added extensions for ChannelOption with static members where Self is bound to a concrete type.
Result:
ChannelOption types can be used with the leading dot syntax. For eg:
```
//before
.channelOption(ChannelOptions.explicitCongestionNotification, value: true)
.channelOption(ChannelOptions.socketOption(.so_reuseaddr), value: 1)
//after
.channelOption(.explicitCongestionNotification, value: true)
.channelOption(.socketOption(.so_reuseaddr), value: 1)
```
* Apply formatting
* Apply no block comments rule
* Apply OmitExplicitReturns
* Apple OnlyOneTrailingClosureArgument
* Apply NoAssignmentInExpressions
* Fix up DontRepeatTypeInStaticProperties lint errors
* Apply `OrderedImports`
* Apply `ReplaceForEachWithForLoop`
* format file
* Enable the formatting pipeline
* Adopt `AmbiguousTrailingClosureOverload`
* Fix license header
* Fix format check
* Fix `EndOfLineComment`
* Fix CI
* Adapt CI script to check if changes when running formatting
* Separate lint and format into to steps
* Fix format
* Adopt `UseEarlyExits`
* Revert "Adopt `UseEarlyExits`"
This reverts commit d1ac5bbe12.
# Motivation
Adding a handler via the regular `pipeline.addHandler` APIs can lead to transferring the handler over isolation regions. To avoid running into `Sendable` warnings here we can use the `syncOperations` of the pipeline to avoid transferring the handlers.
# Modification
This PR is mostly changing code to avoid transferring handlers to and from the event loops in tests. This should be a no-op mostly.
Co-authored-by: Cory Benfield <lukasa@apple.com>
* Fix `SchedulingBenchmark` preheating logic.
Motivation:
At the moment `SchedulingBenchmark` preheats single EL to a specified number of tasks. However, the actual performance test runs number of times, and EL doesn't get drained in between the runs.
There are two problems with it:
* perf test will trigger task heap dublings, which the preheating aims to avoid
* each test run will be working with proportionally deeper heap, which means the run times are going to grow with each run
Modifications:
I plumbed through the # of runs to the `Benchmark.setUp`, and prepare ELG with # of ELs that match the expected number of runs.
Result:
Performance test will be more reliable.
* Revert "Fix `SchedulingBenchmark` preheating logic."
This reverts commit 859ba72c78.
* Remove SchedulingBenchmark. Deemed not valuable enough.
* Benchmark for measuring NIOThreadPool dispatch
* Avoid CoW cost for NIOThreadPool work queue
* Flip stray `var` to a `let`.
* Use .modifying state to prevent CoW, not a class
* Removing some obselete comment noise.
* Use Deque instead of CirclarBuffer in NIOThreadPool
* Ratchet down the # of allocs for read_10000_chunks_from_file. NIOTheadPool changes reduced the allocations required.
Motivation:
The performance test binary was crashing ever since #2589 added the crash
on deinit flow. Crashes here are preventing us from using the performance
tester.
Modifications:
Correctly clean up the async writer.
Result:
The writer is cleaned up now.
* Add `withInboundOutboud` to `NIOAsyncChannel` and deprecate deinit based cleanup
# Motivation
We just released our new async NIO APIs and have already gotten quite a bunch of feedback from adopters. One of the feedback was that the deinit based closing that we have added to the `NIOAsyncChannel` has caused problems since it leads to unexpected closure of their `Channel`. Furthermore, it makes it impossible to determine how many open sockets a program has at any given time since deinit based clean up relies on the optimizer and can happen at random times.
# Modifications
This PR adds new inits to `NIOAsyncSequenceProducer` and `NIOAsyncWriter` which disable the `deinit` based clean up and instead replace them with an assertion. This allows developers to still catch these issues at debug time. Furthermore, I added a new `withInboundOutbound` scoped access to `NIOAsyncChannel` which will close the channel at the end of the scope. This still gives users a nice API while not having to care much about closing themselves.
# Result
We are no longer using deinit based clean up and bring back one of the core principles of NIO which is deterministic resource usage.
* Review comments
* Internal labels for closure arguments
* Rename to `executeThenCloseChannel`
* Actually call `sinkDeinitialized`
* Change preconditions
* Move logic to deinits
* Rename to `executeThenClose` and review nits
Motivation:
Several thread-safety issues were missed in code review. This patch fixes them.
Modifications:
- Removed the use of an unstructured Task, replaced with eventLoop.execute to
ServerHandler's EventLoop.
- Stopped ClientHandler reaching into the benchmark object without any
synchronization, used promises and event loop hops instead.
Result:
Thread safety is back
### Motivation:
Follow up PR for https://github.com/apple/swift-nio/pull/2399
We currently still return `nil` if the current `Task` is canceled before the first call to `NIOThrowingAsyncSequenceProducer.AsyncIterator.next()` but it should throw `CancellationError` too.
In addition, the generic `Failure` type turns out to be a problem. Just throwing a `CancellationError` without checking that `Failure` type is `any Swift.Error` or `CancellationError` introduced a type safety violation as we throw an unrelated type.
### Modifications:
- throw `CancellationError` on eager cancellation
- deprecates the generic `Failure` type of `NIOThrowingAsyncSequenceProducer`. It now must always be `any Swift.Error`. For backward compatibility we will still return nil if `Failure` is not `any Swift.Error` or `CancellationError`.
### Result:
`CancellationError` is now correctly thrown instead of returning `nil` on eager cancelation. Generic `Failure` type is deprecated.
Motivation:
- Uses Concurrency but missing availability requirements; does not build
on macOS.
Modifications:
- Add availability requirements/guards
Result:
Compiles on macOS
# Motivation
We landed the async bridge types a while back but never added allocation and performance tests. Since we expect these types to be used performance critical paths we really should cover those with tests.
# Modification
Extends the allocation counter scaffolding to support async tests. Furthermore, add allocations tests for both the writer and producer. Lastly, I a also added a performance test for the producer.
# Result
We now have baseline tests for the `NIOAsyncWriter` and `NIOAsyncSequenceProducer`
Co-authored-by: Cory Benfield <lukasa@apple.com>
* Implement a `NIOAsyncWriter`
# Motivation
We previously added the `NIOAsyncProducer` to bridge between the NIO channel pipeline and the asynchronous world. However, we still need something to bridge writes from the asynchronous world back to the NIO channel pipeline.
# Modification
This PR adds a new `NIOAsyncWriter` type that allows us to asynchronously `yield` elements to it. On the other side, we can register a `NIOAsyncWriterDelegate` which will get informed about any written elements. Furthermore, the synchronous side can toggle the writability of the `AsyncWriter` which allows it to implement flow control.
A main goal of this type is to be as performant as possible. To achieve this I did the following things:
- Make everything generic and inlinable
- Use a class with a lock instead of an actor
- Provide methods to yield a sequence of things which allows users to reduce the amount of times the lock gets acquired.
# Result
We now have the means to bridge writes from the asynchronous world to the synchronous
* Remove the completion struct and incorporate code review comments
* Fixup some refactoring leftovers
* More code review comments
* Move to holding the lock around the delegate and moved the delegate into the state machine
* Comment fixups
* More doc fixes
* Call finish when the sink deinits
* Refactor the writer to only yield Deques and rename the delegate to NIOAsyncWriterSinkDelegate
* Review
* Fix some warnings
* Fix benchmark sendability
* Remove Failure generic parameter and allow sending of an error through the Sink
* NIOPerformanceTester: Add DeadlineNowBenchmark for NIODeadline.now()
Signed-off-by: Si Beaumont <beaumont@apple.com>
* fixup: Return counter from benchmark
Signed-off-by: Si Beaumont <beaumont@apple.com>
* Use unbuffered IO for stdout in NIOPerformanceTester
Signed-off-by: Si Beaumont <beaumont@apple.com>
* CRASH PERF TESTS: Up task count to 1M to test logging change
* Revert "CRASH PERF TESTS: Up task count to 1M to test logging change"
This reverts commit d8fa50eb15.
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
Protocols like Sequence have "private" hooks that can be implemented to
provide fast-paths for some operations. We missed a few on BBV, and I'd
like to add them. This is one use-case where they can help.
Modifications:
- Add an allocation-counter benchmark and a runtime benchmark for
copying BBV to Array.
Result:
We have some benchmarks.
### Motivation:
In issue https://github.com/apple/swift-nio/issues/1316, we see a large number of allocations to happen when scheduling tasks. This can definitely be optimized. This PR adds a number of baseline allocation and performance tests for both `scheduleTask` and `execute`. In the next PRs, I am going to try a few optimizations to reduce the number of allocations.
### Modifications:
Added baseline performance and allocation tests for `scheduleTask` and `execute`
Motivation:
To judge the cost of `PTHREAD_MUTEX_ERRORCHECK` as well as
`os_unfair_lock` vs `pthread_mutex_t` it's useful to have a few simple
benchmarks.
Modification:
Add lock benchmarks.
Result:
Better data.
Motivation:
Many network protocols (especially for example NFS) have quite a number
of integer values next to each other. In NIO, you'd normally parse/write
them with multiple read/writeInteger calls.
Unfortunately, that's a bit wasteful because we're checking the bounds
as well as the CoW state every time.
Modifications:
- Provide read/writeMultipleIntegers for up to 15 FixedWidthIntegers.
- Benchmarks
Result:
Faster code. For 10 UInt32s, this is a 5x performance win on my machine,
see benchmarks.
Motivation:
To justify performance changes we need to measure the code being
changed. We believe that `HTTPHeaders.subscript(canonicalForm:)` is a
little slow.
Modifications:
- Add allocation and performance tests for fetching header values in
their canonical form
Results:
More benchmarks!
Motivation:
The remaining NIO code really conceptually belongs in a module called
NIOPosix, and NIOCore should really be called NIO. We can't really do
that last step, but we can prepare by pushing the bulk of the remaining
code into a module called NIOPosix.
Modifications:
- Move NIO to NIOPosix
- Make NIO an umbrella module.
Result:
NIOPosix exists.
Motivation:
As we've largely completed our move to split out our core abstractions,
we now have an opportunity to clean up our dependencies and imports. We
should arrange for everything to only import NIO if it actually needs
it, and to correctly express dependencies on NIOCore and NIOEmbedded
where they exist.
We aren't yet splitting out tests that only test functionality in
NIOCore, that will follow in a separate patch.
Modifications:
- Fixed up imports
- Made sure our protocols only require NIOCore.
Result:
Better expression of dependencies.
Co-authored-by: George Barnett <gbarnett@apple.com>
* Add performance test for web socket client random request key
* use `reduce` function to minimise memory consumption of the test
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
In #1733 I accidentally commented out a perf benchmark (who knows why).
Modification:
Bring it back.
Result:
Benchmark is back.
Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
B2MD called out to the decoder's `shouldReclaimBytes` after every
parsing attempt, even if the parser said `.continue`.
That's quite pointless because we won't add any bytes into the buffer
before we're trying the parser again.
Modifications:
Only ask the decoder if we should reclaim bytes if the decoder actually
says `.needMoreData`.
Result:
Faster, better, more sensible.
Motivation:
I'm sick of typing `.init(major: 1, minor: 1)`.
Modifications:
- Added static vars for common HTTP versions.
Result:
Maybe I'll never type `.init(major: 1, minor: 1)` ever again.
Motivation:
Many other systems don't like non ascii characters.
Modifications:
Change a letter 'a' to a letter 'a' but with a more normal encoding.
Result:
Test names should be in ascii
Motivation:
In all contemporary Swift versions, the `class` and the `AnyObject`
protocol restriction is the same. And `class` is deprecated which warns
on newer Swift compilers.
Modifications:
Replace `class` with `AnyObject`.
Result:
No warnings on newer Swift compilers.
Motivation:
The names of most of the BSD socket option values were brought over into
their NIO helper properties, with their namespace removed. This vastly
increases the risk of collision, particularly for things like TCP_INFO,
which just became .info.
Modifications:
- Added the prefixes back, e.g. `.info` is now `.tcp_info`.
- Renamed socket type `.dgram` to `.datagram`, as this is the nice clean
API and we should use nice clean names.
Result:
Better APIs
* NIO: Add new `BSDSocket` namespace
This starts the split of the POSIX/Linux/Darwin/BSD interfaces and the
BSD Socket interfaces in order to support Windows. The constants on
Windows are not part of the C standard library and need to be explicitly
prefixed. Use the import by name to avoid the `#if` conditions on the
wrapped versions. This will allow us to cleanly cleave the dependency
on the underlying C library.
This change adds a `BSDSocket.OptionLevel` and `BSDSocket.Option` types
which allow us to get proper enumerations for these values and pipe them
throughout the NIO codebase.
* Apply suggestions from code review
Co-Authored-By: Cory Benfield <lukasa@apple.com>
Motivation:
getInteger used the normal, generic implementation for UInt8s, however
they can be made considerably faster by using a specialised method.
ByteBufferIterator was also affected by this.
Modifications:
Use a specialised version for UInt8s for getInteger.
Result:
- ByteBufferView can be iterated about 20% faster.
Kushal Pisavadia
Marc Prud'hommeaux
Johannes Weiss
Roman A
George Barnett
Ayush Garg
Franz Busch
ser
Gregor Milos (Grzegorz Miłoś)
David Nadoba
carolinacass
Si Beaumont
Sebastian Vogt
Maxim ZaksandCory Benfield
Peter Adams
Saleem AbdulrasoolandCory Benfield