Skip to content

add String(decoding:as:) benchmarks for Unsafe(Mutable)(Raw)BufferPoi… #21706

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

Closed
wants to merge 1 commit into from

Conversation

weissi
Copy link
Contributor

@weissi weissi commented Jan 8, 2019

…nters

in #21611 I added _HasContiguousBytes conformances for UnsafeRawBufferPointer which should make String decoding from them a lot faster. But this needs a benchmark. This adds UTF8 decoding benchmarks for all Unsafe(Mutable)(Raw)BufferPointer(<UInt8>)s and their slices.

@weissi weissi requested a review from milseman January 8, 2019 11:56
@weissi
Copy link
Contributor Author

weissi commented Jan 8, 2019

@swift-ci Please benchmark

@weissi weissi force-pushed the jw-str-decoding-benchs branch from b7cd394 to 2f572bf Compare January 8, 2019 13:48
@swiftlang swiftlang deleted a comment from swift-ci Jan 8, 2019
@weissi
Copy link
Contributor Author

weissi commented Jan 8, 2019

@swift-ci Please benchmark

@swift-ci
Copy link
Contributor

swift-ci commented Jan 8, 2019

Build comment file:

Performance: -O

TEST MIN MAX MEAN MAX_RSS
Added
String.init.decodingUTF8.UBP 505 505 505
String.init.decodingUTF8.UBPSlice 799 800 799
String.init.decodingUTF8.UMBP 505 505 505
String.init.decodingUTF8.UMBPSlice 799 834 811
String.init.decodingUTF8.UMRBP 506 506 506
String.init.decodingUTF8.UMRBPSlice 602 604 603
String.init.decodingUTF8.URBP 515 515 515
String.init.decodingUTF8.URBPSlice 596 598 597

Code size: -O

TEST OLD NEW DELTA RATIO
Regression
UTF8Decode.o 12574 21406 +70.2% 0.59x

Performance: -Osize

TEST MIN MAX MEAN MAX_RSS
Added
String.init.decodingUTF8.UBP 515 561 534
String.init.decodingUTF8.UBPSlice 858 860 859
String.init.decodingUTF8.UMBP 515 516 515
String.init.decodingUTF8.UMBPSlice 833 835 834
String.init.decodingUTF8.UMRBP 505 506 505
String.init.decodingUTF8.UMRBPSlice 622 625 623
String.init.decodingUTF8.URBP 506 506 506
String.init.decodingUTF8.URBPSlice 623 625 624

Code size: -Osize

TEST OLD NEW DELTA RATIO
Regression
UTF8Decode.o 10990 19150 +74.2% 0.57x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Improvement
DictionaryKeysContainsCocoa 72 59 -18.1% 1.22x
Added
String.init.decodingUTF8.UBP 578 1474 877
String.init.decodingUTF8.UBPSlice 882 884 883
String.init.decodingUTF8.UMBP 626 628 627
String.init.decodingUTF8.UMBPSlice 904 907 905
String.init.decodingUTF8.UMRBP 644 644 644
String.init.decodingUTF8.UMRBPSlice 629 631 630
String.init.decodingUTF8.URBP 562 563 563
String.init.decodingUTF8.URBPSlice 607 608 607
Benchmark Check Report
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB
--------------

@milseman
Copy link
Member

milseman commented Jan 8, 2019

I feel like benchmarks for this don't give us any useful signal, they're just there to catch unintended regressions. @eeckstein, can we instead formulate these as SIL optimizer tests? We want the init decoding with UnsafeBufferPointer<UInt8>/UnsafeMutableBufferPointer<UInt8>/UnsafeRawBufferPointer/UnsafeMutableRawBufferPointer to all compile to the same thing: just get the pointer and go. Array/String/Substring as well, though they may need some introspection and fall-back paths in place. The String initializer is inlinable (and inline always, actually) to fold away this check.

Basically:

  public init(decoding:as:)<C: Collection, Encoding: Unicode.Encoding>(
    decoding codeUnits: C, as sourceEncoding: Encoding.Type
  ) where C.Iterator.Element == Encoding.CodeUnit {
    if let contigBytes = codeUnits as? _HasContiguousBytes,
       sourceEncoding == UTF8.self,
       contigBytes._providesContiguousBytesNoCopy
    {
      // ... calls String._fromUTF8Repairing ...
    }

    // ... calls String._fromCodeUnits(_:encoding:repair:) ...
  }

So we can just test that only the _fromUTF8Repairing is codegen-ed. But, I don't know how to make such a test be less coupled from stdlib implementation details. Any ideas @eeckstein ?

@eeckstein
Copy link
Contributor

@milseman Definitely! If we expect the optimizer to compile a function down to a trivial thing, a SIL (or LLVM IR) FileCheck test is better than a benchmark. Examples are https://github.com/apple/swift/blob/master/test/SILOptimizer/character_literals.swift or https://github.com/apple/swift/blob/master/test/SILOptimizer/optionset.swift.

I would not be worried too much about testing against internals of the stdlib (specifically usableFromInline functions) because with ABI stability those shouldn't change anyway.

@milseman
Copy link
Member

milseman commented Jan 9, 2019

SGTM. @weissi, let me know if you'd like some help writing the tests.

@weissi
Copy link
Contributor Author

weissi commented Jan 9, 2019

@eeckstein / @milseman does this sound about right? #21743

@weissi
Copy link
Contributor Author

weissi commented Jan 21, 2019

closing for #21743

@weissi weissi closed this Jan 21, 2019
@weissi weissi deleted the jw-str-decoding-benchs branch January 21, 2019 11:33
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.

4 participants