Skip to content

Conversation

@dfed
Copy link
Owner

@dfed dfed commented Oct 7, 2024

We don't need to bounce through a global queue in order to ensure FIFS (first-enqueued-first-start) behavior in our ActorQueue. Instead, we just can make the async for loop await switching to the ActorType's context.

This PR is unlikely to affect performance. This PR should make this code a bit easier to follow, however.

@dfed dfed self-assigned this Oct 7, 2024
@codecov
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ef3c52d) to head (86a73ae).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #40   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          657       666    +9     
=========================================
+ Hits           657       666    +9     
Files with missing lines Coverage Δ
Sources/AsyncQueue/ActorQueue.swift 100.00% <100.00%> (ø)

@dfed dfed changed the title Remove need for ActorQueueSynchronization Eliminate unnecessary hops when enqueuing an ActorTask Oct 8, 2024
///
/// - Parameter actor: The actor on which the queue's task will execute. This parameter is not retained by the receiver.
/// - Warning: Calling this method more than once will result in an assertion failure.
/// - Precondition: Calling this method more than once will result in a precondition failure.
Copy link
Owner Author

Choose a reason for hiding this comment

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

Now that we're kicking off the stream execution here, precondition feels appropriate.

Comment on lines 79 to 97
actor.execute { [taskStream] _ in
func beginExecuting(
_ operation: sending @escaping (isolated ActorType) async -> Void,
in context: isolated ActorType
) {
// In Swift 6, a `Task` enqueued from an actor begins executing immediately on that actor.
// Since we're running on our actor's context due to the isolated parmater, we can just dispatch a Task to get first-enqueued-first-start task execution.
Task {
await operation(context)
}
}

for await actorTask in taskStream {
await beginExecuting(
actorTask.task,
in: actorTask.executionContext
)
}
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

this entire closure executes within the actor context!

extension Actor {
nonisolated
func execute(
_ task: @escaping @Sendable (isolated Self?) async -> Void
Copy link
Owner Author

@dfed dfed Oct 8, 2024

Choose a reason for hiding this comment

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

a bit nuts, but using (isolated Self?) enables us to isolate the closure to the actor's context without retaining the actor.

}

for await actorTask in taskStream {
await beginExecuting(
Copy link
Owner Author

Choose a reason for hiding this comment

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

we need to await here despite the fact that we're already isolated to the context because the compiler does not have a guarantee that actorTask.executionContext === actor`. In practice, these are always the same and therefore we do not suspend here.

@dfed dfed changed the title Eliminate unnecessary hops when enqueuing an ActorTask Remove need for ActorQueueSynchronization Oct 10, 2024
@dfed dfed merged commit b51448d into main Oct 10, 2024
@dfed dfed deleted the dfed--simplify branch October 10, 2024 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants