Skip to content

Refactor FixedWidthInteger init #36485

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 1 commit into from
Mar 18, 2021
Merged

Conversation

meg-gupta
Copy link
Contributor

@meg-gupta meg-gupta commented Mar 18, 2021

Without jump threading enabled in ossa, the FixedWidthInteger's init function which is an inline always function causes large cfgs due to the truncation case. This PR separates the truncation case into a different function with no always inlining.

@meg-gupta
Copy link
Contributor Author

@swift-ci benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 4594 6879 +49.7% 0.67x (?)
 
Improvement OLD NEW DELTA RATIO
DictionaryKeysContainsNative 30 28 -6.7% 1.07x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
DictionaryKeysContainsNative 26 29 +11.5% 0.90x (?)
 
Improvement OLD NEW DELTA RATIO
UTF8Decode_InitFromData_ascii_as_ascii 801 745 -7.0% 1.08x (?)

Code size: -Osize

Performance: -Onone

Regression OLD NEW DELTA RATIO
NSError 885 994 +12.3% 0.89x (?)
 
Improvement OLD NEW DELTA RATIO
DataCreateMedium 667900 357100 -46.5% 1.87x
Data.init.Sequence.64kB.Count0 4573 2461 -46.2% 1.86x
Data.init.Sequence.64kB.Count0.I 4543 2467 -45.7% 1.84x
Data.init.Sequence.2047B.Count0.I 7253 3995 -44.9% 1.82x
Data.init.Sequence.2049B.Count0.I 7244 4001 -44.8% 1.81x
Data.init.Sequence.809B.Count0.I 5845 3265 -44.1% 1.79x
Data.append.Sequence.64kB.Count0.I 4477 2518 -43.8% 1.78x
Data.append.Sequence.64kB.Count0 4479 2522 -43.7% 1.78x
Data.init.Sequence.513B.Count0.I 5659 3198 -43.5% 1.77x
Data.init.Sequence.511B.Count0.I 5637 3187 -43.5% 1.77x
Data.init.Sequence.809B.Count0 5834 3311 -43.2% 1.76x
Data.append.Sequence.809B.Count0.I 5686 3255 -42.8% 1.75x
Data.append.Sequence.809B.Count0 5668 3246 -42.7% 1.75x
Data.init.Sequence.64kB.Count 4481 2584 -42.3% 1.73x
Data.init.Sequence.64kB.Count.I 4475 2582 -42.3% 1.73x
Data.append.Sequence.64kB.Count 4479 2585 -42.3% 1.73x
Data.append.Sequence.64kB.Count.I 4476 2584 -42.3% 1.73x
Data.init.Sequence.2049B.Count.I 7035 4065 -42.2% 1.73x
Data.init.Sequence.2047B.Count.I 7018 4062 -42.1% 1.73x
Data.init.Sequence.809B.Count.I 5596 3255 -41.8% 1.72x
DataCreateSmall 78970 46020 -41.7% 1.72x
Data.init.Sequence.513B.Count.I 5360 3151 -41.2% 1.70x
Data.init.Sequence.511B.Count.I 5334 3139 -41.2% 1.70x
Data.init.Sequence.809B.Count 5592 3291 -41.1% 1.70x
Data.append.Sequence.809B.Count.I 5666 3336 -41.1% 1.70x
Data.append.Sequence.809B.Count 5662 3335 -41.1% 1.70x

Code size: -swiftlibs

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

@meg-gupta
Copy link
Contributor Author

@swift-ci test

@meg-gupta meg-gupta requested a review from atrick March 18, 2021 08:01
@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0e5d73d

@meg-gupta meg-gupta changed the title [DNM] Refactor FixedWidthInteger init Refactor FixedWidthInteger init Mar 18, 2021
@meg-gupta meg-gupta marked this pull request as ready for review March 18, 2021 17:11
}

@_alwaysEmitIntoClient
internal static func _truncatingInit<T: BinaryInteger>(_ source: T) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarify if @_alwaysEmitIntoClient is fine to use here

Copy link
Member

@lorentey lorentey Mar 18, 2021

Choose a reason for hiding this comment

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

It looks fine to me from an ABI compatibility viewpoint! 👍

@meg-gupta
Copy link
Contributor Author

@swift-ci test Linux platform

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

LGTM

Someone from the stdlib, @lorentey ? could do a quick sanity check.

I don't think this is a temporary workaround at all. I think it fixes an underlying compile-time and code size problem that jump-threading was somewhat hiding from us. Running jump-threading doesn't change the fact that the compiler is generating a giant CFG to begin with.

@meg-gupta
Copy link
Contributor Author

@lorentey can you please take a look ?

@meg-gupta
Copy link
Contributor Author

@swift-ci test Linux platform

@meg-gupta meg-gupta merged commit dbea482 into swiftlang:main Mar 18, 2021
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