Skip to content
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

[Concurrency] Add a unique Task ID to AsyncTask #37325

Merged
merged 1 commit into from May 12, 2021

Conversation

fredriss
Copy link
Contributor

@fredriss fredriss commented May 7, 2021

The offset of this Id in the AsyncTask is an ABI constant. This way
introspection tools can extract the currently running task identifier
without any need for special APIs.

This PR first changes JobFlags to a 32bits entity in order to be able
to pack the ID in the freed-up space.

@fredriss fredriss requested a review from rjmccall May 7, 2021 21:11
@fredriss
Copy link
Contributor Author

fredriss commented May 7, 2021

@swift-ci test

@swift-ci
Copy link
Collaborator

swift-ci commented May 7, 2021

Build failed
Swift Test OS X Platform
Git Sha - 55b816fb0d3ebe1b0591b69f465c9283b63b4595

@fredriss
Copy link
Contributor Author

fredriss commented May 7, 2021

Obviously this changes the size of Job on 32 bits platforms:

/Users/buildnode/jenkins/workspace/swift-PR-macos/branch-main/swift/include/swift/ABI/Task.h:127:1: error: static_assert failed due to requirement 'sizeof(swift::Job) == 6 * sizeof(void *)' "Job size is wrong"
14:32:20 static_assert(sizeof(Job) == 6 * sizeof(void*),
14:32:20 ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@rjmccall That's expected, any preference how to relax the static assert?

@swift-ci
Copy link
Collaborator

swift-ci commented May 7, 2021

Build failed
Swift Test Linux Platform
Git Sha - 55b816fb0d3ebe1b0591b69f465c9283b63b4595

@fredriss
Copy link
Contributor Author

fredriss commented May 7, 2021

Linux doesn't support std::atomic_uint32_t ? I suppose std::atomic<uint32_t> will do

Copy link
Member

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

Generally LGTM, just some slight nit-picking.

@@ -485,13 +490,24 @@ class AsyncTask : public Job {
return reinterpret_cast<AsyncTask *&>(
SchedulerPrivate[NextWaitingTaskIndex]);
}

/// Get the next non-zero Task ID.
uint32_t GetNextTaskId() {
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: we're consistent about using camelCase for method names in Swift.

uint32_t GetNextTaskId() {
static std::atomic_uint32_t Id;
uint32_t Next = Id.fetch_add(1, std::memory_order_relaxed);
if (Next == 0) Next = Id.fetch_add(1, std::memory_order_relaxed);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe initialize it to 1 just so we don't trip this special case the first time? Minor, but still.

@fredriss
Copy link
Contributor Author

fredriss commented May 9, 2021

@swift-ci test

@swift-ci
Copy link
Collaborator

swift-ci commented May 9, 2021

Build failed
Swift Test OS X Platform
Git Sha - b70ebea48bc12f21d323c99820124883b0fb4254

@fredriss
Copy link
Contributor Author

fredriss commented May 9, 2021

@swift-ci test

@fredriss
Copy link
Contributor Author

fredriss commented May 9, 2021

@swift-ci Test Windows Platform

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - 13f041b759c93cc66d46386ba0224f151dc8e763

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 13f041b759c93cc66d46386ba0224f151dc8e763

@fredriss
Copy link
Contributor Author

This breaks 3 watch simulator tests:

18:36:44 Failed Tests (3):
18:36:44   Swift(watchsimulator-i386) :: Concurrency/Runtime/checked_continuation.swift
18:36:44   Swift(watchsimulator-i386) :: Concurrency/Runtime/objc_async.swift
18:36:44   Swift(watchsimulator-i386) :: Concurrency/Runtime/yielding_continuation.swift

The tests seem to crash. Any idea what could be wrong @rjmccall ?

@fredriss
Copy link
Contributor Author

@swift-ci Test Linux Platform

@fredriss
Copy link
Contributor Author

@swift-ci Test Windows Platform

@fredriss
Copy link
Contributor Author

@swift-ci please test

@fredriss
Copy link
Contributor Author

@swift-ci please test

This commit changes JobFlags storage to be 32bits, but leaves the runtime
API expressed in terms of size_t. This allows us to pack an Id in the
32bits we freed up.

The offset of this Id in the AsyncTask is an ABI constant. This way
introspection tools can extract the currently running task identifier
without any need for special APIs.
@fredriss
Copy link
Contributor Author

@swift-ci test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - bbda706

Copy link
Member

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

LGTM

@fredriss
Copy link
Contributor Author

@swift-ci Test Linux Platform

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - bbda706

@fredriss
Copy link
Contributor Author

@swift-ci Test macOS Platform

@fredriss fredriss merged commit 3ed1112 into apple:main May 12, 2021
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.

None yet

3 participants