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

Add back CustomDebugStringConvertible conformance #1625

Merged

Conversation

gjcairo
Copy link
Contributor

@gjcairo gjcairo commented Apr 12, 2024

This is part of a wider effort to make breaking changes introduced in main non-breaking.

#1246 removed the inheritance of CustomDebugStringConvertible from AnyExtensionField, and instead moved conformance on to the concrete types (OptionalExtensionField, RepeatedExtensionField, etc), but wrapping these extensions in #if DEBUG statements to avoid unnecessary large binary sizes in release mode.

This was a breaking change, so we need to find a compromise to make this non-breaking but get some of the release mode binary size improvements.

This PR moves the implementation of the conformance of several types to the CustomDebugStringConvertible protocol inside #if DEBUG clauses, instead of the whole conformance declaration. We won't get the same size savings since the conformances will still be captured by the runtime, but we won't get any breaking changes.

@thomasvl thomasvl requested a review from tbkka April 15, 2024 16:57
@thomasvl
Copy link
Collaborator

Any chance you can get numbers for a release build before/after this PR on something with some protos so we can gauge the change in size?

@gjcairo gjcairo force-pushed the non-breaking-customdebugstringconvertible branch from f982f33 to 161f1ff Compare April 16, 2024 12:59
@gjcairo gjcairo force-pushed the non-breaking-customdebugstringconvertible branch from 161f1ff to 612667e Compare April 17, 2024 15:35
@gjcairo
Copy link
Contributor Author

gjcairo commented Apr 17, 2024

I created an otherwise empty executable that contains kubernetes' API protobufs (https://github.com/kubernetes/api/blob/master/core/v1/generated.proto) which is quite sizeable. I built in release mode on Linux, using Swift 6.

  • If I revert the change introduced in Conform to CustomDebugStringConvertible for Debug only #1246 altogether, the binary size is 20.7MB.
  • With the changes on main, the binary size is 19.3MB (that's ~8.75% decrease).
  • With the changes on this PR, the binary size is 20MB (that's a ~3.4% decrease).
  • I also tried annotating the debugDescription implementations with @inlinable and @_alwaysEmitIntoClient, but that yielded binaries of around 20.1MB (~2.9% decrease).

Most of this comes from the fact that the Message protocol inherits from CustomDebugStringConvertible, and generated code conforms to Message.
This means that if we merge this to avoid the API breakage, we'd be losing just over half of the size decrease benefit of the original change (from 8.75% to 3.4%).

I know this isn't ideal, but we are still reducing the binary size compared to the latest release. We can revisit this if/when we release a v2 eventually.

Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

LGTM! This is a fair compromise for now to avoid the API breakage. If we tackle a v2 in the future we should check if we can remove all these public protocols like Enum and/or remove all the inherited protocols from it and just do this on the code generation side. This will allow us to give the generator options to include various conformances and end-users can fine tune these options to their needs.

@thomasvl thomasvl merged commit 86380e1 into apple:main Apr 26, 2024
9 of 11 checks passed
@gjcairo gjcairo deleted the non-breaking-customdebugstringconvertible branch April 29, 2024 10:00
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.

3 participants