Add a task scheduler for background indexing and preparation #1208
Conversation
|
@swift-ci Please test |
| case suspendDependency(TaskDescription) | ||
| } | ||
|
|
||
| public protocol TaskDescriptionProtocol: Identifiable, Sendable, CustomLogStringConvertible { |
There was a problem hiding this comment.
I'd love for you to file an issue on https://github.com/apple/swift so we can enable making such a thing as TaskExecutor, rather than having to resort to trickery and unstructured tasks like this. I believe we could allow such things but today are missing pieces in the custom executors infra I think -- like querying a job for its details etc, like priority, maybe some hooks to know a job "finished" etc.
There was a problem hiding this comment.
Filed apple/swift#73372. It’s a little vague, feel free to refine it with the things that you actually think are relevant and doable.
There was a problem hiding this comment.
That's good, thank you -- I'll fill in the bits, and we'll figure out what is actionable :)
Sources/SKCore/TaskScheduler.swift
Outdated
| /// | ||
| /// Suspension might not do anything if the task has already finished, ie. even though `suspend` was called, the | ||
| /// task might still | ||
| func suspend() { |
There was a problem hiding this comment.
consider renaming the entire "suspend" wording in here; suspending is "step 1 ... ... step 2" and implies that we'll come back to a task, but this isn't that; it is "cancel and run again", like a re-schedule maybe
There was a problem hiding this comment.
Good call. Changed to cancelToBeRescheduled.
Sources/SKCore/TaskScheduler.swift
Outdated
| logger.debug( | ||
| "Elevating priority of \(self.description.forLogging) from \(self.priority.rawValue) to \(targetPriority.rawValue)" | ||
| ) | ||
| Task.detached(priority: targetPriority) { |
There was a problem hiding this comment.
probably doesn't need to detach -- plain task with priority should be fine -- and get task locals copied then
There was a problem hiding this comment.
If I remember dispatch behavior this will not necessarily be 100% reliable... even if you passed highest priority it may decide to use lower I think... but the general idea is okey I suppose.
Please file an issue about an API for __priorityBoost(task, priority) even if underscored I think we should offer this API.
There was a problem hiding this comment.
Filed apple/swift#73369
| self.suspendableTaskCreatedContinuation = suspendableTaskCreatedContinuation | ||
|
|
||
| self.resultTask = Task.detached(priority: priority) { | ||
| await withTaskGroup(of: Void.self) { taskGroup in |
There was a problem hiding this comment.
consider using a discarding task group? in that the group will automatically await and since you're ignoring all failures (there's no failures) and results, the discarding group will be more efficient -- it discards tasks as soon as they complete
There was a problem hiding this comment.
I don’t think that works because I need to cancel the first task in the task group (the one that is updating self.priority) when the second task in the task group (the one that runs tasks from executionTaskCreatedStream) finishes.
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
The scheduler isn’t actually being used yet but it’s complex and generic enough that it’s possible to review it by itself.
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
| } | ||
| }, | ||
| validate: { (recordings: [Set<TaskID>]) in | ||
| // Check that all high-priority tasks get executed before the low-priority tasks |
There was a problem hiding this comment.
We could be executing some high priority and low priority at the same time in the actual check (eg. 2 high -> high + low -> low + low would/should pass).
The scheduler isn’t actually being used yet but it’s complex and generic enough that it’s possible to review it by itself.