feat: Add support for Wasm compilation and CI using the Swift SDK for WebAssembly#190
Conversation
|
@MaxDesiatov @kateinoigakukun Mentioning you on this PR as FYI in case you're interested or have feedback. |
Sources/CoreMetrics/Locks.swift
Outdated
| #if os(Windows) | ||
| internal final class Lock: @unchecked Sendable { | ||
| #if canImport(WASILibc) | ||
| // WASILibc is single threaded, provides no locks |
There was a problem hiding this comment.
This is technically not true, WASILibc is orthogonal to threading, and it does provide a pthread-compatible API that works in both single-threaded in multi-threaded environments. For compatibility with multi-threaded WASI, I suggest using Mutex from import Synchronization instead, if possible. That would make it work not just for both single-threaded and multi-threaded WASI, but all major platforms supported by Swift.
There was a problem hiding this comment.
@ktoso do you think that replacing this ad-hoc Lock type with Mutex from Synchronization across the codebase of swift-metrics is feasible?
There was a problem hiding this comment.
Mutex needs to be used carefully on Wasm targets because it will raise an exception when it needs to wait on the main thread. But in this case, the lock is taken only for bootstrapping the factory storage; it is unlikely to fail to acquire, and library users are capable of controlling which thread the lock will be acquired, so I think it makes sense to use Mutex here.
There was a problem hiding this comment.
Mutex has high platform requirements, which we can't bump without a major version here, so we need to stick with some custom type here.
We should just copy Swift Log's Locks.swift, which already solved it, as it supports Wasm now.
There was a problem hiding this comment.
It does seem to be a copy from swift-log, but my point still stands: the current implementation for WASI there has no support for multi-threading and the comment about "no locking" is not technically correct.
There was a problem hiding this comment.
Ideally, we should use import wasi_pthread and fix it both here and in swift-log. NIO already has an implementation for wasi_pthread https://github.com/apple/swift-nio/blob/main/Sources/NIOConcurrencyHelpers/lock.swift
There was a problem hiding this comment.
@MaxDesiatov @czechboy0 @kateinoigakukun It looks like the Lock implementation is a copy paste of the lock.swift in NIO, and the ReadWriteLock is a copy-paste of the implementation in swift-log or swift-distributed tracing.
I'm happy to replicate the implementation in all those places. Is there a preferred order? It looks like I should start in swift-log, get that reviewed and merged, then copy-paste verbatim into swift-metrics and swift-distributed tracing.
For ReadWriteLock, it will have to be refactored to follow the import wasi_pthread pattern, since none of the current implementations have wasi_pthread handling.
For Lock, any preference on the following options:
A) Refactor the current implementation in this PR to follow the import patterns used in the nio lock implementation Max provided?
B) Copy-paste the latest Lock implementation verbatim from NIO, but remove the deprecation?
C) Use the latest NIOLock with a type alias to map NIOLock to Lock.
There was a problem hiding this comment.
I'm happy to replicate the implementation in all those places. Is there a preferred order? It looks like I should start in swift-log, get that reviewed and merged, then copy-paste verbatim into swift-metrics and swift-distributed tracing.
Yes, that's the right order.
For Lock, any preference on the following options:
I think we should stick as closely as possible to whatever NIO is doing, so B), just keep the type internal so no need to deprecate the old name.
There was a problem hiding this comment.
@czechboy0 @MaxDesiatov @kateinoigakukun I created the first PR to update the lock implementation for swift-log:
It's in draft at the moment. Want to run a few more checks.
There was a problem hiding this comment.
@czechboy0 apple/swift-log#398 has been merged, and I've updated this PR to match the Locks.swift file in swift-log.
8bc0433 to
0fad628
Compare
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #if canImport(WASILibc) |
There was a problem hiding this comment.
Note: This implementation is a verbatim copy paste of the change in https://github.com/apple/swift-log/pull/398/changes
The only difference is the project name in the file header comments
0fad628 to
2351eab
Compare
|
@czechboy0 I've rebased this PR on the latest Is there anything else needed for this PR? |
67b652f to
ad772d6
Compare
… lock implementation to match the latest implementation in swift-log provided by apple/swift-log#408.
ad772d6 to
95a6857
Compare
|
Updated this with latest changes in swift-log that will be introduced with apple/swift-log#408 |
|
Please update PR title to use "Wasm" instead of "WASM" before merging to use correct spelling per the Wasm spec: https://webassembly.github.io/spec/core/intro/introduction.html#wasm |
Motivation:
As part of expanding platform and CI coverage of Swift Configuration, Swift Metrics should also build to Wasm using the Swift SDK for WebAssembly.
Modifications:
Brought over locking changes from Swift Log to support Wasm.
Conditionally elided some APIs that require Dispatch when Dispatch isn't available.
Result:
The package now builds against the Wasm SDK, both normal and the test targets.
Important
This PR is a cherry pick of #183 created by @czechboy0. It has been modified to resolve conflicts and is rebased on the latest
main.