FlattenSequence/distance(from:to:) untrapping Int.min #71912

oscbyspro · 2024-02-27T06:18:57Z

This patch makes it so FlattenSequence/distance(from:to:) can return Int.min without trapping. The (#71648) version traps Int.min because 0...Int.max ~= count. I have double-checked, however, and Range/distance(from:to:) (etc.) can return Int.min without trapping:

(Int.min ..< Int.max).distance(from: 0, to: Int.min) // Int.min

I don't believe there are stdlib tests for this, as those would have been too slow before, but perhaps I could add some. But, I haven't yet had time to look at StdlibUnittest.

This patch makes it so FlattenSequence/distance(from:to:) can return Int.min without trapping.

A better, albeit a bit more verbose, method.

1. I added a file with some tests [test/stdlib/FlattenDistanceFromTo.swift]. 2. I used [test/stdlib/StaticBigInt.swift] as the template for the new file.

oscbyspro · 2024-02-28T11:53:04Z

I've checked some more and not all Range/distance(from:to:) allow Int.min:

let range: Range<UInt> = 0 ..< UInt.max/2 + 1
print(range.distance(from: range.endIndex, to: range.startIndex + 1)) // Int.min + 1
print(range.distance(from: range.endIndex, to: range.startIndex + 0)) // Int.min + 0 💥

But, it also looks like that might be fixed by (#71387):

https://github.com/apple/swift/blob/0b5de0652be95f0dd8a4c2ff04c523a6117086c8/stdlib/public/core/Range.swift#L255-L258

1. I removed some command-like comments. 2. I added a test for Int.min and Int.max distances.

oscbyspro · 2024-02-28T14:14:32Z

@glessard can you please test and benchmark this? 🙏

I've untrapped Int.min and added some stdlib tests.

glessard · 2024-02-29T17:09:47Z

@swift-ci please test

glessard · 2024-02-29T17:09:59Z

@swift-ci please benchmark

oscbyspro · 2024-02-29T19:58:52Z

~~[TODO] The new test cases need availability checks ([test] guard a behavioral change #71981).~~

The Int.min and Int.max distance assertions need availability checks because the implementation before (#71648) would count the distance by iterating from one index to the other, taking forever.

oscbyspro · 2024-03-01T08:47:02Z

Hm. I'm looking for FlattenDistanceFromToTests in the logs, but I can't find it. I also can't find any other test/stdlib tests. Have I misunderstood something? For what it's worth, FlattenDistanceFromToTests was made similar to StaticBigIntTests.

1. FlattenSequence/distance(from:to:) cleanup. 2. FlattenDistanceFromToTests cleanup. 3. FlattenDistanceFromToTests availability checks.

oscbyspro · 2024-03-01T10:28:08Z

I believe I have added appropriate availability checks in the most recent commit.

glessard · 2024-03-03T06:00:44Z

@swift-ci please benchmark

glessard · 2024-03-03T06:00:56Z

@swift-ci please test

glessard · 2024-03-07T00:17:39Z

Performance (x86_64): -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListLoop	1021.0	1637.0	+60.3%	0.62x (?)
FlattenListFlatMap	3659.0	4528.0	+23.7%	0.81x (?)
String.replaceSubrange.String	9.556	11.22	+17.4%	0.85x (?)
StringBuilderSmallReservingCapacity	238.6	269.0	+12.7%	0.89x (?)
StringDistance.characters.ascii	2891.0	3245.0	+12.2%	0.89x (?)
StringBuilder	232.5	259.286	+11.5%	0.90x (?)
BridgeString.find.native.longNonASCII	418.75	457.0	+9.1%	0.92x (?)
NSStringConversion.InlineBuffer.UTF8	650.0	707.5	+8.8%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
EqualStringSubstring	31.265	22.84	-26.9%	1.37x
LessSubstringSubstring	30.889	23.767	-23.1%	1.30x
EqualSubstringString	30.92	23.818	-23.0%	1.30x
LessSubstringSubstringGenericComparable	31.027	23.939	-22.8%	1.30x (?)
EqualSubstringSubstring	31.032	23.961	-22.8%	1.30x
EqualSubstringSubstringGenericEquatable	31.12	24.061	-22.7%	1.29x (?)
Data.hash.Medium	30.929	24.303	-21.4%	1.27x (?)
UTF8Decode_InitFromData	173.818	141.714	-18.5%	1.23x (?)
UTF8Decode_InitFromBytes	180.182	147.75	-18.0%	1.22x (?)
UTF8Decode_InitFromCustom_contiguous	172.6	141.923	-17.8%	1.22x
UTF8Decode_InitDecoding	169.167	140.583	-16.9%	1.20x
UTF8Decode_InitFromCustom_noncontiguous	211.444	181.091	-14.4%	1.17x (?)
StringComparison_longSharedPrefix	242.6	208.778	-13.9%	1.16x (?)
String.replaceSubrange.RepChar	1574.0	1426.0	-9.4%	1.10x (?)
FindString.Loop1.Substring	335.0	308.571	-7.9%	1.09x (?)
Breadcrumbs.UTF16ToIdx.longASCII	42.491	39.328	-7.4%	1.08x (?)
CStringLongNonAscii	154.917	144.462	-6.7%	1.07x (?)

Code size: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenDistanceFromTo.o	10793	11321	+4.9%	0.95x

Performance (x86_64): -Osize

Regression	OLD	NEW	DELTA	RATIO
String.replaceSubrange.String	9.646	11.218	+16.3%	0.86x (?)
StringSwitch	225.25	258.857	+14.9%	0.87x (?)
FlattenDistanceFromTo.Array.Array.08.08	68.952	78.895	+14.4%	0.87x
ObjectiveCBridgeStringHash	66.333	74.143	+11.8%	0.89x (?)
NSStringConversion.InlineBuffer.UTF8	638.5	709.0	+11.0%	0.90x (?)
FlattenDistanceFromTo.Array.String.08.08	2210.0	2441.0	+10.5%	0.91x (?)
ConvertFloatingPoint.MockFloat64ToInt64	493.6	542.5	+9.9%	0.91x (?)
FlattenDistanceFromTo.Array.Array.08.04	28.212	30.611	+8.5%	0.92x (?)
Data.hash.Empty	52.261	56.636	+8.4%	0.92x (?)
FlattenDistanceFromTo.Array.Array.04x08	14.87	15.992	+7.5%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
EqualSubstringSubstring	30.259	23.156	-23.5%	1.31x (?)
EqualSubstringSubstringGenericEquatable	30.258	23.159	-23.5%	1.31x
EqualStringSubstring	30.259	23.167	-23.4%	1.31x (?)
LessSubstringSubstring	31.581	24.282	-23.1%	1.30x
EqualSubstringString	31.25	24.156	-22.7%	1.29x (?)
LessSubstringSubstringGenericComparable	31.031	24.375	-21.4%	1.27x
Data.hash.Medium	30.267	24.297	-19.7%	1.25x (?)
UTF8Decode_InitFromBytes	178.3	144.75	-18.8%	1.23x
UTF8Decode_InitFromCustom_contiguous	172.214	140.154	-18.6%	1.23x (?)
UTF8Decode_InitDecoding	169.364	139.182	-17.8%	1.22x
UTF8Decode_InitFromData	174.182	146.75	-15.7%	1.19x (?)
StringComparison_longSharedPrefix	240.1	204.6	-14.8%	1.17x (?)
String.replaceSubrange.RepChar	1582.0	1386.0	-12.4%	1.14x (?)
Breadcrumbs.UTF16ToIdx.longASCII	43.583	39.32	-9.8%	1.11x (?)
StringHashing_slowerPrenormal	252.321	234.688	-7.0%	1.08x (?)
CStringLongNonAscii	153.769	143.615	-6.6%	1.07x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression	OLD	NEW	DELTA	RATIO
ArrayAppendGenericStructs	1432.5	1795.0	+25.3%	0.80x (?)
String.replaceSubrange.String	11.137	12.531	+12.5%	0.89x (?)
ObjectiveCBridgeStringHash	67.706	74.5	+10.0%	0.91x (?)
BridgeString.find.native.longNonASCII	418.0	456.5	+9.2%	0.92x (?)
StringHasSuffixAscii	4875.0	5320.0	+9.1%	0.92x (?)
StringBuilderWithLongSubstring	1885.0	2031.25	+7.8%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
FlattenDistanceFromTo.Array.Array.04x08	2032.0	1509.0	-25.7%	1.35x (?)
FlattenDistanceFromTo.Array.Array.08.08	10012.0	7575.0	-24.3%	1.32x
FlattenDistanceFromTo.Array.Array.04.04	525.333	399.5	-24.0%	1.31x
FlattenDistanceFromTo.Array.Array.08.04	2559.0	1956.0	-23.6%	1.31x
FlattenDistanceFromTo.Array.String.04.04	549.0	425.25	-22.5%	1.29x (?)
EqualSubstringSubstring	35.844	27.923	-22.1%	1.28x (?)
EqualStringSubstring	35.857	27.955	-22.0%	1.28x
EqualSubstringSubstringGenericEquatable	34.235	26.906	-21.4%	1.27x
LessSubstringSubstringGenericComparable	33.44	26.394	-21.1%	1.27x (?)
EqualSubstringString	36.161	28.625	-20.8%	1.26x (?)
FlattenDistanceFromTo.Array.String.08.04	2706.0	2147.0	-20.7%	1.26x (?)
FlattenDistanceFromTo.Array.String.04.08	2226.0	1770.0	-20.5%	1.26x (?)
LessSubstringSubstring	35.172	28.5	-19.0%	1.23x
UTF8Decode_InitFromData	174.111	143.167	-17.8%	1.22x
FlattenDistanceFromTo.Array.String.08.08	11470.0	9468.0	-17.5%	1.21x
UTF8Decode_InitFromBytes	178.167	147.5	-17.2%	1.21x (?)
Data.hash.Medium	34.25	28.48	-16.8%	1.20x (?)
UTF8Decode_InitFromCustom_contiguous	181.0	151.417	-16.3%	1.20x
UTF8Decode_InitDecoding	177.0	148.75	-16.0%	1.19x (?)
String.replaceSubrange.RepChar	1579.0	1391.0	-11.9%	1.14x (?)
Set.isSubset.Seq.Box25	1252.0	1152.5	-7.9%	1.09x (?)

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 mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 32 GB

glessard · 2024-03-07T00:21:55Z

@swift-ci please smoke test macOS platform

oscbyspro · 2024-03-07T11:38:06Z

I think I can fix the -Osize performance regression with some patience on your part.

I see at least two improvement opportunities:

move the first region inside the if-else check (same predicate)
use the forward distance in the last region (as done in the middle)

I've performed some preliminary measurements, and it's ~10% faster on my machine.

Edit: you may also perform a single negation at the end, but that is slower somehow...

Edit: I have updated this PR with the above changes.

1. Put the first region inside the if-else (same branch condition). 2. Use the forward distance in the highest region (as done in the middle).

glessard · 2024-03-08T05:46:42Z

@swift-ci please benchmark

glessard · 2024-03-08T05:47:00Z

@swift-ci please test

oscbyspro · 2024-03-09T07:36:25Z

Performance (x86_64): -O

Regression	OLD	NEW	DELTA	RATIO
StringWithCString2	0.0	0.001	+100.0%	0.50x (?)
FlattenListFlatMap	2658.0	4323.0	+62.6%	0.61x (?)
ArrayAppendGenericStructs	1425.0	1770.0	+24.2%	0.81x (?)
FlattenListLoop	1406.0	1627.0	+15.7%	0.86x (?)

Improvement	OLD	NEW	DELTA	RATIO
String.replaceSubrange.RepChar	1569.0	1372.0	-12.6%	1.14x (?)

Code size: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenDistanceFromTo.o	10793	11561	+7.1%	0.93x

Performance (x86_64): -Osize

Regression	OLD	NEW	DELTA	RATIO
Data.hash.Medium	28.083	30.258	+7.7%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
String.replaceSubrange.RepChar	1597.0	1401.0	-12.3%	1.14x (?)

Code size: -Osize

Regression	OLD	NEW	DELTA	RATIO
FlattenDistanceFromTo.o	10165	10398	+2.3%	0.98x

Performance (x86_64): -Onone

Regression	OLD	NEW	DELTA	RATIO
IndexPath.Max	275.286	328.333	+19.3%	0.84x (?)
Set.isStrictSubset.Seq.Int100	2758.0	3078.0	+11.6%	0.90x (?)
CharIteration_chinese_unicodeScalars	91880.0	100320.0	+9.2%	0.92x (?)
Set.isStrictSubset.Seq.Int50	1408.0	1536.0	+9.1%	0.92x (?)
Set.isStrictSubset.Seq.Int25	719.667	783.0	+8.8%	0.92x (?)

Improvement	OLD	NEW	DELTA	RATIO
FlattenDistanceFromTo.Array.Array.04x08	2054.0	1513.0	-26.3%	1.36x
FlattenDistanceFromTo.Array.Array.04.04	531.333	394.5	-25.8%	1.35x
FlattenDistanceFromTo.Array.String.04.08	2056.0	1533.0	-25.4%	1.34x
FlattenDistanceFromTo.Array.String.04.04	524.667	395.75	-24.6%	1.33x (?)
FlattenDistanceFromTo.Array.String.08.08	10156.0	7715.0	-24.0%	1.32x
FlattenDistanceFromTo.Array.String.08.04	2544.0	1945.0	-23.5%	1.31x (?)
FlattenDistanceFromTo.Array.Array.08.08	9941.0	7631.0	-23.2%	1.30x
FlattenDistanceFromTo.Array.Array.08.04	2534.0	1958.0	-22.7%	1.29x (?)
StringWithCString2	0.007	0.006	-12.5%	1.14x (?)
String.replaceSubrange.RepChar	1597.0	1401.0	-12.3%	1.14x (?)
ArrayAppendAscii	12444.0	11509.0	-7.5%	1.08x (?)

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 mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 32 GB

oscbyspro · 2024-03-09T07:50:53Z

It appears the last commit closed the performance gap. It also added some code size, but perhaps that's OK? In saying this, I have ignored "FlattenListLoop" and "FlattenListFlatMap" because they never call joined(). The latter returns an array, so it uses the eager method instead:

https://github.com/apple/swift/blob/cf619e2a7e483312b8b58c2075285426ffc99f53/stdlib/public/core/SequenceAlgorithms.swift#L753-L763

https://github.com/apple/swift/blob/cf619e2a7e483312b8b58c2075285426ffc99f53/benchmark/single-source/FlattenList.swift#L33-L63

jameesbrown · 2024-04-23T01:29:10Z

Apologies if this is a silly question but (with the exception of code size) what units or by what factors is the performance being measured here?

oscbyspro · 2024-04-23T05:41:31Z

I don't remember the context for this pull request - other than wanting to un-trap the Int.min case. I also don't think there are any public benchmarks targeting this method besides the ones I added in my earlier contributions - so I might have been referring to the "FlattenDistanceFromTo" regression a few comments up.

jameesbrown · 2024-04-25T00:50:02Z

Thanks!

glessard · 2024-05-17T23:16:55Z

@oscbyspro Thanks for this! Sorry for the long delay.

oscbyspro added 3 commits February 27, 2024 07:14

FlattenSequence/distance(from:to:) untrapping Int.min

401f8d1

This patch makes it so FlattenSequence/distance(from:to:) can return Int.min without trapping.

FlattenSequence/distance(from:to:) untrapping Int.min (v2).

e8db705

A better, albeit a bit more verbose, method.

FlattenSequence/distance(from:to:) untrapping Int.min (v3).

e06f80a

1. I added a file with some tests [test/stdlib/FlattenDistanceFromTo.swift]. 2. I used [test/stdlib/StaticBigInt.swift] as the template for the new file.

FlattenSequence/distance(from:to:) untrapping Int.min (v4).

d5e8bdf

1. I removed some command-like comments. 2. I added a test for Int.min and Int.max distances.

oscbyspro marked this pull request as ready for review February 29, 2024 07:30

oscbyspro requested a review from a team as a code owner February 29, 2024 07:30

FlattenSequence/distance(from:to:) untrapping Int.min (v5).

7703ae4

1. FlattenSequence/distance(from:to:) cleanup. 2. FlattenDistanceFromToTests cleanup. 3. FlattenDistanceFromToTests availability checks.

FlattenSequence/distance(from:to:) untrapping Int.min (v6).

af0c9fa

1. Put the first region inside the if-else (same branch condition). 2. Use the forward distance in the highest region (as done in the middle).

glessard merged commit a27d9c4 into swiftlang:main May 17, 2024

FlattenSequence/distance(from:to:) untrapping Int.min #71912

FlattenSequence/distance(from:to:) untrapping Int.min #71912

Uh oh!

Conversation

oscbyspro commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oscbyspro commented Feb 28, 2024

Uh oh!

oscbyspro commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glessard commented Feb 29, 2024

Uh oh!

glessard commented Feb 29, 2024

Uh oh!

oscbyspro commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oscbyspro commented Mar 1, 2024

Uh oh!

oscbyspro commented Mar 1, 2024

Uh oh!

glessard commented Mar 3, 2024

Uh oh!

glessard commented Mar 3, 2024

Uh oh!

glessard commented Mar 7, 2024

Performance (x86_64): -O

Code size: -O

Performance (x86_64): -Osize

Code size: -Osize

Performance (x86_64): -Onone

Code size: -swiftlibs

Uh oh!

glessard commented Mar 7, 2024

Uh oh!

oscbyspro commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glessard commented Mar 8, 2024

Uh oh!

glessard commented Mar 8, 2024

Uh oh!

oscbyspro commented Mar 9, 2024

Performance (x86_64): -O

Code size: -O

Performance (x86_64): -Osize

Code size: -Osize

Performance (x86_64): -Onone

Code size: -swiftlibs

Uh oh!

oscbyspro commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jameesbrown commented Apr 23, 2024

Uh oh!

oscbyspro commented Apr 23, 2024

Uh oh!

jameesbrown commented Apr 25, 2024

Uh oh!

glessard commented May 17, 2024

Uh oh!

Uh oh!

oscbyspro commented Feb 27, 2024 •

edited

Loading

oscbyspro commented Feb 28, 2024 •

edited

Loading

oscbyspro commented Feb 29, 2024 •

edited

Loading

oscbyspro commented Mar 7, 2024 •

edited

Loading

oscbyspro commented Mar 9, 2024 •

edited

Loading