Skip to content

updating error type to use computed properties #559

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

heckj
Copy link

@heckj heckj commented May 30, 2025

Small updates to convert this error type to using computed properties instead of static properties, following the pattern at apple/swift-nio#3229

@heckj heckj requested review from fabianfett and gwynne as code owners May 30, 2025 22:53
@gwynne
Copy link
Member

gwynne commented May 31, 2025

Not that I have any objection, but does this pattern confer any particular benefit?

Edit: Nevermind, I should've clicked through to the PR you linked before asking 😅.

@heckj
Copy link
Author

heckj commented May 31, 2025

Heh - the tl;dr for anyone looking: Cory said it much better than I would:

Public static lets can serve a bunch of roles, but one of them is to store simple constants: integers, and other trivial types, for example. This is a nice pattern and for internal and private static lets it works well, but for public ones it produces some inefficient code.

In particular, it has two downsides. First, it allocates storage for that value. We don't actually need to allocate a few hundred extra megabytes for the various integers we want to store.

Secondly, it forces calling code to access the address and call the dispatch_once code in order to get hold of the value. For trivial types we don't need that cost: they can just know what the value is directly.

Inlinable computed vars avoid all of these costs: they have no size overhead for storage, and they are visible to all clients so their values can be directly assembled.

Result:

Better codegen, smaller memory footprints, more attributes.

@heckj
Copy link
Author

heckj commented May 31, 2025

As for the "why did I pick this particular thing to poke at?" - I'm referencing this type in some internal documentation as an example of a good way to wrangle Error types. Other examples got the above treatment, and I wanted to both have the code I'm referencing actually match what's in this project while also reflecting consistency among the examples I choose.

Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

@heckj sorry for taking so long to come back to you!

Comment on lines +9 to +12
@usableFromInline
let base: Base

@inlinable
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this one now marked as @inlinable?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't need to be, I was cargo-culting the pattern I saw in apple/swift-nio#3229, which added @inlinable to the initializers as well. I presumed that was helping with performance effects from inlining in that code, but I'm replicating that somewhat blindly.

Happy to pull it back out.

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