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

Make everything inlinable. #89

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

Conversation

Lukasa
Copy link

@Lukasa Lukasa commented Feb 21, 2025

There are a number of performance constraints that can be lifted in the source-available build mode. Many of these are ultimately about specialization, but we should resolve all of these with a judicious sprinkling of @inlinables.

There are a number of performance constraints that can be lifted in
the source-available build mode. Many of these are ultimately about
specialization, but we should resolve all of these with a judicious
sprinkling of @inlinables.
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Feb 21, 2025
Comment on lines +39 to +48
/* fileprivate but */ @usableFromInline let rawValue: UInt8

private static let maxRawValue: UInt8 = 3
/* private but */ @usableFromInline static let maxRawValue: UInt8 = 3

private init(uncheckedValue: UInt8) {
/* private but */ @inlinable init(uncheckedValue: UInt8) {
assert(uncheckedValue <= Self.maxRawValue)
self.rawValue = uncheckedValue
}

fileprivate init?(rawValue: UInt8) {
/* fileprivate but */ @inlinable init?(rawValue: UInt8) {
Copy link

Choose a reason for hiding this comment

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

Is it worth renaming these (and elsewhere in this PR where we had to relax the access modifier) by prepending a _ (in the case of the inits, maybe append to the argument name)?

Copy link
Author

Choose a reason for hiding this comment

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

We could, but that makes the blast radius of the change a lot larger, diff-wise. I'll take guidance from everyone else on that, but I aimed to keep the diff small.

public static var disallow: Self { .init(uncheckedValue: 3) }

fileprivate let rawValue: UInt8
/* fileprivate but */ @usableFromInline let rawValue: UInt8
Copy link
Contributor

Choose a reason for hiding this comment

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

These are also going to run afoul of the somewhat-debated "no block comments" rule in our standard formatter configuration.

@guoye-zhang
Copy link
Contributor

Do private functions need to be inlineable? I thought the compiler does that automatically.

This does make it harder for me to import and keep the internal version in sync. Will cross-module optimization eliminate the need for doing this manually?

@Lukasa
Copy link
Author

Lukasa commented Feb 22, 2025

The compiler only does that automatically within the module. When private functions are called from inlinable functions they need to themselves be marked inlinable to be eligible for optimisation.

Cross-module optimisation is already turned on and failing to fire. Generally speaking my experience has been that CMO tends to be pretty limited, and frequently fails to fire.

Let’s chat next week about whether we can work together on some automated tooling to add and remove these annotations. We already have some for their removal that you may be able to use.

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

Successfully merging this pull request may close these issues.

4 participants