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

Use TaskGroups instead of array of tasks #7

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Frizlab
Copy link

@Frizlab Frizlab commented Dec 13, 2021

Rationale for this comes from this thread https://forums.swift.org/t/taskgroup-vs-an-array-of-tasks/53931
Apparently launching a lot of Tasks just like that is not very good, and we should use task groups instead.

I did not test the performance implication of this change, though. Maybe it’s actually worse because of the additional sort needed. In theory it should be better though.

@AndrewBarba
Copy link

AndrewBarba commented Jan 17, 2022

Instead of sorting does it make sense to do something like:

var results = Array<T?>(repeating: nil, count: self.count)
...
for try await result in group {
    results[result.offset] = result.value
}
return results as! [T]

@Frizlab
Copy link
Author

Frizlab commented Jan 23, 2022

@AndrewBarba I’d say yes. And it’s probably faster too. I’ll push your alternative.

I’m not sure we gain a lot. We’d have to actually measure this solution and the one in the previous commit.
@Frizlab
Copy link
Author

Frizlab commented Jan 23, 2022

Done. However, count is not available in a generic Sequence, so I have to grow the result array as I retrieve the results. I’m not sure we actually gain anything compared to the sort solution. We should measure both solution to find the fastest.
Previous implementation is still available here 1105df4.

@tachyonics
Copy link

I don't think it is necessary to have count available on the input Sequence. We are iterating through the sequence to add tasks to the group so we know how many tasks we have added to the group and therefore how many we are expecting back-

https://github.com/tachyonics/CollectionConcurrencyKit/blob/main/Sources/CollectionConcurrencyKit.swift#L123

@Frizlab
Copy link
Author

Frizlab commented Oct 11, 2022

@tachyonics You’re not wrong. I have pushed your method.

@johnrogers
Copy link

Fully support these changes. Not only will it make the functions more performant, but it will ensure each function participates in Task cancellation inheritance correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants