Add a lot of resilience-related annotations to stdlib to improve the performance of the resilient stdlib #7350
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@airspeedswift @dabrahams This is the initial set of annotations, which results in a pretty good performance for many of our benchmarks. Please have a look to see if I touched something that should not be made @_inlineable, @_versioned or @_fixed_layout. The PR does not pass testing yet, because there are about 10 tests that need to be updated. |
|
@slavapestov If you take this PR and revert the last commit: 17ed6ba, then you get a compiler crasher. Could you have a look? I hope it is easy to fix, just like the previous resilience related bugs I've reported. |
|
I'll take a look, thanks @swiftix. |
|
@swiftix Designated initializers on classes cannot be @_inlineable for the same reason as designated initializers on non-fixed-layout value types. Is it important for these initializers to be inlineable? If not, I'll add a diagnostic prohibiting them. If so, we should discuss some options. |
swiftix
force-pushed
the
resilience-benchmarks
branch
from
17ed6ba
to
2237ff2
Compare
|
@slavapestov Thanks for improving the diagnostics. One minor annoying thing I noticed: when the compiler produces a diagnostics that @_inlinable cannot be used with stored properties, it does not show the name of the stored property. Could you add the name of the properties to the message? it is very annoying to search by the line number in gyb generated Swift files ;-) Searching by name in the original gyb/swift files is much easier. |
|
If the line-directive tool is not repointing those errors at the original .gyb files then something is very broken.
Sent from my moss-covered three-handled family gradunza
… On Feb 18, 2017, at 9:20 AM, Roman Levenstein ***@***.***> wrote:
@slavapestov Thanks for improving the diagnostics. One minor annoying thing I noticed: when the compiler produces a diagnostics that @_inlinable cannot be used with stored properties, it does not show the name of the stored property. Could you add the name of the properties to the message? it is very annoying to search by the line number in gyb generated Swift files ;-) Searching by name in the original gyb/swift files is much easier.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
swiftix
force-pushed
the
resilience-benchmarks
branch
from
2237ff2
to
a315513
Compare
|
@airspeedswift @dabrahams Ping! I rebased on ToT. Please have a look. In particular, check if I put @_fixed_layout or @_versioned on types, where you do not want to expose the representation or any details about it. |
|
@swift-ci please smoke test |
slavapestov
reviewed
Mar 9, 2017
| @@ -17,6 +17,7 @@ let arrayCount = 1024 | |||
|
|
|||
| // This test case exposes rdar://17440222 which caused rdar://17974483 (popFront | |||
| // being really slow). | |||
| @_versioned | |||
There was a problem hiding this comment.
Why did the benchmark change?
slavapestov
reviewed
Mar 9, 2017
| @@ -75,6 +75,8 @@ public func autoreleasepoolIfUnoptimizedReturnAutoreleased( | |||
| #endif | |||
| } | |||
|
|
|||
| @_inlineable | |||
| @_versioned | |||
There was a problem hiding this comment.
This function has no body, it should not be @_inlineable (I'll add a diagnostic for that)
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Arrays.swift.gyb
Outdated
| @@ -451,9 +456,9 @@ if True: | |||
| }% | |||
|
|
|||
| ${SelfDocComment} | |||
| % if Self != 'ArraySlice': | |||
| //% if Self != 'ArraySlice': | |||
There was a problem hiding this comment.
Just remove the gybbing here?
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Arrays.swift.gyb
Outdated
| public var startIndex: Int { | ||
| %if Self == 'ArraySlice': | ||
| return _buffer.startIndex | ||
| %else: | ||
| return 0 | ||
| @_inlineable |
There was a problem hiding this comment.
Oops, the @_inlineable should apply to the getter too. Can you revert this and I'll make sure it works?
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Arrays.swift.gyb
Outdated
| @@ -994,6 +1046,8 @@ extension ${Self} : RangeReplaceableCollection, _ArrayProtocol { | |||
| /// emptyArray = [] | |||
| /// print(emptyArray.isEmpty) | |||
| /// // Prints "true" | |||
| // @inline(__always) | |||
slavapestov
reviewed
Mar 9, 2017
| public mutating func next() -> Element? { | ||
| return _base.next().map(_transform) | ||
| } | ||
|
|
||
| @_inlineable | ||
| public var base: Base { return _base } |
There was a problem hiding this comment.
I wonder if this could be private(set) instead
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Map.swift.gyb
Outdated
| public var underestimatedCount: Int { | ||
| return _base.underestimatedCount | ||
| } | ||
|
|
||
| /// Create an instance with elements `transform(x)` for each element | ||
| /// `x` of base. | ||
| //@_inlineable |
There was a problem hiding this comment.
Why wasn't this one inlineable?
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Mirror.swift
Outdated
| @@ -875,6 +876,7 @@ extension String { | |||
| /// // Prints "(21, 30)" | |||
| /// | |||
| /// - SeeAlso: `String.init<Subject>(reflecting: Subject)` | |||
| @_inlineable | |||
There was a problem hiding this comment.
Mirrors are not performance-critical, I think you should remove this
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Optional.swift
Outdated
| @@ -670,10 +676,13 @@ public func ?? <T>(optional: T?, defaultValue: @autoclosure () throws -> T?) | |||
| } | |||
|
|
|||
| extension Optional { | |||
|
|
|||
| @_inlineable | |||
There was a problem hiding this comment.
These are not available and should not be inlineable
slavapestov
reviewed
Mar 9, 2017
| @@ -386,13 +387,17 @@ internal func _print_unlocked<T, TargetStream : TextOutputStream>( | |||
| /// | |||
| /// This function is forbidden from being inlined because when building the | |||
| /// standard library inlining makes us drop the special semantics. | |||
| @_inlineable | |||
There was a problem hiding this comment.
This is inline(never) so it doesn't have to be inlineable either -- you'll just end up emitting useless copies of this into client binaries
There was a problem hiding this comment.
May be we want to specialize it?
There was a problem hiding this comment.
I see. In practice does specializing here make a difference? Perhaps just having some pre-specializations is sufficient.
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Range.swift.gyb
Outdated
| @@ -108,33 +109,43 @@ public struct CountableRange<Bound> : RandomAccessCollection | |||
| public typealias IndexDistance = Bound.Stride | |||
|
|
|||
| public var startIndex: Index { | |||
| return lowerBound | |||
| @_inlineable | |||
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Range.swift.gyb
Outdated
| @@ -173,6 +184,7 @@ public struct CountableRange<Bound> : RandomAccessCollection | |||
| /// (`..<`) to form `CountableRange` instances is preferred. | |||
| /// | |||
| /// - Parameter bounds: A tuple of the lower and upper bounds of the range. | |||
| @inline(__always) | |||
There was a problem hiding this comment.
Why not leave the decision up to the optimizer?
slavapestov
reviewed
Mar 9, 2017
| @@ -1207,6 +1238,7 @@ public func += < | |||
| public typealias RangeReplaceableCollectionType = RangeReplaceableCollection | |||
|
|
|||
| extension RangeReplaceableCollection { | |||
| @_inlineable | |||
There was a problem hiding this comment.
Unavailable methods don't need to be inlineable
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Repeat.swift
Outdated
| @@ -105,6 +112,7 @@ public func repeatElement<T>(_ element: T, count n: Int) -> Repeated<T> { | |||
| public struct Repeat<Element> {} | |||
|
|
|||
| extension Repeated { | |||
| @_inlineable | |||
There was a problem hiding this comment.
Unavailable methods don't need to be inlineable
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Sequence.swift
Outdated
| @@ -1081,6 +1132,7 @@ extension Sequence { | |||
| /// element is a match. | |||
| /// - Returns: The first element of the sequence that satisfies `predicate`, | |||
| /// or `nil` if there is no element that satisfies `predicate`. | |||
| // @_inlineable | |||
slavapestov
reviewed
Mar 9, 2017
stdlib/public/core/Sequence.swift
Outdated
| @@ -1429,16 +1494,19 @@ public typealias GeneratorType = IteratorProtocol | |||
| public typealias SequenceType = Sequence | |||
|
|
|||
| extension Sequence { | |||
| @_inlineable | |||
There was a problem hiding this comment.
Unavailable methods don't need to be inlineable
slavapestov
reviewed
Mar 9, 2017
| final var countAndCapacity: _ArrayBody | ||
|
|
||
| @_inlineable |
There was a problem hiding this comment.
These constructors just throw errors, don't have to be inlinable
slavapestov
reviewed
Mar 9, 2017
| final var countAndCapacity: _ArrayBody | ||
|
|
||
| @_inlineable | ||
| @_versioned |
There was a problem hiding this comment.
I don't think this should be versioned either, it says doNotCallMe :-)
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
@swift-ci please test |
|
@swift-ci please benchmark |
|
Build failed |
|
Build failed |
Build comment file:Optimized (O) Regression (6)
Improvement (6)
No Changes (180)
Regression (1)
Improvement (3)
No Changes (188)
|
swiftix
force-pushed
the
resilience-benchmarks
branch
from
648debb
to
3f40824
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
… mode, when -sil-serialize-all is disabled This commit mostly improves the performance of arrays and ranges. It does not cover Strings, Dictionaries and Sets yet.
swiftix
force-pushed
the
resilience-benchmarks
branch
from
3f40824
to
2c811b8
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
swiftix
added a commit
to swiftix/swift
that referenced
this pull request
Mar 17, 2017
swiftix
added a commit
that referenced
this pull request
Mar 17, 2017
Un-XFAIL some tests which are passing now, after merging PR #7350
|
Having merged this into my branch I am getting piles of errors like and I don't know the rules. @slavapestov @swiftix HALP! |
|
@dabrahams An
So, in your case you most likely want to add If you think that it makes sense to make this internal method also potentially available for inlining (i.e. you want to serialize its SIL body), you can also add |
|
@palimondo Yeah, Filter was probably overlooked. PRs are welcome ;-) |
|
@swiftix I’d love to, but I can not get the swift-source to compile on my machine… any pointers to where I might get help? |
|
@palimondo You could ask questions on the swift-dev@swift.org mailing list. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.