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

Remove useless optional unwrap from Unsafe[Raw]BufferPointer subscript. #22338

Merged
merged 1 commit into from
Feb 6, 2019
Merged

Remove useless optional unwrap from Unsafe[Raw]BufferPointer subscript. #22338

merged 1 commit into from
Feb 6, 2019

Conversation

atrick
Copy link
Contributor

@atrick atrick commented Feb 4, 2019

It is unfortunate that Unsafe[Raw]BufferPointer._pointer and
baseAddress are declared Optional. This leaves extra runtime checks
in the code in the most performance critical paths. Contrast this with
Array, which uses an sentinal pointer for the empty representation.

This forces us to use _unsafelyUnwrappedUnchecked whenever we just
want to dereference a non-empty buffer pointer.

Fixes SR-9809: swiftc appears to make some sub-optimal optimization choices

@atrick
Copy link
Contributor Author

atrick commented Feb 4, 2019

@swift-ci test.

@atrick
Copy link
Contributor Author

atrick commented Feb 4, 2019

@swift-ci benchmark.

@swift-ci
Copy link
Contributor

swift-ci commented Feb 4, 2019

Build comment file:

Performance: -O

TEST OLD NEW DELTA RATIO
Regression
CharIndexing_tweet_unicodeScalars 11280 12760 +13.1% 0.88x
CharIndexing_ascii_unicodeScalars 5760 6480 +12.5% 0.89x
CharIndexing_punctuated_unicodeScalars 1440 1600 +11.1% 0.90x
CharIndexing_chinese_unicodeScalars 5400 5960 +10.4% 0.91x (?)
CharIndexing_japanese_unicodeScalars 8800 9680 +10.0% 0.91x
CharIndexing_korean_unicodeScalars 7120 7800 +9.6% 0.91x
CharIndexing_punctuatedJapanese_unicodeScalars 1440 1560 +8.3% 0.92x
Improvement
Data.hash.Empty 71 65 -8.5% 1.09x

Performance: -Osize

TEST OLD NEW DELTA RATIO
Regression
CharIndexing_ascii_unicodeScalars_Backwards 5560 6320 +13.7% 0.88x
CharIndexing_tweet_unicodeScalars_Backwards 10960 12400 +13.1% 0.88x (?)
InsertCharacterEndIndex 159 173 +8.8% 0.92x
CharIndexing_punctuated_unicodeScalars_Backwards 1400 1520 +8.6% 0.92x (?)
SortStringsUnicode 3336 3593 +7.7% 0.93x
Improvement
Data.hash.Empty 74 68 -8.1% 1.09x (?)
ObjectiveCBridgeStringHash 52 48 -7.7% 1.08x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Regression
ArrayOfGenericPOD2 1065 1235 +16.0% 0.86x (?)
Dictionary3 610 707 +15.9% 0.86x
ArrayOfPOD 776 855 +10.2% 0.91x (?)
StrComplexWalk 6860 7530 +9.8% 0.91x
ObjectiveCBridgeStubDateMutation 742 800 +7.8% 0.93x (?)
Improvement
ObjectiveCBridgeStubToNSDate2 1474 1361 -7.7% 1.08x (?)
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
--------------

@swift-ci
Copy link
Contributor

swift-ci commented Feb 4, 2019

Build failed
Swift Test Linux Platform
Git Sha - 3e1781999350fc29cef3540b79711e04eae59341

It is unfortunate that `Unsafe[Raw]BufferPointer._pointer` and
`baseAddress` are declared Optional. This leaves extra runtime checks
in the code in the most performance critical paths. Contrast this with
Array, which uses an sentinal pointer for the empty representation.

This forces us to use _unsafelyUnwrappedUnchecked whenever we just
want to dereference a non-empty buffer pointer.

Fixes SR-9809: swiftc appears to make some sub-optimal optimization choices
@atrick
Copy link
Contributor Author

atrick commented Feb 4, 2019

@swift-ci smoke test.

@jrose-apple
Copy link
Contributor

(For the record, they're optional so that they round-trip with C pointer/length pairs, which usually accept NULL/0. But as Andy points out, that's not interesting once we've already checked that we have an in-bound index.)

@milseman
Copy link
Member

milseman commented Feb 4, 2019

If you want to keep it as a debugPrecondition, like the others in those methods, you should use unsafelyUnwrapped. _unsafelyUnwrappedUnchecked is a internalInvariant.

@milseman
Copy link
Member

milseman commented Feb 4, 2019

@swift-ci please benchmark

@milseman
Copy link
Member

milseman commented Feb 4, 2019

(I've seen issues where removing force unwraps slows down benchmarks due to unpredictability of the optimizer, so double checking here)

@swift-ci
Copy link
Contributor

swift-ci commented Feb 5, 2019

Build comment file:

Performance: -O

TEST OLD NEW DELTA RATIO
Regression
DictionaryBridgeToObjC_Access 921 1064 +15.5% 0.87x (?)
CharIndexing_ascii_unicodeScalars 5160 5840 +13.2% 0.88x
CharIndexing_tweet_unicodeScalars 10120 11440 +13.0% 0.88x
Data.hash.Empty 58 64 +10.3% 0.91x (?)
CharIndexing_japanese_unicodeScalars 7920 8680 +9.6% 0.91x
CharIndexing_punctuated_unicodeScalars 1280 1400 +9.4% 0.91x
CharIndexing_punctuatedJapanese_unicodeScalars 1280 1400 +9.4% 0.91x
CharIndexing_korean_unicodeScalars 6400 6960 +8.7% 0.92x
LessSubstringSubstring 36 39 +8.3% 0.92x
LessSubstringSubstringGenericComparable 36 39 +8.3% 0.92x
CharIndexing_chinese_unicodeScalars 4840 5240 +8.3% 0.92x (?)
Improvement
DataSetCountSmall 128 117 -8.6% 1.09x (?)

Performance: -Osize

TEST OLD NEW DELTA RATIO
Regression
CharIndexing_tweet_unicodeScalars_Backwards 9800 11120 +13.5% 0.88x
CharIndexing_ascii_unicodeScalars_Backwards 5000 5640 +12.8% 0.89x
CharIndexing_punctuated_unicodeScalars_Backwards 1240 1360 +9.7% 0.91x (?)
InsertCharacterEndIndex 143 156 +9.1% 0.92x (?)
LessSubstringSubstring 36 39 +8.3% 0.92x (?)
SortStringsUnicode 2994 3225 +7.7% 0.93x
Improvement
DataSetCountSmall 128 117 -8.6% 1.09x (?)

Performance: -Onone

TEST OLD NEW DELTA RATIO
Regression
ArrayOfGenericPOD2 954 1108 +16.1% 0.86x (?)
Dictionary3 547 635 +16.1% 0.86x
ArrayOfPOD 689 763 +10.7% 0.90x (?)
StrComplexWalk 6190 6750 +9.0% 0.92x
StringAdder 604 655 +8.4% 0.92x (?)
Data.hash.Empty 135 146 +8.1% 0.92x
Improvement
ArrayAppendAscii 24412 22814 -6.5% 1.07x (?)
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: 8-Core Intel Xeon E5
  Processor Speed: 3 GHz
  Number of Processors: 1
  Total Number of Cores: 8
  L2 Cache (per Core): 256 KB
  L3 Cache: 25 MB
  Memory: 64 GB
--------------

@atrick
Copy link
Contributor Author

atrick commented Feb 5, 2019

I have a number of patches that fix inlining and pass manager problems but haven't had time to look into the benchmark numbers for those either. So for now I'd say this is blocked on other fixes.

@milseman
Copy link
Member

milseman commented Feb 5, 2019

E.g. #21132, which just removes branches, slowing down benchmarks.

@atrick
Copy link
Contributor Author

atrick commented Feb 6, 2019

Merging this unambiguously good bug fix and recording the regressions here so we can fix the inliner...

https://bugs.swift.org/browse/SR-9875
SR-9875 Optimizer's inlining heuristics introducing instability

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