[Second review] SE-0392: Custom Actor Executors

Evolution Proposal Reviews
11

Apologies for the very late review.

I think the overall direction is uncontroversial, but I have some questions/concerns about the proposed design.

Executor:

  • The call to ExecutorJob.runSynchronously(on:) must happen-after the call to enqueue(_:).

    I don't know what this means - I thought it meant the enqueued job must happen after the call to enqueue returns. But later in the proposal it says that you can execute jobs synchronously inside of enqueue (it's not recommended, but you can).

  • (Editing) The section about the Executor protocol (link) starts with:

    First, we introduce an Executor protocol, that serves as the parent protocol of all the specific kinds of executors we'll discuss next. It is the simplest kind of executor that does not provide any ordering guarantees about the submitted work.

    And ends with:

    Executors are required to follow certain ordering rules when executing their jobs

    It's difficult to follow and I would recommend that this gets tidied up for the benefit of future readers. The ordering rules for serial executors should be part of the serial executor section as it refines the looser Executor behaviour.

SerialExecutor:

  • I'm not entirely happy with the design of the asUnownedSerialExecutor() requirement, and I wonder if it could be modelled better.

    It took a little while for me to understand what this is supposed to do; initially I thought this would basically exclude executors which needed to own (and clean up) their execution resources, but the critical piece of information is over in the UnownedSerialExecutor type:

    /// Generally there are extra constraints imposed on core operations
/// in order to allow this. For example, keeping an actor alive must
/// also keep the actor's associated executor alive; if they are
/// different objects, the executor must be referenced strongly by the
/// actor.

    IIUC this is essentially just Unmanaged.passUnretained on the executor (which any caller could easily do themselves), except that it stores an extra bit to tell the runtime how to perform same-context checks. That extra bit is all that this requirement really provides, isn't it?

    I don't think this is the most obvious design to express that. The word "as" makes it feel like I'm performing some kind of conversion, when I'm not, or that there's some kind of special process the executor needs to perform in order to create that unowned reference (like performing an unbalanced retain on itself), when there isn't - we just need a particular piece of data from it which isn't really related to the idea of an unowned reference.

    I think this could be improved by replacing it with a requirement which specifically returned the same-context checking bit, either as a Bool or enum. I think it would be more logical, and make it easier to ensure that it was correctly implemented. Consider that if I implement isSameExclusiveExecutionContext, I will also need to update my implementation of asUnownedSerialExecutor to use the complexEquality initialiser - that's a non-obvious change, but if that requirement was more clearly related to the context-checking function, it would be more obvious.

    Taking the initialiser call out of the SerialExecutor conformance also means we'd only need a single initialiser for UnownedSerialExecutor, which would read this bit (and any bits we introduce in the future), and set the appropriate internal flags:

    protocol SerialExecutor: Executor {
  var hasCustomSameExecutionContextCheck: Bool { get }
  func isSameExclusiveExecutionContext(as other: Self) -> Bool
}

// or:

enum ExecutionContextEqualityKind {
  case trivial
  case custom
}

protocol SerialExecutor: Executor {
  /* static? */ var executionContextEquality: ExecutionContextEqualityKind { get }
  func isSameExclusiveExecutionContext(as other: Self) -> Bool
}

-------

struct UnownedSerialExecutor {
  init(_ executor: some SerialExecutor) {
    switch executor.executionContextEquality {
    case .trivial: // ... implementation detail of UnownedSerialExecutor ...
    case .custom:  // ... implementation detail of UnownedSerialExecutor ...
    }
  }
  // Only one initialiser needed
}

    (It's a bit of a shame that we need people to manually say whether they have a custom implementation of the same-context check. The language should provide a better way to model these kinds of requirements. In Obj-C you could make it optional and check respondsToSelector...)

  • isSameExclusiveExecutionContext(other executor: Self)

    I think the argument label (other) doesn't read very well; it should be isSameExclusiveExecutionContext(as other: Self).

    Also, I think the "exclusive" part could be clearer. Given this text in the proposal:

    When performing the "is this the same (or a compatible) serial execution context" checks, the Swift runtime first compares the raw pointers to the executor objects. If those are not equal and the executors in question have complexEquality , following some additional type-checks, the following isSameExclusiveExecutionContext(other:) method will be invoked:

    I think the term used here ("serial execution context") would more clearly express that it is known the underlying execution resource will serialise jobs from each given SerialExecutor and prevent their jobs from executing in parallel.

    I also wonder if this is a function where there is no natural self, and whether it should instead be a static function taking 2 parameters (as the == operator does). But that's a minor thing.

JobPriority:

  • Does this type have any fixed values? Or do we just get the rawValue (a UInt8) and assume bigger values have a higher priority than smaller values?

ExecutorJob:

  • Is ExecutorJob also CustomStringConvertible? Or only UnownedExecutorJob?

  • The proposal says:

    A job's description includes its job or task ID, that can be used to correlate it with task dumps as well as task lists in Instruments and other debugging tools

    Will we have to parse that out of the string (like <Job ID=...>? Can it be provided directly, maybe with a property named debugIdentifier (to mirror debugDescription from CustomDebugStringConvertible)?

  • (Future) Is it possible to inspect the task-locals associated with a job? I understand that it would have to account for non-task jobs, but I'd like to know if it's a possible future direction. I think it could be interesting to pass other metadata along to the executor besides the built-in priority levels.

UnownedExecutorJob:

  • Since the unowned version of runSynchronously does not validate its precondition (that the job has not already been executed), and violating that precondition can lead to undefined behaviour, I very strongly suggest that it include the word "unsafe" in its name.

    It is not enough to document that it can cause UB. The documentation should tell you how to use it correctly, but it must be abundantly clear at the call-site that extra consideration is required to audit the safety of code which uses it.

Actors:

  • I'm really uncomfortable with the unownedExecutor requirement for actors. I know that it already exists, but I think it goes against everything we've tried to achieve in Swift with regards to safety. In the best case, it's a pointer and we just hope that keeping the actor alive is enough to keep the executor alive as well; and in the worst case, you can return different executors every time and defeat all of the data race safety that we hope to achieve with language-integrated concurrency.

    I think it it worth exploring safer designs here. For instance, distributed actors have a synthesised actorSystem stored property which must be set. I think a similar design could work for the executor of a regular actor:

    distributed actor Greeter {
  init(name: String, actorSystem: ActorSystem) {
    self.name = name
    self.actorSystem = actorSystem // required (!) initialization of implicit actorSystem property
  }
}

    That way, we could guarantee the invariant that executors will live for as long as there are any actors which use them, that the executor cannot be changed after initialisation, and that all accesses to the synthesised property return the same executor instance. That's at least something.

    If an actor does not explicitly initialise its executor, it would be initialised to a default executor.

Actor assertions:

  • All fine, except that I think preconditionIsolated reads weirdly. assert and assume are verbs, so they read quite well, but precondition is not. Perhaps requireIsolated?
2 Likes