Skip to content

[Concurrency] Task names #77609

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
merged 8 commits into from
Feb 21, 2025
Merged

[Concurrency] Task names #77609

merged 8 commits into from
Feb 21, 2025

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Nov 14, 2024

Work in progress on adding names to tasks.

Pending SE proposal.

@ktoso ktoso force-pushed the wip-task-names branch 2 times, most recently from 8538eef to 39038d8 Compare December 12, 2024 05:42
@ktoso ktoso force-pushed the wip-task-names branch 2 times, most recently from b74c08b to b5be7ee Compare January 18, 2025 04:04
@ktoso ktoso force-pushed the wip-task-names branch 2 times, most recently from ec11380 to 55c5ae6 Compare February 17, 2025 13:10
void *allocation = _swift_task_alloc_specific(
this, sizeof(class TaskNameStatusRecord));

// TODO: Copy the string maybe into the same allocation at an offset or retain the swift string?
Copy link
Contributor

Choose a reason for hiding this comment

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

Retaining the Swift string makes sense to me. It avoids an extra copy when the string is dynamic, and avoids any copies when it's constant. I'm not sure how inconvenient it would be to manipulate a String from the C++ side of things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so sadly it's quite painful to make this work... we have to manually inspect flags in the String's flags and then copy or not copy accordingly... I'll try to prepare our ABI so we COULD do this but I will probably not do so in the first impl.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is our LTO good enough to have the stdlib expose some minimal calls for us to use here, and have it optimize to something reasonable? (Or maybe this isn't performance sensitive enough for that to matter anyway?) We'd really want to reuse the existing String code and treat it as an opaque value here.

@ktoso ktoso marked this pull request as draft February 18, 2025 02:30
@ktoso ktoso changed the title [WIP] Task names work in progress [Concurrency] Task names Feb 20, 2025
@ktoso ktoso marked this pull request as ready for review February 20, 2025 10:11
#endif
// NOTE: We had to move sources touching TaskGroup addTask APIs into TaskGroup+addTask.swift.gyb
// due to the build having trouble with using the generated source in the same module
// rdar://145171772
Copy link
Contributor Author

@ktoso ktoso Feb 20, 2025

Choose a reason for hiding this comment

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

Build problem nr 1, I can't figure out how to get the gyb generated file included in the module 🤔


import Swift

// ==== addTask / addTaskUnlessCancelled ---------------------------------------
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gyb replaces a few thousand lines (and like 20 definitions) of addTask implementations. We still have ad hoc ones though, and I can't get the gyb result to be included in the build so there's workarounds here still

@ktoso ktoso removed request for xedin and DougGregor February 20, 2025 13:39
@ktoso
Copy link
Contributor Author

ktoso commented Feb 20, 2025

@swift-ci please smoke test

@kubamracek kubamracek self-requested a review February 20, 2025 22:21
@ktoso
Copy link
Contributor Author

ktoso commented Feb 21, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Feb 21, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Feb 21, 2025

The build isn't still right, we still need the stdlib/public/Concurrency/TaskGroup+Embedded.swift workaround file... @etcwilde maybe we can investigate some more -- basically when we checked I had that file still, even while we removed other workarounds so it seemed to work (i.e. see the gyb generated functions), but seems it doesn't still

@ktoso
Copy link
Contributor Author

ktoso commented Feb 21, 2025

@swift-ci please smoke test

@ktoso ktoso merged commit 4ab5d26 into swiftlang:main Feb 21, 2025
3 checks passed
@ktoso ktoso deleted the wip-task-names branch February 21, 2025 13:28
@ktoso
Copy link
Contributor Author

ktoso commented Feb 22, 2025

resolves rdar://88918762

ktoso added a commit to ktoso/swift that referenced this pull request Feb 23, 2025
ktoso added a commit to ktoso/swift that referenced this pull request Feb 23, 2025
ktoso added a commit that referenced this pull request Feb 24, 2025
ktoso added a commit to ktoso/swift that referenced this pull request Feb 25, 2025
ktoso added a commit to ktoso/swift that referenced this pull request Mar 4, 2025
@ktoso ktoso restored the wip-task-names branch March 4, 2025 08:50
ktoso added a commit to ktoso/swift that referenced this pull request Mar 7, 2025
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