Skip to content

[benchmark] Add mock floating-point types for conversion benchmarks #33821

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

Merged
merged 2 commits into from
Sep 6, 2020

Conversation

xwu
Copy link
Collaborator

@xwu xwu commented Sep 5, 2020

This is a companion PR which includes, for benchmarking purposes, a protocol MockBinaryFloatingPoint with default implementations that allows easy creation of "newtypes" that wrap existing binary floating-point types.

This permits performance testing of generic conversions between binary floating-point types where one or both are not known to the standard library, a scenario which is encountered in real-world libraries such as SwiftUI that convert between CGFloat and standard library types.

@xwu
Copy link
Collaborator Author

xwu commented Sep 5, 2020

@swift-ci benchmark

@xwu xwu requested a review from benrimmington September 5, 2020 03:49
@swift-ci
Copy link
Contributor

swift-ci commented Sep 5, 2020

Performance: -O

Improvement OLD NEW DELTA RATIO
CharacterLiteralsLarge 108 97 -10.2% 1.11x
ObjectiveCBridgeStubFromNSStringRef 162 151 -6.8% 1.07x (?)
CharacterLiteralsSmall 340 317 -6.8% 1.07x (?)
 
Added MIN MAX MEAN MAX_RSS
ConvertFloatingPoint.MockFloat64ToDouble 3978 4214 4072

Code size: -O

Regression OLD NEW DELTA RATIO
FloatingPointConversion.o 1775 26662 +1402.1% 0.07x

Performance: -Osize

Regression OLD NEW DELTA RATIO
StringFromLongWholeSubstring 4 5 +25.0% 0.80x
CharacterLiteralsLarge 100 111 +11.0% 0.90x
Array2D 6944 7520 +8.3% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
Breadcrumbs.UTF16ToIdxRange.longASCII 33 29 -12.1% 1.14x
ObjectiveCBridgeStubFromNSStringRef 171 151 -11.7% 1.13x (?)
 
Added MIN MAX MEAN MAX_RSS
ConvertFloatingPoint.MockFloat64ToDouble 3980 4127 4030

Code size: -Osize

Regression OLD NEW DELTA RATIO
FloatingPointConversion.o 1681 25233 +1401.1% 0.07x

Performance: -Onone

Regression OLD NEW DELTA RATIO
RandomTree.insert.Unmanaged.fast 771 841 +9.1% 0.92x (?)
 
Added MIN MAX MEAN MAX_RSS
ConvertFloatingPoint.MockFloat64ToDouble 6993 7362 7131

Code size: -swiftlibs

Benchmark Check Report
⚠️🔤 ConvertFloatingPoint.MockFloat64ToDouble name is composed of 5 words.
Split ConvertFloatingPoint.MockFloat64ToDouble name into dot-separated groups and variants. See http://bit.ly/BenchmarkNaming
⚠️ ConvertFloatingPoint.MockFloat64ToDouble execution took at least 3951 μs.
Decrease the workload of ConvertFloatingPoint.MockFloat64ToDouble by a factor of 4 (10), to be less than 1000 μs.
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

Copy link
Contributor

@benrimmington benrimmington left a comment

Choose a reason for hiding this comment

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

I'm not the best person to review this benchmark.

Can the warnings about naming (>40 chars) and workload (>1000 μs) be ignored?

@xwu
Copy link
Collaborator Author

xwu commented Sep 5, 2020

@benrimmington Thanks for taking a look. I'll see if someone else can chime in as well on this.

The warnings about the name length look to be false positives, as the suggested remedy is to use dots to break up the name--which we already do! The workload issue is definitely something I'm paying attention to; in all cases, the benchmarks are intended to perform under 1000 µs imminently, but if I change the workload upfront, then some of them will be very noisy as they'll run in the <20 µs range after the improvements land.

@xwu xwu requested a review from palimondo September 5, 2020 15:29
@xwu xwu marked this pull request as ready for review September 5, 2020 15:30
@xwu
Copy link
Collaborator Author

xwu commented Sep 6, 2020

Since I'm eager to see how much these benchmarks can be improved on CI with the mentioned PR, I'm going to go ahead and merge this. Happy to work with folks in the know to correct any naming issues that come up.

@xwu

This comment has been minimized.

1 similar comment
@xwu
Copy link
Collaborator Author

xwu commented Sep 6, 2020

@swift-ci smoke test and merge

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.

3 participants