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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline all the things! 馃帹 #109

Merged
merged 2 commits into from Mar 23, 2021
Merged

Inline all the things! 馃帹 #109

merged 2 commits into from Mar 23, 2021

Conversation

karwa
Copy link
Contributor

@karwa karwa commented Mar 16, 2021

#92 introduced quite a serious performance regression by refactoring inlinable code in to a helper function which was only usable from inlined code, but not itself inlinable. The first commit corrects this.

Upon further inspection, I noticed a pattern - that all internal functions seemed to only be @usableFromInline and not @inlinable, whereas their public siblings were all @inlinable. Inlining is critical for exactly this kind of code - lazy wrappers and collections and so on, and especially when they are generic. For instance, how is the client's instance of the compiler supposed to be able to optimise Chain, when the implementation of offsetForward is hidden from it?

So the second commit inlines basically everything that was previously @usableFromInline.

Additionally, I would strongly recommend setting up a benchmark suite, so that small refactorings don't cause huge performance regressions in future. I have some concerns about adding swift-algorithms as a dependency for my performance-sensitive project, and a benchmark suite which ensures at least stable baseline performance would help convince me that it's safe to rely on this project.

@karwa karwa mentioned this pull request Mar 16, 2021
@kylemacomber
Copy link
Contributor

@swift-ci please test

@natecook1000
Copy link
Member

Thanks for chasing these down, @karwa! Agreed that this is a better default, and that we do eventually want a wait to keep benchmarks for this package.

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.

None yet

3 participants