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

Optimize HandySwift for performance-critical code #40

Closed
knothed opened this issue Feb 5, 2020 · 4 comments
Closed

Optimize HandySwift for performance-critical code #40

knothed opened this issue Feb 5, 2020 · 4 comments

Comments

@knothed
Copy link
Contributor

knothed commented Feb 5, 2020

When using HandySwift in performance-critical code, I noticed a bottleneck arising through the missing inlinability of (trivial) HandySwift methods. Because HandySwift's trivial methods (e.g. Int.timesMake) live in another (already compiled) module than the client code, the client compiler cannot inline and optimize these methods. It turned out that reimplementing Int.timesMake myself and putting in the same module was significantly (!) faster than using HandySwift's Int.timesMake.

To be specific, the range-map combination of Int.timesMake ((0 ..< n).map { _ in closure() }) results in cumbersome compiled code which cannot / is not optimized away during HandySwift's compilation, and will therefore always be a performance burden for the client.

Compiled code of HandySwift's Int.timesMake

As Swift strives to be a performant language, very many of Swift's stdlib-implementations are actually annotated with @inlinable or even @_transparent to allow the client compiler to perform inlining and subsequent dataflow-analysis and (aggressive) optimizations.
Examples: Array, Range

Therefore, I propose to add @inlinable or @_transparent to relevant HandySwift methods. These include all Array- / Dictionary- / Collection- / Range-related methods.

@knothed knothed changed the title Add @inlinable to relevant methods Optimize HandySwift for performance-critical code Feb 5, 2020
@Jeehut
Copy link
Member

Jeehut commented Feb 6, 2020

@knothed Hey, nice catch! Sounds very reasonable. Is there any downside in making most of the API @inlinable or @_transparent? If not, I'd happily merge a PR adding these features!

@knothed
Copy link
Contributor Author

knothed commented Feb 10, 2020

Hi @Jeehut, I dont see any downsides with @inlinable.
Citing from the relevant Swift-Evolution-PR:

Any changes to the body of an @inlinable declaration should be considered very carefully. As a general guideline, we feel that @inlinable makes the most sense with "obviously correct" algorithms which manipulate other data types abstractly through protocols, so that any future changes to an @inlinable declaration are optimizations that do not change observed behavior.

Many of the functions of HandySwift (e.g. Int.timesMake, Comparable.clamped(to:), Dictionary.merge, etc.) are such "obviously correct" algorithms, as they are either extremely simple or actually interact on a higher (protocol) level (e.g. using < in Comparable.clamped(to:)).

Some functions do not fall into this "simple" category, like Array.sorted(stable:) (as it performs actual algorithmic work). Nonetheless, I feel it would be against the direction of this proposal to exclude "work-heavy" algorithms like Array.sorted(stable:) from being inlined, as they benefit from being inlined just as all other HandySwift functions.

Regarding _@transparent:
TransparentAttr explains when it is sensible to prefer @_transparent over @inlinable.

The HandySwift functions I referred to as being "simple" fall under this category - they fulfill all requirements for being @_transparent.

The difference of @_transparent and @inlinable is that @_transparent functions are inlined before the compiler performs dataflow analyses. This means, parts of the inlined functions could further be optimized out in some cases, e.g. when arguments are statically known.
I don't know if this is a real performance improvement over @inlinable in real-world use cases.

Therefore, @inlinable seems to be the most sensible option.

@Jeehut
Copy link
Member

Jeehut commented Feb 10, 2020

@knothed Thank you for that detailed explanation, very much appreciated! 👍

I think it makes sense to make relevant APIs @inlinable – I'm looking forward to your PR! 🙂

@Jeehut
Copy link
Member

Jeehut commented Mar 25, 2020

Merged #43, closing this. 🎉

@Jeehut Jeehut closed this as completed Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants