Introduce basic support for custom executors #36878

rjmccall · 2021-04-13T07:57:33Z

Introduce an UnownedSerialExecutor type into the concurrency library.
Create a SerialExecutor protocol which allows an executor type to change how it executes jobs.
Add an unownedExecutor requirement to the Actor protocol.
Change the ABI for Builtin.Executor (aka ExecutorRef, aka UnownedSerialExecutor) so that it stores a SerialExecutor witness table pointer in the implementation field. This effectively makes the type an unowned(unsafe) SerialExecutor, except that default actors are represented without a witness table pointer.
Synthesize the unownedExecutor method for default actors (i.e. actors that don't provide any other execution logic).
Make synthesized unownedExecutor properties @_resilientFinal, and give them a semantics attribute specifying that they're for default actors.
Split Builtin.buildSerialExecutorRef into a few more precise builtins. We're not using the main-actor one yet, though.

Pitch thread: https://forums.swift.org/t/support-custom-executors-in-swift-concurrency/44425

rjmccall · 2021-04-13T07:57:41Z

@swift-ci Please test

swift-ci · 2021-04-13T09:01:56Z

Build failed
Swift Test Linux Platform
Git Sha - c6560720b14230400410208ca3f7f642ef169c90

swift-ci · 2021-04-13T09:04:56Z

Build failed
Swift Test OS X Platform
Git Sha - c6560720b14230400410208ca3f7f642ef169c90

ktoso

I'm in no position to provide useful feedback on impl I guess, but read through and at least much more of it nowadays I understood :)

Some minor questions in line... only if you have the time, nothing urgent. Looking great overall!

stdlib/public/Concurrency/Executor.swift

stdlib/public/Concurrency/PartialAsyncTask.swift

lib/IRGen/GenConcurrency.cpp

stdlib/public/Concurrency/Executor.swift

stdlib/public/Concurrency/Actor.swift

lib/Sema/TypeCheckProtocol.cpp

lib/SILOptimizer/Mandatory/LowerHopToActor.cpp

rjmccall · 2021-04-14T08:04:01Z

@swift-ci Please test

swift-ci · 2021-04-14T09:02:32Z

Build failed
Swift Test OS X Platform
Git Sha - dca5f2233093245e7e44cc9dd07ead2c18ee7da1

swift-ci · 2021-04-14T09:07:35Z

Build failed
Swift Test Linux Platform
Git Sha - dca5f2233093245e7e44cc9dd07ead2c18ee7da1

rjmccall · 2021-04-14T19:38:34Z

@swift-ci Please test

swift-ci · 2021-04-14T21:04:34Z

Build failed
Swift Test Linux Platform
Git Sha - 6240c7362a194c9e9d9a5a3a120eb48c8414b1f6

swift-ci · 2021-04-14T21:13:56Z

Build failed
Swift Test OS X Platform
Git Sha - 6240c7362a194c9e9d9a5a3a120eb48c8414b1f6

rjmccall · 2021-04-15T04:26:37Z

@swift-ci Please test

swift-ci · 2021-04-15T06:29:12Z

Build failed
Swift Test OS X Platform
Git Sha - 374b29a554b7cf5d40adce12a49c1be6fb8640cb

jckarter · 2021-04-15T16:31:28Z

lib/SILGen/SILGenType.cpp

@@ -59,8 +59,13 @@ SILGenModule::emitVTableMethod(ClassDecl *theClass,
  // case we won't emit it if building a resilient module.
  SILVTable::Entry::Kind implKind;
  if (baseClass == theClass) {
-    // This is a vtable entry for a method of the immediate class.
-    implKind = SILVTable::Entry::Kind::Normal;
+    if (derivedDecl->isResilientFinal())


Can you reuse isNonOverridden here instead of adding a new SILVTable entry kind? I had started by trying to add a new kind, but I ended up needing to make the attribute orthogonal.

Can you remember why? The modeling seems much cleaner this way.

I don't completely remember the specifics, but the distinction between Normal and Inherited is significant, and when a method has no overrides but the class does have other subclasses, then it is perfectly valid to have an entry that is both inherited and non-overridden in the subclass.

jckarter · 2021-04-15T16:38:21Z

@_resilientFinal seems like it's nearly identical in runtime impact to [nonoverridden] for pruned vtable entries. It's nice to have an attribute to force a declaration to remain nonoverridden within its compilation unit, but I'm concerned about it having ABI impact making it externally distinguishable from a public non-open declaration without overrides, because that will be yet another edge case we need to deal with forever. It should be straightforward to add support for nonoverridden method descriptors using the representation from #36897, since you don't need to be concerned about backward deployment here so can rely on a modification to swift_lookUpClassMethod to honor a new attribute flag.

rjmccall · 2021-04-15T21:08:08Z

@swift-ci Please test

rjmccall · 2021-04-15T22:43:54Z

@_resilientFinal seems like it's nearly identical in runtime impact to [nonoverridden] for pruned vtable entries. It's nice to have an attribute to force a declaration to remain nonoverridden within its compilation unit, but I'm concerned about it having ABI impact making it externally distinguishable from a public non-open declaration without overrides, because that will be yet another edge case we need to deal with forever.

How do you feel this is distinguished? Just because it (might) forbid super dispatch and so doesn't expose a method descriptor? Because in every other way, I think this is treated identically.

Remember that we want to support back-deployment in short order, so that seems like a constraint we can't ignore.

swift-ci · 2021-04-15T23:10:16Z

Build failed
Swift Test OS X Platform
Git Sha - ebfcb3a054090dbd959700ff23d00c5e3267812c

jckarter · 2021-04-15T23:22:16Z

IIRC method descriptors are also referenced by external subclasses so we can resiliently build their vtable; would that ever be a concern here? To be clear, I don't think the backward deployment support here is particularly onerous; I had planned to address it in the linked PR by linking in a lookup method implementation that just does the "is nonoverridden" check first then forwards to the OS implementation.

rjmccall · 2021-04-16T22:09:51Z

@swift-ci Please test

rjmccall · 2021-04-16T22:25:22Z

IIRC method descriptors are also referenced by external subclasses so we can resiliently build their vtable; would that ever be a concern here?

I think we only do this with overrides, and of course these aren't overridable. I don't think you can safely back-deploy an override in any case. The existing runtime seems to fatalError if you have an override descriptor for something that's not within the bounds of the class's v-table methods; it gracefully degrades if the overridden method descriptor is null (i.e. is a weak import that's not present in the current class), but not if we have some sort of non-vtable descriptor.

I do see what I think is your point: that we don't semantically prevent super dispatch to arbitrary methods that you haven't overridden, and so the optimizer-driven use case still needs to allow such dispatch, and therefore needs to emit a method descriptor just in case. Method descriptors are quite small in practice, so I guess most of the code-size hit of allowing that would just be the symbol name being present in the export tables, plus the impact on the lookup function.

I wonder if it would be feasible to treat this as a language defect and ban super dispatch to something that you wouldn't be allowed to override in your module. Or we could make super-dispatchers recognize this case and try to weak-import the method descriptor, and if it's null they'd just fall back on calling the resilient dispatch function. Then the way to declare something as non-overridable in the ABI would specifically be to not export a method descriptor symbol for it.

swift-ci · 2021-04-17T00:13:27Z

Build failed
Swift Test OS X Platform
Git Sha - fcb9901f45639e4f17d62f10bcc3404a3be63efd

jckarter · 2021-04-19T17:36:15Z

I think we only do this with overrides, and of course these aren't overridable.

But your goal here is to allow this to become overridable in the future, no? Otherwise it would just be final. Going from public to open should already be ABI compatible.

rjmccall · 2021-04-29T08:01:41Z

@swift-ci Please test

swift-ci · 2021-04-29T09:25:01Z

Build failed
Swift Test Linux Platform
Git Sha - 6ac1f15d5f2bc3c45c34e0298a3beb7f8c452cde

swift-ci · 2021-04-29T09:54:01Z

Build failed
Swift Test OS X Platform
Git Sha - 6ac1f15d5f2bc3c45c34e0298a3beb7f8c452cde

rjmccall · 2021-04-29T20:19:16Z

@swift-ci Please test

swift-ci · 2021-04-29T22:34:55Z

Build failed
Swift Test OS X Platform
Git Sha - 6cea7478409401ba7c2b477da2ac921c15344174

- Introduce an UnownedSerialExecutor type into the concurrency library. - Create a SerialExecutor protocol which allows an executor type to change how it executes jobs. - Add an unownedExecutor requirement to the Actor protocol. - Change the ABI for ExecutorRef so that it stores a SerialExecutor witness table pointer in the implementation field. This effectively makes ExecutorRef an `unowned(unsafe) SerialExecutor`, except that default actors are represented without a witness table pointer (just a bit-pattern). - Synthesize the unownedExecutor method for default actors (i.e. actors that don't provide an unownedExecutor property). - Make synthesized unownedExecutor properties `final`, and give them a semantics attribute specifying that they're for default actors. - Split `Builtin.buildSerialExecutorRef` into a few more precise builtins. We're not using the main-actor one yet, though. Pitch thread: https://forums.swift.org/t/support-custom-executors-in-swift-concurrency/44425

and traffic in Optional<Builtin.Executor> in various places.

Previously, they were storing a low-bit flag that indicated that they were a default actor. Using an extra inhabitant frees up the low bit for future use without being conspicuously more expensive to check.

rjmccall · 2021-04-30T07:12:46Z

@swift-ci Please test

rjmccall · 2021-04-30T07:22:14Z

@swift-ci Please clean test Windows

drodriguez · 2021-05-01T00:22:06Z

I had to kill the Windows+VS2017 CI job that was testing this because it was stuck executing the custom_executors.swift test. It has been 6 hours, when normally the builds are around 1 hour. Just FYI. It might probably get stuck again. I might disable the test in Windows+VS2017 during the weekend.

rjmccall · 2021-05-01T03:56:59Z

I've also merged this into the concurrency-5.5-abi-2 branch.

rjmccall · 2021-05-01T04:00:07Z

I had to kill the Windows+VS2017 CI job that was testing this because it was stuck executing the custom_executors.swift test. It has been 6 hours, when normally the builds are around 1 hour. Just FYI. It might probably get stuck again. I might disable the test in Windows+VS2017 during the weekend.

Damn. The Windows bot was broken earlier, and I just had to hope that that wasn't masking a bug, but I guess it didn't work out. I can't debug the problem, but I'll look the code over to see if I can figure it out by inspection.

rjmccall · 2021-05-01T04:17:52Z

Opened #37200 to disable the test on Windows. IIUC there are several major bugs in the Windows concurrency support that cause this kind of test failure.

drodriguez · 2021-05-01T06:12:07Z

It seems to have been working OK for two builds in a row. It is probably flaky, but not that flaky. I haven't look, but I imagine it is not a logic error, but a small race in the Windows implementation that would be hard to figure out without a lot of trying. It is fine disabling it so it is less noise.

ktoso reviewed Apr 13, 2021

View reviewed changes

lib/SILOptimizer/Mandatory/LowerHopToActor.cpp Show resolved Hide resolved

rjmccall force-pushed the custom-executors branch from c656072 to dca5f22 Compare April 14, 2021 08:03

rjmccall marked this pull request as ready for review April 14, 2021 08:03

rjmccall force-pushed the custom-executors branch from dca5f22 to 6240c73 Compare April 14, 2021 19:38

rjmccall force-pushed the custom-executors branch from 6240c73 to 374b29a Compare April 15, 2021 04:26

jckarter reviewed Apr 15, 2021

View reviewed changes

rjmccall force-pushed the custom-executors branch from ebfcb3a to fcb9901 Compare April 16, 2021 22:09

rjmccall force-pushed the custom-executors branch 2 times, most recently from 5b9257c to 6ac1f15 Compare April 29, 2021 08:01

rjmccall force-pushed the custom-executors branch from 6ac1f15 to 6cea747 Compare April 29, 2021 20:18

rjmccall added 4 commits April 30, 2021 03:11

Add a utility to clone an @available attribute.

769c2e7

Remove the implicit nil inhabitant of Builtin.Executor,

ec5215b

and traffic in Optional<Builtin.Executor> in various places.

Default actors carry a null witness-table pointer in Builtin.Executor.

565198e

Previously, they were storing a low-bit flag that indicated that they were a default actor. Using an extra inhabitant frees up the low bit for future use without being conspicuously more expensive to check.

rjmccall force-pushed the custom-executors branch from 6cea747 to 565198e Compare April 30, 2021 07:12

rjmccall merged commit cc2863c into swiftlang:main Apr 30, 2021

rjmccall deleted the custom-executors branch April 30, 2021 17:54

aschwaighofer mentioned this pull request Jun 4, 2021

[5.5-05142021] Fix ClassDecl::isRootDefaultActor to use the maximal resilience expansion #37786

Merged

Introduce basic support for custom executors #36878

Introduce basic support for custom executors #36878

Uh oh!

Conversation

rjmccall commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmccall commented Apr 13, 2021

Uh oh!

swift-ci commented Apr 13, 2021

Uh oh!

swift-ci commented Apr 13, 2021

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjmccall commented Apr 14, 2021

Uh oh!

swift-ci commented Apr 14, 2021

Uh oh!

swift-ci commented Apr 14, 2021

Uh oh!

rjmccall commented Apr 14, 2021

Uh oh!

swift-ci commented Apr 14, 2021

Uh oh!

swift-ci commented Apr 14, 2021

Uh oh!

rjmccall commented Apr 15, 2021

Uh oh!

swift-ci commented Apr 15, 2021

Uh oh!

jckarter Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmccall Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

jckarter Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

jckarter commented Apr 15, 2021

Uh oh!

rjmccall commented Apr 15, 2021

Uh oh!

rjmccall commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Apr 15, 2021

Uh oh!

jckarter commented Apr 15, 2021

Uh oh!

rjmccall commented Apr 16, 2021

Uh oh!

rjmccall commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Apr 17, 2021

Uh oh!

jckarter commented Apr 19, 2021

Uh oh!

rjmccall commented Apr 29, 2021

Uh oh!

swift-ci commented Apr 29, 2021

Uh oh!

swift-ci commented Apr 29, 2021

Uh oh!

rjmccall commented Apr 29, 2021

Uh oh!

swift-ci commented Apr 29, 2021

Uh oh!

rjmccall commented Apr 30, 2021

rjmccall commented Apr 13, 2021 •

edited

Loading

jckarter Apr 15, 2021 •

edited

Loading

rjmccall commented Apr 15, 2021 •

edited

Loading

rjmccall commented Apr 16, 2021 •

edited

Loading