-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[5.5][concurrency] Implement support for Task local exclusivity access sets. #38598
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
Merged
gottesmm
merged 8 commits into
swiftlang:release/5.5
from
gottesmm:release/5.5/rdar80492364
Jul 23, 2021
Merged
[5.5][concurrency] Implement support for Task local exclusivity access sets. #38598
gottesmm
merged 8 commits into
swiftlang:release/5.5
from
gottesmm:release/5.5/rdar80492364
Jul 23, 2021
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…PRINTF_DEBUG allow it to be used to log Task completion. This should let people debug Task lifetime easier. (cherry picked from commit ff6b67c)
…g a job synchronously on an executor. I am going to use this to test that we propagate synchronous accesses into asynchronous tasks access sets. To ensure this is not ABI, I underscored/marked this as alwaysEmitIntoClient. (cherry picked from commit fc4d6c8)
@swift-ci test |
mikeash
approved these changes
Jul 23, 2021
Build failed |
0d4bd2f
to
e7bed8d
Compare
@swift-ci test |
The implemented semantics are that: 1. Tasks have separate exclusivity access sets. 2. Any synchronous context that creates tasks will have its exclusive access set merged into the Tasks while the Task is running. rdar://80492364 (cherry picked from commit 0aec816) Conflicts: stdlib/public/Concurrency/TaskPrivate.h
…e exclusivity part of the runtime to emit logging information. The specific environment variable is SWIFT_DEBUG_RUNTIME_EXCLUSIVITY_LOGGING. We only check the environment the first time we try to lookup the TLSContext, so it shouldn't impact exclusivity performance. My intention is to use this to write some FileCheck tests so that we can really be sure that the runtime is doing what we think it is. As such I also changed the small amount of logging routines meant for use in the debugger to use stdout. (cherry picked from commit f79108c)
(cherry picked from commit e662a14)
…builds to more rigorously test the Task based swizzling AccessSet functionality. I was writing a bunch of functional tests and realized I needed to be more rigorous and get actual runtime logging to be able to be sure that the runtime feature was working correctly. (cherry picked from commit 7a41f4c)
(cherry picked from commit f390450)
…m to hang. Custom executor isn't a true feature yet, so I am disabling on Windows until we can look at why the hangs are occuring on Windows. (cherry picked from commit dae605a)
e7bed8d
to
cdac1ea
Compare
@swift-ci test |
@swift-ci nominate |
DougGregor
approved these changes
Jul 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implemented semantics are that:
rdar://80492364
CCC
Explanation: Exclusivity and concurrency are incompatible due to exclusivity relying on thread local storage to store exclusive access sets. This results in different Tasks potentially having the same access set (breaking the program model where we have not defined any such relationship) and also the possibility of an access associated with a task beginning on one thread and then ending on a different thread if the task runs on a different thread (causing the exclusivity runtime to crash since it can't find the access).
In this change, I fix these issues by:
Scope: Without this change, Tasks will (depending if they are on the same thread) hit exclusivity violations and the runtime may hit nullptrs at runtime if an access for a Task begins on one thread and is ended on a different Thread that the task runs upon.
SR Issue: rdar://80492364
Risk: Medium. Since this is a semantic change, I think it is a medium risk change. That being said, I think the exhaustive testing significantly reduces the risk.
Testing: I wrote exhaustive tests of all of the cases that the runtime can hit. I provided both runtime tests and specific logging based FileCheck tests.
Reviewer: @mikeash
Example: