feat: Update lock implementation to more closely align with swift-nio implementation#398
Conversation
|
Note, leaving this PR in draft at the moment while I finish checking on a few things. I expect to take it out of draft next week. |
Ok, I believe everything is in order. @MaxDesiatov @czechboy0 @kateinoigakukun please feel free to review. |
kateinoigakukun
left a comment
There was a problem hiding this comment.
Thanks! The Wasm part of the change seems good to me!
|
@scottmarchant I can't seem to be able to update the PR with the latest changes, can you do it manually please? |
czechboy0
left a comment
There was a problem hiding this comment.
Approving in this form to make incremental progress, but please make the suggested improvements in the NIO version, especially the potential leak of the attributes.
…entation in swift-nio. This improves support for Swift for WebAssembly toolchains that support pthreads.
0ccf2cf to
da7d3f3
Compare
@czechboy0 This is rebased on the tip of main now. |
… pick czechboy0's initial wasm support provided in apple#183 with merge conflicts resolved. This also contains the latest Lock implementations from NIO, duplicated first in swift-log: apple/swift-log#398
This change was found and requested by czechboy0 when reviewing a copy of this file propagated to swift-log apple/swift-log#398 (comment).
|
@czechboy0 The PR to add the missing |
|
Thanks @scottmarchant - and for the change to avoid allocating the lock completely on single-threaded Wasm, will that be done separately? |
For single-threaded Wasm I'd expect those function calls to become no-ops, ideally eliminated completely in release mode through aggressive inlining and DCE. If not, please raise a bug on the compiler repo and ping me, we certainly expect this optimization to happen, similarly to how debug loging statements can be eliminated in release mode. |
|
My question is more about the explicit unsafe pointer allocate/deallocate calls stored in the Lock struct. I don't assume that'd get optimized away? |
…utexattr_destroy in Lock (#3480) ### Motivation: In reviewing a copy of this file in swift-log, @czechboy0 noticed that the call to `pthread_mutexattr_destroy` is missing. Since pthread_mutexattr_t is like allocated on the stack rather than heap on most platforms, this is likely not a memory leak. But for correctness the missing destroy call should be added. This change is the start of a resolution [requested](apple/swift-log#398 (comment)) in a corresponding Lock rollout in swift-log ### Modifications: - Added missing call to `pthread_mutexattr_destroy` - Duplicated test for `NIOLock` to also test `Lock`, as a sanity test. ### Result: The new sanity test passes. If there are other ways this change can be realistically verified as safe and proper, please feel free to provide feedback in the PR.
|
@czechboy0 Here is the PR to avoid allocation altogether. Needed to do a little research to feel a bit more confident the change wouldn't adversely affect platforms that were relying on the unconditional |
|
I think/hope that @MaxDesiatov is correct about the allocation being optimized out by the compiler. But the unconditional deallocate call in the deinit makes me wonder if the compiler would be able to realize the variable is never used, since it might appear to be used in the deinit. Either way, figured the variable should be completely elided (conditionally) anyways to prevent someone from trying to use it down the road without realizing there may be missing mutex initializations for certain platforms. |
### Motivation: In reviewing a copy of this file in swift-log, @czechboy0 [noticed](apple/swift-log#398 (comment)) that the `mutex` variable is essentially unused and unnecessarily allocated for certain conditions. It is possible the compiler elided usage. But given the unconditional variable reference in the `deinit`, the compiler may not be able to. This change ensures the `mutex` property is completely elided except in the conditions where it is used. ### Modifications: - Adjusted compiler directive pattern to completely elide the `mutex` property for conditions where it would be unused. - Adjusted `deinit` to avoid referencing `mutex` unless it is compiled into the `Lock` class ### Result: The `mutex` property is no longer allocated or even compiled into the `Lock` class for unsupported configurations. All unit tests and checks pass. ### Research: The `deinit` used to have a blanket call to `mutex.deallocate()`. This has been moved into the platform-specific checks within the `deinit`. This allows complete elision of the `mutex` property altogether. The `_runtime(_multithreaded)` condition [appears to be rooted to this implementation](https://github.com/swiftlang/swift/blob/ffc51b914602765c5d680241796dbd3c3711fa6b/lib/Basic/LangOptions.cpp#L490). Diving deeper, all [operating systems in this list](https://github.com/swiftlang/swift/blob/ffc51b914602765c5d680241796dbd3c3711fa6b/lib/Basic/LangOptions.cpp#L554) except for UnknownOS and WASI result in `_runtime(_multithreaded)` returning true. That means that `OpenBSD` (and many other operating systems besides windows) were almost certainly using the `#elseif (compiler(<6.1) && !os(WASI)) || (compiler(>=6.1) && _runtime(_multithreaded))` condition in the `deinit`. In conclusion, moving `mutex.deallocate()` up into the conditional clauses inside `deinit` should be identical to the previous implementation for all operating systems except `llvm::Triple::UnknownOS`. And `llvm::Triple::UnknownOS` is likely not to have every compiled anyways due to the [check on line 32](https://github.com/apple/swift-nio/blob/main/Sources/NIOConcurrencyHelpers/lock.swift#L32). So in summary, it is expected that this clean up will compile identical to the previous implementation for all platforms except WASI platforms that don't have pthread support. For non-pthread WASI platforms, the unused `mutex` is properly elided from compilation altogether. ### Testing Done: - Confirmed that unit tests pass locally using Xcode - Verified [PR checks pass](https://github.com/PassiveLogic/swift-nio/actions/runs/21149729751)
… pick czechboy0's initial wasm support provided in apple#183 with merge conflicts resolved. This also contains the latest Lock implementations from NIO, duplicated first in swift-log: apple/swift-log#398
This PR adds support for FreeBSD to the `CNIOSHA1` target and the `NIOConcurrencyHelpers` target. ### Motivation: This PR is partial because the `NIOCore` target still can't be built, and the existing PR #3398 includes almost the same changes too. However, #3398 is a little bit old. Besides, the source code in `NIOConcurrencyHelpers` is used in other repositories now, for example: - apple/swift-log#398 - apple/swift-log#387 was merged before swift-nio is fixed. So I think it's important to fix `NIOConcurrencyHelpers` first, even if the entire swift-nio is yet to be fixed. ### Modifications: - Make `swift build --target NIOConcurrencyHelpers` work on FreeBSD - Make `swift build --target CNIOSHA1` work on FreeBSD ### Result: The following commands finished successfully. ```shell swift build --target NIOConcurrencyHelpers swift build --target CNIOSHA1 ``` Environment: - FreeBSD 15.0-RELEASE-p1 x86_64 - [Swift on FreeBSD Preview toolchain](https://forums.swift.org/t/swift-on-freebsd-preview/83064) ``` Swift version 6.3-dev (LLVM b58b2a34d509492, Swift cf535d8b998d09b) Target: x86_64-unknown-freebsd14.3 Build config: +assertions ``` --------- Co-authored-by: Cory Benfield <lukasa@apple.com>
Motivation:
The swift-nio lock implementation differs from this older copy-paste of the implementation. This PR brings over the latest changes while preserving any deviations.
Besides alignment, this change improves the wasi pthread support, which was previously elided for wasm SDK's that support pthreads.
Modifications:
Lockimplementation over from swift-nioLockto `ReadWriteLockDeviations from swift-nio:
packageinstead ofpublicACL specifiersLockto `ReadWriteLockResult:
Testing done:
Related work
This work is driven by a larger effort by PassiveLogic to improve Wasm support across many swift repos.
This specific PR was requested as foundational work to support apple/swift-metrics#190.