SE-0392: Custom Actor Executors

Hi everyone. The review of SE-0392: Custom Actor Executors begins now and runs through March 21, 2023.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager. When emailing the review manager directly, please keep the proposal link at the top of the message.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/main/process.md

Thank you for helping advance Swift!

Joe Groff
Review Manager

34 Likes

The section on the delegateActor property in Future Directions doesn't have anything on why it was removed for this version of the proposal. Are there drawbacks, or was it just to contain the scope?

2 Likes

I’ve reread this since it was pitched and I have a couple of questions:

  1. The examples of assert, precondition, assume use functions suffixed with “executor” but then they pass MainActor.shared, which reads weird. To me MainActor shared would indicate the type of shared is MainActor, but then the suffix the function makes me believe it should be an executor.
assertOnActorExecutor(MainActor.shared)

Shouldn’t they use sharedUnownedExecutor instead?

  1. The assume APIs are not prefixed with unsafe? I probably missed the reasoning but seeing how they basically are “compiler trust me, i know what i’m doing” I would expect them to be “unsafe”

But besides that big +1!!

3 Likes

Should this:

  nonisolated var unownedExecutor: UnownedSerialExecutor { 
    // use the shared specific thread executor mentioned above.
    // alternatively, we can pass specific executors to this actors init() and store and use them this way.
    SpecificThreadExecutor.shared
  }

be:

  nonisolated var unownedExecutor: UnownedSerialExecutor { 
    // use the shared specific thread executor mentioned above.
    // alternatively, we can pass specific executors to this actors init() and store and use them this way.
    SpecificThreadExecutor. sharedUnownedExecutor
  }

?

1 Like

It was, and still is, just a potential idea that we might want to get to. It's not fully designed or implemented. We're interested in it, but not as part of this proposal - it's large enough already :slight_smile:

This is on purpose in order to provide better error messages. For example, since you may have many actors on the same executor, without passing the actor we would not be able to provide as good messages as those:

Expected same executor as actor 'a.DefaultExecutorSomeone(...description...)' ('DefaultActorExecutor'), but was executing on 'GenericExecutor'.")

We'd only be able to say "got default executor" which is very uninformative. There is some follow-up implementation work here to potentially be able to print the actor information from an unowned default actor executor but that's actually very very hard due to how racy unowned default executor references are with regards to their actors... the actor could have been destroyed already even though there is code executing on its default executor still and other fun situations like that.

And in general I believe focusing on "the same executor as [some other actor]" mindset is more useful than just passing unowned things around. It gives more structure to things we're expressing and designing.

Unlike the initial pitch that explored the assume API, the proposed API is safe, as the docs comment explains:

/// - Attention: If this is called from a different execution context,
///   the method will crash, in order to prevent creating race conditions
///   with the MainActor.

(and same for the some Actor version of the API).

In other words, this is not a "trust me™" API, it is a checked API in which you express an assumption that the runtime verifies for you. We do not continue running if the assumption was incorrect, making this API safe in the Swift meaning of the word, i.e. you will not cause a race, you may cause program termination.

5 Likes

Thanks for the typo spotting, let's fix typos directly on github :slight_smile: I applied the change: Typo in executor reference by ktoso · Pull Request #1974 · apple/swift-evolution · GitHub

Could this be used to create an actor that wraps a DispatchQueue?

1 Like

Yes, that's one of the use cases. Dispatch queues, Swift NIO, or "specific thread" are the examples that come to mind as immediate use-cases.

There is more work to be done that is outside the governance of the Swift evolution to make dispatch queues efficiently implement serial executors, but it definitely is one of the reasons to standardize custom executors to begin.

You can also see the "complex" equality part of the proposal is specifically tailored for dispatch queues in order for queues targeting the same "root queue" to be able to return "yes, we're on the same context".

5 Likes

For some reason, I didn't even consider the option of creating a PR. I will do that next time.

3 Likes

Yes I agree with that and I understand its purpose. My issue was with the naming of the function that ends with "executor" but it accepts an "actor" type. Reading it again assertOnActorExecutor I guess it makes sense if I read it as "on the actor**'S** executor" and treat the parameter as the subject.

Makes sense, I was just confused on first read.

There you go, I missed those docs. Thanks for clarifying <3

3 Likes

+1 From me. I've been using custom executors for a while and they have proved to be useful. Seeing as the executor protocols are already back ported it would be ideal if an official way of running jobs was back ported. Without this it would create an awkward situation where we would have to add availability annotations to types that are "technically" available on older platforms but cannot actually be used by actors.

2 Likes

The “run” function on UnownedJob we can backdeploy easily I think (since it calls into existing runtime function), agreed.

Some of the other things like the new job type are not as simple but will give it a look cause by case.

3 Likes

Something I think the proposal should clarify is how custom executors for a distributed actor will work. In particular, for this new protocol requirement:

public protocol DistributedActor: AnyActor {
  /// ...
  nonisolated var unownedExecutor: UnownedSerialExecutor { get }
}

what happens when the instance is remote? There isn't really an executor for the "proxy" instances. Since it's nonisolated, we can't make a call to the remote instance to ask about the executor. And even if we could, these executors aren't really Codable.

One idea is to make the type of unownedExecutor for a distributed actor return an optional. You'd get back nil only in the remote case. A downside is that I forsee this optionality being abused as a non-async form of whenLocal to query whether the instance is local.

2 Likes

An anonymous contributor has a naming suggestion here: It seems like the actor portion of the function name could be downplayed by relying on the name of the passed-in value, since assertOnExecutor(of: MainActor.shared) reads rather naturally.

15 Likes

That's a great naming idea, thank you anonymous :slight_smile:

I'll follow up on Kavon's excellent question today, sorry for the delay here, was wrangling with this exact implementation bit just now the last days to form a better opinion on it. :pray:

3 Likes

Sorry for the slow reply here @kavon, and thanks for raising a good topic to discuss here!

I was actively looking into related things for the last few days, so wanted to focus there and be able to provide a good discussion here, so let's dive in.

So in normal usage the executor would never be used in practice, because:

  • calls to distributed func on remote actors are effectively calls to nonisolated thunks that notice the reference is remote, and instead of calling the distributed func hello() on the distributed actor, redirect the call to an invocation to its ActorSystem's remoteCall.

So in that sense, an executor never "matters" for remote distributed function calls -- we never use it, because the code-path for remote actors is always nonisolated for all distributed calls. And non-distributed calls are not allowed externally. If we are "inside" a distributed actor (i.e. have an isolated DistributedActor), we are guaranteed that it is local, so all the usual Actor semantics about the executor hold.

Now, my assumption when proposing this API shape was that there's two ways to implement an unownedExecutor for a distributed actor:

  1. as reference to some global shared executor
    • nonisolated var unownedExecutor: UnownedSerialExecutor { MainActor.sharedUnownedExecutor }
  2. the same way we configure other things about distributed actors, by carrying the information in the ID which is the only stored property the user has a lot of control over (though as usual it depends how much control the specific actor system allows you here),
    • we do this a lot actually in practice, so if this seems weird I assure you it's not as weird as it seems :sweat_smile:
    • this boils down to having an ActorID have e.g. id.preferredExecutor: UnownedExecutor? that you'd use this way to implement the ID (The ActorID can have all kinds of "preferences", and I do think this is a good pattern to stick to, like "which region to prefer" for lookups etc. etc.):
    • I (wrongly! so thank you for reminding me to look into this deeper) assumed we'll then implement the unownedExecutor as id.preferredExecutor ?? buildDefaultActorExecutorRef(self)
    • I was wrong here though, because we don't expose the Builtin.buildDefaultActorExecutorRef(self) and probably shouldn't :exclamation:

So yes, I agree with your proposal to change the user-facing part here to ? and at the same time I don't think we should make the exposed interface and how distributed actors interact with the rest of the compiler different... So I think we can achieve both goals bo doing the following: make the user-facing API more explicit that "this only matters for local actor" but also keep the synthesized unownedExecutor the same way as default actors to it today:

public protocol DistributedActor: AnyActor {
  nonisolated var localUnownedExecutor: UnownedSerialExecutor? { get }
}

distributed actor Worker {
  nonisolated var localUnownedExecutor: UnownedSerialExecutor? { ... }
  
  // KEEP the synthesized implementation, but make it do this:
  // nonisolated var localUnownedExecutor: UnownedSerialExecutor { // do not mark as default actor though
  //   localUnownedExecutor ?? Builtin.buildDefaultActorExecutorRef(self)
  // }
}

This way we don't have to special case a lot of places in the compiler but keep the deciding "is it a default actor" as well as accessess by the compiler when it wants to make hop_to_executor still the same as they are today. I think this way we can nicely keep the "devirtualize" logic for existing default distributed actors as well... :thinking:

I'm going through implementing / validating this idea though in order to make sure all the layout questions work out if we did this. Maybe hanging the hop_to_executor logic won't be too bad... I'll give this a shot too, though I would like to not have to change all places that touch the unownedExecutor but just keep them the same code-paths basically. As far as assert/precondition/assume APIs go, this should be working out fine still, but I'll also verify.

Thanks @kavon for mentioning this bit!

Looking deeper into it I think maybe the ?? default is not a great idea; yes, it allows us to always give an answer, but then again, what would that answer be useful for.

So perhaps changing the synthesized impl to do if __isLocalActor(self) { Builtin.default... } else { nil } might be simpler and more consitent to understand.

However, developers may implement this function however they like, even returning an executor for a remote reference if they wanted to. It won't cause "trouble" because we never use it for anything, nor is there ANY API that would allow abusing this - i.e. the assumeOnLocalExecutor(of: someDistributedActor) also already prevents abuse here. So it is a weird hack to check "is remote" on a default actor, but should not be relied on...

We would have to require that returning nil for a local distributed actor as its executor be illegal; which my previous idea did work around because it just meant "yeah, still default" but that just muddied the waters. Perhaps indeed, let's have it clean cut here and just say returning nil when local is illegal.

I'll investigate this angle as well, thanks again @kavon -- in general I agree we need the ? here, but just figuring out what exactly that means for the implementation.

Excited to see what comes of this in the NIO space specifically. I don’t have anything of substance to say about the specific design. I try to avoid commenting on naming usually, especially for APIs I won’t use directly. But….

I feel that “Job” is an unfortunate name for what is expected to be a very niche API in the standard library. My understanding from this proposal is that Job is expected to be used only by implementations of custom executors and that relatively few people/libraries are expected to write their own custom executors.

Two concrete concerns:

  1. The name sounds as high-level as Task, leading to wasted time and confusion when learning about concurrency. This might be avoided by careful documentation.
  2. The name is widely used in programs. It’s true that declarations in the current module will shadow those in the standard library, but I think it’s generally understood to be bad style to make your own thing which shadows the standard library especially when that thing is totally different from the standard library’s version (e.g. Result/Result shadowing is OK-ish, but making your own Task that has nothing to do with concurrency is probably a bad idea).

To expand on that second point: I have several server-side Swift programs which act as “workers” — they pull a job from a queue, do the work, and output the results somewhere. In those worker programs, the very natural declaration is:

struct Job: Codable {
   var id: String
   var customJobParameter: Int
   // etc
}

I think that having a Job in the standard library makes code more confusing to a new programmer in my codebase. This would be fine if Job were widely used like Task, but I think if it’s meant to be niche, the name should be somewhat niche as well.

I’d recommend ExecutorJob, but any old qualifier would do IMO.

6 Likes

You can track our experiments here.

9 Likes

Could we make the assertOnExecutor function a method, e.g. in a protocol extension of Actor? That would have the clear advantage that we could make it a static method on global actors, so that you could just write something like:

MainActor.assertOnExecutor()

or

MainActor.assertIsolated()

instead of

assertOnExecutor(of: MainActor.shared)

Also, I think I really like the assertIsolated name.

6 Likes