Skip to content

Refactoring of the TaskLocal::Item #77241

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

Conversation

nickolas-pohilets
Copy link
Contributor

Replace NextLinkType with Item::Kind with more clear definition. Use llvm::PointerIntPair<>. Use hierarchy of classes instead of optional fields inside Item.

Combined Storage::copyToOnlyOnlyFromCurrentGroup() into Storage::initializeLinkParent(). Also create parent task marker when copying items created inside withTaskGroup(). Removed Storage::peekHeadLinkType().

Replace NextLinkType with Item::Kind with more clear definition.
Use llvm::PointerIntPair<>.
Use hierarchy of classes instead of optional fields inside Item.
Combined Storage::copyToOnlyOnlyFromCurrentGroup() into Storage::initializeLinkParent().
Also create parent task marker when copying items created inside withTaskGroup().
Removed Storage::peekHeadLinkType().
@ktoso
Copy link
Contributor

ktoso commented Oct 27, 2024

@swift-ci please smoke test

///
/// Items of this kind must be copied by a group child task for access
/// safety reasons, as otherwise the pop would happen before the child
/// task
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: may wanna join this and the next line

/// does not "contribute" any task local values. This is to speed up
/// lookups by skipping empty parent tasks during get(), and explained
/// in depth in `initializeLinkParent()`.
ParentTaskMarker = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I like that way of expressing it

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, that's a nice refactor, thanks!

@ktoso ktoso merged commit 7ea2b3d into swiftlang:main Oct 28, 2024
3 checks passed
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