[ABI] [stdlib] Remove magnitude-based overload of abs(_:). #21037

DougGregor · 2018-12-05T07:12:10Z

The standard library has two versions of the abs(_:) function:

func abs<T : SignedNumeric>(_ x: T) -> T where T.Magnitude == T
func abs<T : SignedNumeric & Comparable>(_ x: T) -> T

The first is more specialized than the second because T.Magnitude is
known to conform to Comparable. Indeed, it’s a more specialized
implementation that returns magnitude.

However, this overload behaves oddly: in the expression abs(-8), the type
checker will pick the first overload because it is more specialized. That’s
a general guiding principle for overloading: pick the most specialized
overload that works.

However, to select that overload, it needs to pick a type for the literal
“8” for which that overload works, and it chooses Double. The “obvious”
answer, Int, doesn’t work because Int.Magnitude == UInt.

There is a conflict between the two rules, here: we prefer more-specialized
overloads (but we’ll fall back to less-specialized if those don’t work) and we prefer to use Int for integer literals (but we’ll fall back to Double if it doesn’t work). We have a few options from a type-checker
perspective:

Consider the more-specialized-function rule to be more important
Consider the integer-literals-prefer-Int rule to be more important
Call the result ambiguous and make the user annotate it

The type checker currently does #1, although at some point in the past it
did #2. Moving forward, #1 is a better choice because it prunes the number
of overloads that need to be considered: if the more-specialized overload
succeeds its type-check, the others need not be considered. It’s also
easier to reason about than the literal-scoring approach, because there can
be a direct definition for “more specialized than” that can be reasoned
about.

I think we should dodge the issue by removing the more-specialized version
of abs(_:). Its use of magnitude seems unlikely to provide a
significant performance benefit, and the presence of overloading either
forces us to consider both overloads always (which is bad for type checker
performance) or accept the regression that abs(-8) is Double. Better
to eliminate the overloading and, if needed in the future, find a better
way to introduce the more-specialized implementation without it being a
separate signature.

Fixes rdar://problem/42345366.

The standard library has two versions of the `abs(_:)` function: ``` func abs<T : SignedNumeric>(_ x: T) -> T where T.Magnitude == T func abs<T : SignedNumeric & Comparable>(_ x: T) -> T ``` The first is more specialized than the second because `T.Magnitude` is known to conform to `Comparable`. Indeed, it’s a more specialized implementation that returns `magnitude`. However, this overload behaves oddly: in the expression `abs(-8)`, the type checker will pick the first overload because it is more specialized. That’s a general guiding principle for overloading: pick the most specialized overload that works. However, to select that overload, it needs to pick a type for the literal “8” for which that overload works, and it chooses `Double`. The “obvious” answer, `Int`, doesn’t work because `Int.Magnitude == UInt`. There is a conflict between the two rules, here: we prefer more-specialized overloads (but we’ll fall back to less-specialized if those don’t work) and we prefer to use `Int` for integer literals (but we’ll fall back to `Double` if it doesn’t work). We have a few options from a type-checker perspective: 1. Consider the more-specialized-function rule to be more important 2. Consider the integer-literals-prefer-`Int` rule to be more important 3. Call the result ambiguous and make the user annotate it The type checker currently does #1, although at some point in the past it did #2. Moving forward, #1 is a better choice because it prunes the number of overloads that need to be considered: if the more-specialized overload succeeds its type-check, the others need not be considered. It’s also easier to reason about than the literal-scoring approach, because there can be a direct definition for “more specialized than” that can be reasoned about. I think we should dodge the issue by removing the more-specialized version of `abs(_:)`. Its use of `magnitude` seems unlikely to provide a significant performance benefit, and the presence of overloading either forces us to consider both overloads always (which is bad for type checker performance) or accept the regression that `abs(-8)` is `Double`. Better to eliminate the overloading and, if needed in the future, find a better way to introduce the more-specialized implementation without it being a separate signature. Fixes rdar://problem/42345366.

DougGregor · 2018-12-05T07:12:18Z

@swift-ci please benchmark

DougGregor · 2018-12-05T07:12:23Z

@swift-ci please smoke test

DougGregor · 2018-12-05T07:13:06Z

Potentially controversial standard library change here ;). Let's see if it affects benchmarks at all.

swift-ci · 2018-12-05T08:01:18Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
Breadcrumbs.CopyUTF16CodeUnits.Mixed	57	68	+19.3%	0.84x
IterateData	1399	1563	+11.7%	0.90x
Improvement
NopDeinit	54193	44961	-17.0%	1.21x
SortStringsUnicode	3594	3210	-10.7%	1.12x
CStringLongAscii	470	422	-10.2%	1.11x

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
MonteCarloPi.o	1593	1641	+3.0%	0.97x
MonteCarloE.o	3396	3444	+1.4%	0.99x
Improvement
Walsh.o	8420	7708	-8.5%	1.09x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
IterateData	1355	1532	+13.1%	0.88x
Integrate	334	362	+8.4%	0.92x
Improvement
SortStringsUnicode	3565	3156	-11.5%	1.13x
Breadcrumbs.CopyUTF16CodeUnits.ASCII	20	18	-10.0%	1.11x

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
MonteCarloPi.o	1578	1610	+2.0%	0.98x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
Integrate	1517	1926	+27.0%	0.79x
ArrayOfGenericPOD2	1066	1180	+10.7%	0.90x (?)
ArrayOfPOD	777	859	+10.6%	0.90x
Improvement
SortStringsUnicode	5180	4820	-6.9%	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: 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

--------------

stephentyrone · 2018-12-05T12:59:09Z

It's not only there for a performance benefit; the SignedNumeric & Comparable implementation is incorrect for types conforming FloatingPoint (it does the wrong thing by failing to negate -0 and nans with the sign bit set).

We can probably address this by adding a FloatingPoint overload.

DougGregor · 2018-12-06T04:47:53Z

In a generic context, we won't get the magnitude-based implementation regardless, and overloading on FloatingPoint will introduce the same problems as the existing overload.

DougGregor · 2018-12-06T04:51:26Z

For example, this does the wrong thing (prints -0.0) in Swift 4.2:

func test<T: SignedNumeric & Comparable>(_ x: T) {
  print(abs(x))
}

test(-0.0)

Within the (single) implementation of abs(_:), dynamically check whether the numeric type and its `Magnitude` are of the same type and, if so, return the result of `magnitude`. This ensures that we do the right thing with respect to (e.g.) floating point values like -0.0, without resorting to overloading of abs(_:).

DougGregor · 2018-12-06T05:07:03Z

Hey @stephentyrone , I'd love your thoughts on the evil trick in ab75577

DougGregor · 2018-12-06T05:07:10Z

@swift-ci please test

DougGregor · 2018-12-06T05:07:18Z

@swift-ci please benchmark

DougGregor · 2018-12-06T05:07:32Z

@swift-ci please benchmark

DougGregor · 2018-12-06T05:07:40Z

@swift-ci please test

swift-ci · 2018-12-06T05:07:49Z

Build failed
Swift Test OS X Platform
Git Sha - 85d488d

swift-ci · 2018-12-06T05:54:11Z

Build comment file:

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Improvement
Walsh.o	8420	7628	-9.4%	1.10x
Integrate.o	2520	2456	-2.5%	1.03x

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Improvement
Walsh.o	6020	5900	-2.0%	1.02x
Integrate.o	2502	2454	-1.9%	1.02x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
Integrate	1509	2059	+36.4%	0.73x

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

--------------

stephentyrone · 2018-12-06T14:22:49Z

Hey @stephentyrone , I'd love your thoughts on the evil trick in ab75577

LGTM.

stephentyrone · 2018-12-06T14:42:37Z

There's a few things about this function that kind of stink, but your changes aren't making it any worse. We should take this as-is, but I may poke at it some more and send you another follow-on PR.

stephentyrone · 2018-12-06T16:03:13Z

(Also, please clone this for 5.0 when you have a chance.)

DougGregor · 2018-12-06T16:36:16Z

Okay!

DougGregor requested review from airspeedswift, moiseev and stephentyrone December 5, 2018 07:12

stephentyrone approved these changes Dec 6, 2018

View reviewed changes

DougGregor merged commit 00d2acd into swiftlang:master Dec 6, 2018

DougGregor deleted the flatten-abs branch December 7, 2018 03:29

xwu mentioned this pull request Jun 24, 2019

[Foundation] Fix two Decimal members with respect to NaN #25698

Merged

[ABI] [stdlib] Remove magnitude-based overload of abs(_:). #21037

[ABI] [stdlib] Remove magnitude-based overload of abs(_:). #21037

Uh oh!

Conversation

DougGregor commented Dec 5, 2018

Uh oh!

DougGregor commented Dec 5, 2018

Uh oh!

DougGregor commented Dec 5, 2018

Uh oh!

DougGregor commented Dec 5, 2018

Uh oh!

swift-ci commented Dec 5, 2018

Build comment file:

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Uh oh!

stephentyrone commented Dec 5, 2018

Uh oh!

DougGregor commented Dec 6, 2018

Uh oh!

DougGregor commented Dec 6, 2018

Uh oh!

DougGregor commented Dec 6, 2018

Uh oh!

DougGregor commented Dec 6, 2018

Uh oh!

DougGregor commented Dec 6, 2018

Uh oh!

DougGregor commented Dec 6, 2018

Uh oh!

DougGregor commented Dec 6, 2018

Uh oh!

swift-ci commented Dec 6, 2018

Uh oh!

swift-ci commented Dec 6, 2018

Build comment file:

Code size: -O

Code size: -Osize

Performance: -Onone

Uh oh!

stephentyrone commented Dec 6, 2018

Uh oh!

stephentyrone commented Dec 6, 2018

Uh oh!

stephentyrone commented Dec 6, 2018

Uh oh!

DougGregor commented Dec 6, 2018

Uh oh!

Uh oh!