-
Notifications
You must be signed in to change notification settings - Fork 10.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Concurrency] Provide a Swift interface for custom main and global executors. #79789
Conversation
@swift-ci Please smoke test |
(This is not yet complete; there's a problem I need to resolve relating to setting the main executor.) |
5ac5fad
to
aeae636
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments so far; overall this is looking great :)
@swift-ci Please test |
d861a9f
to
8ef09ef
Compare
@swift-ci Please test |
b4f96b8
to
ff1575a
Compare
@swift-ci Please test |
1 similar comment
@swift-ci Please test |
ca24acd
to
47efda5
Compare
@swift-ci Please test |
Reorganise the Concurrency code so that it's possible to completely implement executors (both main and global) in Swift. Provide API to choose the desired executors for your application. Also make `Task.Sleep` wait using the current executor, not the global executor, and expose APIs on `Clock` to allow for conversion between time bases. rdar://141348916
Added an `-executor-factory` argument to the compiler to let you safely specify the executors you wish to use (by naming a type that returns them). Also added some tests of the new functionality. rdar://141348916
Remove the hacky support for mapping clocks to a Dispatch clock ID, in favour of clocks publishing traits and having the Dispatch executor select the clock on the basis of those traits.
Fix a couple of potential ABI breaks. Also add the new functions and types to the baseline lists. rdar://141348916
The rename appears to have had some kind of potentially ABI breaking consequence. rdar://141348916
Remove a printf() I'd added for debugging. rdar://141348916
Embedded Swift doesn't have MainActor, so remove it. rdar://141348916
We can't use blocks, because Swift doesn't support them on Linux or Windows. Instead, use a C function pointer, and box up the handler. rdar://141348916
Rename `DispatchTaskExecutor` to `DispatchGlobalTaskExecutor` as we may want to use the former for an executor that runs things on an arbitrary Dispatch queue. Rename `DispatchExecutor` to `DispatchExecutorProtocol`; again, we might want the name for something else. Add `@Sendable` attribute to `registerEvent`. Fix missing `extern "C" SWIFT_CC(swift)` on `_swift_exit` (merge error). Remove stray whitespace from `CMakeLists.txt` rdar://141348916
Also fix a missing availability annotation. rdar://141348916
This needed an update to make things build on Linux and Windows. rdar://141348916
Remove `supportsScheduling` in favour of a type-based approach. Update the storage for `ClockTraits` to `UInt32`. Adjust ordering of executors for `currentExecutor`. rdar://141348916
EventableExecutor is being removed, for now, but hopefully will return in some form in the future. The `asSchedulable` implementation needs to change for reasons of ABI stability. rdar://141348916
SE proposal is here: swiftlang/swift-evolution#2654 |
WASI concurrency tests failed, which is not a great surprise, as I've only just discovered that those were a thing. |
Also tweak the sleep implementations to let the hooks run if there isn't a `SchedulableExecutor` (for hooked mode). rdar://141348916
@swift-ci Please test |
Fix up availability after the non-Darwin changes. Also update the ABI baseline. Fix a Win32 typo. rdar://141348916
@swift-ci Please test |
We need to annotate the `timestamp()` function for watchOS (apparently not for macOS for some reason). rdar://141348916
Fix a typo that caused us to not include the correct code for platforms that use "singlethreaded" concurrency. Also use `var` not `let` for the computed property in `PlatformExecutorFactory`. rdar://141348916
We need to explicitly require `libdispatch` here, otherwise the Windows build will fail. rdar://141348916
@swift-ci Please test |
CooperativeExecutor needs to store its timestamp out-of-line for 32-bit architectures (like WASI). rdar://141348916
@swift-ci Please test |
`CooperativeExecutor` should fall back to heap allocations if the task allocator isn't available when it's enqueuing delayed jobs. Also, remove the reference to `DispatchGlobalExecutor` from the custom executor test. rdar://141348916
@swift-ci Please test |
Fix a couple of `unsafe` warnings. Also update `withUnsafeExecutorPrivateData` to use typed throws instead of `rethrow`. rdar://141348916
@swift-ci Please test |
@swift-ci Please test Linux platform |
OK. Everything is now passing except for the minimalstdlib configuration. |
When in task-to-thread model concurrency mode, there is no `MainActor` and we cannot use `ExecutorJob`, so disable various things. rdar://141348916
@swift-ci Please test |
@swift-ci Please test Windows platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spotted a couple of mistakes in comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just small things here and there, feel free to merge and let's follow up on what makes sense to address 👍
@@ -80,6 +80,7 @@ FEATURE(IsolatedDeinit, (6, 1)) | |||
|
|||
FEATURE(ValueGenericType, (6, 2)) | |||
FEATURE(InitRawStructMetadata2, (6, 2)) | |||
FEATURE(CustomExecutors, (6, 2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is CustomGlobalExecutors? Custom executor sounds like the actor ones hm...
Btw, i noticed TaskExecutor is FUTURE...? That's not right is it? 🤔
ERROR(cannot_find_executor_factory_type, none, | ||
"the specified executor factory '%0' could not be found", (StringRef)) | ||
ERROR(executor_factory_must_conform, none, | ||
"the executor factory '%0' does not conform to 'ExecutorFactory'", (Type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, your call: Maybe better to get the type name passed in?
@@ -507,6 +511,46 @@ FuncDecl *SILGenModule::getExit() { | |||
return exitFunction; | |||
} | |||
|
|||
Type SILGenModule::getExecutorFactory() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this maybe getConfiguredExecutorFactory
?
ProtocolDecl *SILGenModule::getExecutorFactoryProtocol() { | ||
auto &ctx = getASTContext(); | ||
|
||
return ctx.getProtocol(KnownProtocolKind::ExecutorFactory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this func? There should be ctx.getExecutorFactoryDecl()
generated since you added it to KnownProtocols.def
return SerialExecutorRef::generic(); | ||
return executor; | ||
} | ||
return swift_getMainExecutor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really right? if yes, can you add a comment?
Say we're just on some random thread, there's no executor tracking, we're not on the main executor though -- this seems a bit off?
0, "Incorrect actor executor assumption; expected '%.*s' executor.\n", | ||
(int)typeName.length, typeName.data); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thanks for moving these over to ExecutorBridge now
/// | ||
/// N.B. Because this allocator is stack disciplined, explicitly | ||
/// deallocating memory will also deallocate all memory allocated | ||
/// after the block being deallocated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I had this question before already: that's now how we behave today, if you deallocate "in wrong order" we crash; are we really changing that? That was good for detecting we messed up somehow, while deallocating other things like this may hide bugs in stack discipline...?
= DispatchGlobalTaskExecutor() | ||
} | ||
|
||
#endif // os(Linux) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how those Platform...swift look now 👍
//if (auto identity = reinterpret_cast<HeapObject *>(jobQueue)) { | ||
// return SerialExecutorRef::forOrdinary( | ||
// identity, _swift_task_getDispatchQueueSerialExecutorWitnessTable()); | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
@@ -0,0 +1,92 @@ | |||
// RUN: %target-run-simple-swift(-Xfrontend -disable-availability-checking -g %import-libdispatch -parse-as-library -executor-factory SimpleExecutorFactory) | %FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a test specifying the executor factory in another module?
Sounds good. Will merge and then make a new PR to address the various things noted on this one. |
Reorganise the Concurrency code so that it's possible to completely implement executors (both main and global) in Swift.
Provide API to choose the desired executors for your application.
Also make
Task.Sleep
wait using the current executor, not the global executor, and expose APIs onClock
to allow for conversion between time bases.rdar://141348916