Skip to content

feat: Update lock implementation to more closely align with swift-nio implementation#398

Merged
czechboy0 merged 1 commit intoapple:mainfrom
PassiveLogic:feat/swift-wasm-wasipthread-support
Jan 15, 2026
Merged

feat: Update lock implementation to more closely align with swift-nio implementation#398
czechboy0 merged 1 commit intoapple:mainfrom
PassiveLogic:feat/swift-wasm-wasipthread-support

Conversation

@scottmarchant
Copy link
Copy Markdown
Contributor

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:

  • Copied Lock implementation over from swift-nio
  • Extended the conditional patterns used in Lock to `ReadWriteLock

Deviations from swift-nio:

  • Preserved Android condtional, which is missing from swift-nio
  • Preserved package instead of public ACL specifiers
  • Extended the conditional patterns used in Lock to `ReadWriteLock

Result:

  • Existing tests and builds still pass
  • Mutex protection is no longer eliminated for wasm pthread targets.

Testing done:

  • Verified locally that wasm compilation still works using the following commands:
swift build --swift-sdk swift-6.3-DEVELOPMENT-SNAPSHOT-2026-01-06-a_wasm --target Logging
swift build --swift-sdk swift-6.3-DEVELOPMENT-SNAPSHOT-2026-01-06-a_wasm --target InMemoryLogging
  • Verified unit tests all pass locally when running packages tests in Xcode
  • Verified all expected CI checks pass in separate temporary PR:

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.

@scottmarchant
Copy link
Copy Markdown
Contributor Author

scottmarchant commented Jan 10, 2026

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.

@scottmarchant
Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The Wasm part of the change seems good to me!

@czechboy0 czechboy0 added the 🔨 semver/patch No public API change. label Jan 14, 2026
@czechboy0
Copy link
Copy Markdown
Contributor

@scottmarchant I can't seem to be able to update the PR with the latest changes, can you do it manually please?

Copy link
Copy Markdown
Contributor

@czechboy0 czechboy0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@scottmarchant scottmarchant force-pushed the feat/swift-wasm-wasipthread-support branch from 0ccf2cf to da7d3f3 Compare January 14, 2026 20:59
@scottmarchant
Copy link
Copy Markdown
Contributor Author

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.

@czechboy0 This is rebased on the tip of main now.

scottmarchant added a commit to PassiveLogic/swift-metrics that referenced this pull request Jan 14, 2026
… 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
@czechboy0 czechboy0 enabled auto-merge (squash) January 15, 2026 08:02
@czechboy0 czechboy0 merged commit a92ccc6 into apple:main Jan 15, 2026
68 checks passed
scottmarchant added a commit to PassiveLogic/swift-nio that referenced this pull request Jan 15, 2026
This change was found and requested by czechboy0 when reviewing a copy of this file propagated to swift-log apple/swift-log#398 (comment).
@scottmarchant
Copy link
Copy Markdown
Contributor Author

@czechboy0 The PR to add the missing pthread_mutexattr_destroy in swift-nio is available now. Please feel free to take a look. Wouldn't mind a few eyes on this given the low-level nature of the change.

apple/swift-nio#3480

@scottmarchant scottmarchant deleted the feat/swift-wasm-wasipthread-support branch January 15, 2026 18:37
@czechboy0
Copy link
Copy Markdown
Contributor

Thanks @scottmarchant - and for the change to avoid allocating the lock completely on single-threaded Wasm, will that be done separately?

@MaxDesiatov
Copy link
Copy Markdown
Member

MaxDesiatov commented Jan 16, 2026

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.

@czechboy0
Copy link
Copy Markdown
Contributor

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?

Lukasa pushed a commit to apple/swift-nio that referenced this pull request Jan 19, 2026
…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.
@scottmarchant
Copy link
Copy Markdown
Contributor Author

@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 mutex.deallocate(). I'm pretty sure the changes are right there, but I wouldn't mind another set of eyes. Please feel free to take a look: apple/swift-nio#3483.

@scottmarchant
Copy link
Copy Markdown
Contributor Author

scottmarchant commented Jan 20, 2026

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.

Lukasa pushed a commit to apple/swift-nio that referenced this pull request Jan 22, 2026
### 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)
scottmarchant added a commit to PassiveLogic/swift-metrics that referenced this pull request Feb 3, 2026
… 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
Lukasa added a commit to apple/swift-nio that referenced this pull request Mar 18, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants