Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid converting array holding existentials #3006

Merged
merged 4 commits into from
Nov 28, 2024

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Nov 27, 2024

Motivation:

When converting an Array that holds existentials, it is necessary for Swift to allocate a new Array and copy the elements into it, so that it can fix up their existential boxes. We forced this to happen with taking a [ChannelHandler] to [ChannelHandler & Sendable], without rewriting the other functions.

The result of that was that the other functions needed to convert the channel handler types, causing extra allocations in this path.

Modifications:

Propagate the constraint down to the point where we iterate the Array.

Result:

Allocations reduced.

Motivation:

When converting an Array that holds existentials, it is necessary for
Swift to allocate a new Array and copy the elements into it, so that
it can fix up their existential boxes. We forced this to happen with
taking a `[ChannelHandler]` to `[ChannelHandler & Sendable]`, without
rewriting the other functions.

The result of that was that the other functions needed to convert the
channel handler types, causing extra allocations in this path.

Modifications:

Propagate the constraint down to the point where we iterate the
Array.

Result:

Allocations reduced.
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Nov 27, 2024
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Nice one!

@Lukasa
Copy link
Contributor Author

Lukasa commented Nov 28, 2024

Of course, this change just moved the cost into the SyncOperations instead. So they now need to have totally separate codepaths to avoid reprojecting the array.

@Lukasa Lukasa merged commit 8126cba into apple:main Nov 28, 2024
42 of 43 checks passed
@Lukasa Lukasa deleted the cb-allocation-regression-on-add-handler branch November 28, 2024 11:30
@dnadoba
Copy link
Member

dnadoba commented Jan 9, 2025

it is necessary for Swift to allocate a new Array [...] We forced this to happen with taking a [ChannelHandler] to [ChannelHandler & Sendable]

That should be same type at runtime as Sendable is just a marker protocol. @Lukasa did you opened a swiftlang issue?

@Lukasa
Copy link
Contributor Author

Lukasa commented Jan 10, 2025

I didn't, but I don't think the fact that it's the same type at runtime is quite true.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants