-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Concurrency] Set thread base priority when running escalated Tasks #84895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e2344fb to
a4bdd3d
Compare
|
@swift-ci please smoke test |
ktoso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small requests, but looks good logic wise, thanks @Bryce-MW !
|
@swift-ci please smoke test |
a4bdd3d to
0d8812b
Compare
|
@swift-ci please smoke test |
|
Sadly a real error: /home/build-user/swift/stdlib/public/Concurrency/Task.cpp:157:7: error: use of undeclared identifier 'contextInitialized' |
0d8812b to
90456a2
Compare
|
Should build and the test succeeds in my testing. I will test the runtime further but at least this fixes the issue that I was trying to fix. |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Windows |
|
docc seems to have broken windows CI, attempting to test the fix with this build ^ |
|
My local testing hasn't found any issues so far |
|
@swift-ci please smoke test Windows |
| qosClass: DispatchQoS.QoSClass(rawValue: qos_class_self())!, | ||
| relativePriority: 0) | ||
| expectEqual(initialQos, DispatchQoS.utility) | ||
| let childTask = Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming nitpick, that's not a "child" task, we can fix in followup tho
ktoso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, thank you!
When using Dispatch as the default global executor, the threads that Tasks are run on have base priorities corresponding to the current priority of the Task at the time of enqueue. Some APIs outside of Swift look at the base priority of the thread (e.g qos_class_self). The thread's base priority may be higher than the base priority of the Task. Swift Concurrency APIs that use the Task's base priority do not have this issue.
This change informs Dispatch of the Task's base priority before running the Task giving it a chance to update the base priority of the thread. Dispatch returns a value that it requires us to give back after execution. I assert in cases where we do not expect to need to change the priority that a zero value is returned (since this indicates no override was done).
In addition, it seems that SWIFT_CONCURRENCY_ENABLE_PRIORITY_ESCALATION wasn't being enabled because it checks SWIFT_CONCURRENCY_ENABLE_DISPATCH which isn't set until later (in Concurrency.h which includes Task.h before the definition).
rdar://88155873