Skip to content

SR-10294: convertBoolToDarwinBool and friends should be inlined #23802

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 15, 2019
Merged

SR-10294: convertBoolToDarwinBool and friends should be inlined #23802

merged 1 commit into from
Apr 15, 2019

Conversation

Catfish-Man
Copy link
Contributor

@Catfish-Man Catfish-Man commented Apr 4, 2019 •
edited
Loading

Fixes rdar://problem/49926079

@Catfish-Man Catfish-Man requested a review from mikeash April 4, 2019 21:32
@Catfish-Man Catfish-Man self-assigned this Apr 4, 2019
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@swift-ci
Copy link
Contributor

swift-ci commented Apr 4, 2019

Performance: -O

TEST OLD NEW DELTA RATIO
Regression
FlattenListLoop 3976 4333 +9.0% 0.92x
RemoveWhereMoveInts 34 37 +8.8% 0.92x (?)
Array2D 6928 7520 +8.5% 0.92x
RemoveWhereSwapInts 60 65 +8.3% 0.92x
ObjectiveCBridgeStubNSDateRefAccess 343 371 +8.2% 0.92x (?)
ObjectiveCBridgeStubFromNSDateRef 4060 4390 +8.1% 0.92x (?)
ObjectiveCBridgeStubFromNSDate 6650 7190 +8.1% 0.92x (?)
FlattenListFlatMap 6073 6532 +7.6% 0.93x (?)
Improvement
CharacterPropertiesStashed 1640 1230 -25.0% 1.33x
CharacterPropertiesStashedMemo 1550 1250 -19.4% 1.24x
CharacterLiteralsLarge 108 97 -10.2% 1.11x

Performance: -Osize

TEST OLD NEW DELTA RATIO
Improvement
CharacterPropertiesStashed 1840 1470 -20.1% 1.25x
CharacterPropertiesStashedMemo 1790 1500 -16.2% 1.19x
CharacterPropertiesFetch 4600 4150 -9.8% 1.11x (?)
ObjectiveCBridgeStubFromNSDateRef 4390 4000 -8.9% 1.10x (?)

Performance: -Onone

TEST OLD NEW DELTA RATIO
Improvement
CharacterPropertiesStashed 3250 2590 -20.3% 1.25x (?)
CharacterPropertiesStashedMemo 4290 3910 -8.9% 1.10x (?)
CharacterPropertiesFetch 6050 5540 -8.4% 1.09x (?)

Code size: -swiftlibs

TEST OLD NEW DELTA RATIO
Improvement
libswiftCloudKit.dylib 77824 73728 -5.3% 1.06x
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

@Catfish-Man Catfish-Man requested a review from jrose-apple April 4, 2019 23:58
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

jrose-apple
jrose-apple reviewed Apr 5, 2019
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

It looks good to me, but it'd be nice to know what changed in the optimized build benchmarks.

jrose-apple
jrose-apple reviewed Apr 5, 2019
View reviewed changes
stdlib/public/Darwin/ObjectiveC/ObjectiveC.swift
@@ -27,26 +27,30 @@ import _SwiftObjectiveCOverlayShims
public struct ObjCBool : ExpressibleByBooleanLiteral {
#if os(macOS) || (os(iOS) && (arch(i386) || arch(arm)))
// On OS X and 32-bit iOS, Objective-C's BOOL type is a "signed char".
var _value: Int8
@usableFromInline var _value: Int8
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically cheating because this wasn't ABI in 5.0 and therefore there wasn't a symbol there, so we have to really cross our fingers that nothing will stay referencing the symbol. That seems pretty likely, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah hm good point >.<

What about at -Onone? Seems like there’s more risk there maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think since the struct is fixed-layout it should be okay. It's still usable-from-inline and not public so it's not like it can be used in arbitrary ways.

@Catfish-Man
Copy link
Contributor Author

I think the benchmark regressions are noise. Most of them are ‘?’ and I would be surprised if most of them use DarwinBool at all; it’s not real common.

@jrose-apple
Copy link
Contributor

Yes, but you've got ObjCBool in there too, which is a little more common. (Still seems unlikely, but…)

@Catfish-Man Catfish-Man force-pushed the transparently-false branch from f0c9923 to 2ebfe98 Compare April 5, 2019 20:29
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@swift-ci
Copy link
Contributor

swift-ci commented Apr 5, 2019

No performance and code size changes

@Catfish-Man
Copy link
Contributor Author

Intriguing.

@Catfish-Man
Copy link
Contributor Author

Looks like we don't get to do this until the compiler crash is fixed

@Catfish-Man Catfish-Man closed this Apr 5, 2019
@jrose-apple
Copy link
Contributor

I mean, that's not too surprising to me. Our benchmarks mostly aren't trafficking in ObjCBool anyway, and I'm sure none of them are testing DarwinBoolean.

@Catfish-Man Catfish-Man reopened this Apr 10, 2019
@Catfish-Man
Copy link
Contributor Author

Blocking bug was fixed

@Catfish-Man Catfish-Man force-pushed the transparently-false branch from 2ebfe98 to cc202b1 Compare April 10, 2019 21:31
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@swift-ci
Copy link
Contributor

Performance: -O

TEST OLD NEW DELTA RATIO
Regression
ObjectiveCBridgeStubFromNSDateRef 4060 4640 +14.3% 0.88x (?)
FlattenListLoop 4334 4709 +8.7% 0.92x
ObjectiveCBridgeStubNSDateRefAccess 343 371 +8.2% 0.92x (?)
Array2D 7520 8112 +7.9% 0.93x (?)
RemoveWhereSwapInts 65 70 +7.7% 0.93x
Improvement
CharacterPropertiesStashed 1660 1230 -25.9% 1.35x
DataSetCountSmall 160 125 -21.9% 1.28x
CharacterPropertiesStashedMemo 1540 1250 -18.8% 1.23x
CharacterLiteralsLarge 108 97 -10.2% 1.11x (?)
CharacterPropertiesFetch 4280 3970 -7.2% 1.08x (?)

Performance: -Osize

TEST OLD NEW DELTA RATIO
Improvement
DataSetCountSmall 160 125 -21.9% 1.28x
CharacterPropertiesStashed 1820 1460 -19.8% 1.25x
CharacterPropertiesStashedMemo 1780 1470 -17.4% 1.21x
CharacterPropertiesFetch 4480 4130 -7.8% 1.08x (?)
IterateData 1465 1360 -7.2% 1.08x (?)

Performance: -Onone

TEST OLD NEW DELTA RATIO
Improvement
CharacterPropertiesStashed 3250 2630 -19.1% 1.24x
DataSetCountSmall 214 180 -15.9% 1.19x
CharacterPropertiesStashedMemo 4290 3890 -9.3% 1.10x (?)
CharacterPropertiesFetch 6020 5590 -7.1% 1.08x (?)

Code size: -swiftlibs

TEST OLD NEW DELTA RATIO
Improvement
libswiftCloudKit.dylib 77824 73728 -5.3% 1.06x
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

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man
Copy link
Contributor Author

Dammit, accidentally got another branch mixed in there >.<

@Catfish-Man Catfish-Man force-pushed the transparently-false branch from 7f5fb02 to ca5e554 Compare April 12, 2019 23:11
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man
Copy link
Contributor Author

Yaaay tests passed

@stephentyrone stephentyrone self-requested a review April 15, 2019 15:53
stephentyrone
stephentyrone approved these changes Apr 15, 2019
View reviewed changes
Copy link
Contributor

@stephentyrone stephentyrone left a comment

Choose a reason for hiding this comment

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

LGTM. DarwinBoolean.== could probably also be transparent, but I'm not sure that it matters much.

@Catfish-Man Catfish-Man force-pushed the transparently-false branch from ca5e554 to 7db8cb1 Compare April 15, 2019 16:00
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

4 similar comments
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 73edb17 into swiftlang:master Apr 15, 2019
@Catfish-Man Catfish-Man deleted the transparently-false branch April 15, 2019 23:31
@xedin xedin mentioned this pull request Apr 16, 2019
Merged
@atrick atrick mentioned this pull request Apr 16, 2019
Closed
@Catfish-Man Catfish-Man restored the transparently-false branch April 16, 2019 22:10
@jrose-apple jrose-apple mentioned this pull request Apr 23, 2019
Merged
@Catfish-Man Catfish-Man mentioned this pull request May 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrose-apple jrose-apple jrose-apple left review comments

@stephentyrone stephentyrone stephentyrone approved these changes

@mikeash mikeash Awaiting requested review from mikeash

Assignees

@Catfish-Man Catfish-Man

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Catfish-Man @swift-ci @jrose-apple @stephentyrone