Skip to content

[Concurrency] Build executors as their own separate object modules. #76483

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 2, 2024
Merged

[Concurrency] Build executors as their own separate object modules. #76483

merged 7 commits into from
Oct 2, 2024

Conversation

al45tair
Copy link
Contributor

@al45tair al45tair commented Sep 16, 2024 •
edited
Loading

We want to split the global executor implementations out into their own separate object files, because that will allow us to provide a custom executor specific to the target platform when using Embedded Concurrency.

rdar://135380149

@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair al45tair marked this pull request as ready for review September 20, 2024 15:04
@al45tair al45tair requested a review from a team as a code owner September 20, 2024 15:04
@al45tair
Copy link
Contributor Author

Hmmm. Looks like I've broken a few tests there :-) More to do, it seems.

ktoso
ktoso approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

AFAICS this looks pretty right. The new header looks good and has all the hooks I'd expect.

@al45tair
Copy link
Contributor Author

@swift-ci Please test

rjmccall
rjmccall reviewed Sep 24, 2024
View reviewed changes
Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

To what extent is this protocol for async main (the drain / donate system) already ABI vs. just an internal detail of the concurrency library? That's the main thing that stands out to me — that we're effectively making this pseudo-ABI (admittedly still something we reserve the right to change in later releases), and we should consider what clients are likely to actually want to take over.

@al45tair
Copy link
Contributor Author

@rjmccall That's an interesting point. I think swift_task_asyncMainDrainQueue() is actually ABI, in that a call to it is emitted by the compiler when it builds the async_Main entry point in SILGenFunction::emitAsyncMainThreadStart(). I'm less sure about swift_task_donateThreadToGlobalExecutorUntil(), in that I don't see that being emitted by the compiler (but that might just be me failing to find it)… I assume that was added for some embedded use-case or other?

@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair
Copy link
Contributor Author

@swift-ci Please test

@rjmccall
Copy link
Contributor

@rjmccall That's an interesting point. I think swift_task_asyncMainDrainQueue() is actually ABI, in that a call to it is emitted by the compiler when it builds the async_Main entry point in SILGenFunction::emitAsyncMainThreadStart(). I'm less sure about swift_task_donateThreadToGlobalExecutorUntil(), in that I don't see that being emitted by the compiler (but that might just be me failing to find it)… I assume that was added for some embedded use-case or other?

Yeah, it's otherwise impossible to actually drain the queue.

Okay, if asyncMainDrainQueue is called directly by the compiler, then it's what we've got to live with.

@kubamracek kubamracek mentioned this pull request Sep 27, 2024
Merged
6 tasks
@al45tair
Copy link
Contributor Author

@swift-ci Please test macOS platform

@al45tair
[Concurrency] Refactor executors so they are in their own object module.
efe5d66 
C++ executor implementations were `#include`ed into `GlobalExecutor.cpp`,
which makes it difficult to replace the global executor when using the
Embedded Concurrency library.  Refactor things so that they build into
separate objects, which means replacing them is just a matter of writing
the relevant functions yourself.

rdar://135380149
@al45tair
[Concurrency] Rename ExecutorHooks.h, tidy up the interface.
26b5fa6 
`ExecutorHooks.h` is now nothing to do with hooks, so rename it.  Also
there are some additional functions it should declare, and a couple of
places where we've slightly messed up the boundary, for instance
`swift_task_asyncMainDrainQueue` was defined in `Task.cpp` rather than
in the executor implementations, which is wrong, so fix that too.

`CooperativeGlobalExecutor.cpp` now builds against the interface from
`ExecutorImpl.h`, rather than including the all the concurrency headers.

rdar://135380149
@al45tair
[Concurrency] Install ExecutorImpl.h in include/swift in toolchain.
4ef4305 
When Embedded Concurrency is enabled, install `ExecutorImpl.h` in the
`include/swift` directory in the toolchain.

rdar://135380149
@al45tair
[Concurrency] Add a test for the new custom executor feature.
31df4ed 
Added an executor implementation and a test that tries to use it.
Also fix up a few issues in `ExecutorImpl.h`.

rdar://135380149
@al45tair
[Concurrency] Fix test failures, add struct layout checks.
837513d 
We were missing a field in `SwiftJob`, which broke various things.  To
avoid that problem in future, this PR adds a set of static asserts to
check the layout of various structures and that we're using the same
values as well.

Also added some functions to the ABI, and fixed things so that if you
enable the debug logging the library still builds (there was an extra
`_` in `Actor.cpp` that caused a build failure).

Finally, renamed `Hooks.cpp` to `ConcurrencyHooks.cpp`.

rdar://135380149
@al45tair
[Concurrency] Fix Linux/Windows build issues.
e6882f2 
Define `NOMINMAX` and `WIN32_LEAN_AND_MEAN` when including `<Windows.h>`.

Don't export the interface from DLLs.

Make sure we include `<new>` when using placement operator new.

rdar://135380149
@al45tair
Copy link
Contributor Author

Rebased.

@al45tair
Copy link
Contributor Author

@swift-ci Please test

@al45tair
[Concurrency] Fix macOS build issue.
cf77aa3 
We need to use the `swift` namespace in `NonDispatchGlobalExecutor.cpp`.

rdar://135380149
@al45tair
Copy link
Contributor Author

al45tair commented Oct 1, 2024

@swift-ci Please test

@rjmccall
Copy link
Contributor

rjmccall commented Oct 1, 2024

Okay. Ideally I think that entry point would purely be an additional function offered when we build in that mode. And just in general I don't think we care about supporting any of the patching mechanisms in the embedded mode that uses this — they're really only there for the stable-ABI platforms.

@al45tair al45tair merged commit f8d6012 into swiftlang:main Oct 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmccall rjmccall rjmccall left review comments

@ktoso ktoso ktoso approved these changes

@kubamracek kubamracek Awaiting requested review from kubamracek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@al45tair @rjmccall @ktoso