Add a private implementation of a String initializer with access to uninitialized storage (https://github.com/apple/swift-evolution/pull/1022) and use it to speed up uppercased() and lowercased() #26007

Catfish-Man · 2019-07-08T22:04:59Z

Fixes https://bugs.swift.org/browse/SR-11069

Catfish-Man · 2019-07-08T22:05:57Z

@swift-ci please test

Catfish-Man · 2019-07-08T22:06:04Z

@swift-ci please benchmark

swift-ci · 2019-07-08T22:12:10Z

Build failed
Swift Test OS X Platform
Git Sha - 393a1dfd2b2e29807638598cbc38c4aa1680be4e

Catfish-Man · 2019-07-08T22:24:36Z

@swift-ci please test

Catfish-Man · 2019-07-08T22:24:45Z

@swift-ci please benchmark

swift-ci · 2019-07-08T22:24:53Z

Build failed
Swift Test OS X Platform
Git Sha - 393a1dfd2b2e29807638598cbc38c4aa1680be4e

swift-ci · 2019-07-08T22:26:49Z

Build failed
Swift Test Linux Platform
Git Sha - 393a1dfd2b2e29807638598cbc38c4aa1680be4e

swift-ci · 2019-07-08T22:50:18Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
StringHasSuffixAscii	1170	1280	+9.4%	0.91x

Improvement	OLD	NEW	DELTA	RATIO
AngryPhonebook.ASCII	382	11	-97.1%	34.72x
AngryPhonebook.ASCII.Small	78	12	-84.6%	6.50x
AngryPhonebook	1764	273	-84.5%	6.46x
Array2D	3920	3472	-11.4%	1.13x
MapReduce	215	192	-10.7%	1.12x (?)
MapReduceAnyCollection	216	193	-10.6%	1.12x (?)
FlattenListFlatMap	4513	4043	-10.4%	1.12x (?)
RemoveWhereSwapInts	34	31	-8.8%	1.10x (?)
FlattenListLoop	2982	2728	-8.5%	1.09x (?)
ArraySetElement	262	240	-8.4%	1.09x (?)
Calculator	146	134	-8.2%	1.09x (?)
RandomShuffleLCG2	400	368	-8.0%	1.09x
RemoveWhereFilterInts	25	23	-8.0%	1.09x
PrefixWhileSequence	199	185	-7.0%	1.08x (?)
PrefixWhileAnySeqCntRange	208	194	-6.7%	1.07x (?)
ArrayLiteral2	76	71	-6.6%	1.07x (?)

Added	MIN	MAX	MEAN	MAX_RSS
NSStringConversion.Medium	217	230	221	—
NSStringConversion.Rebridge	143	143	143	—
NSStringConversion.Rebridge.Long	136	136	136	—
NSStringConversion.Rebridge.LongUTF8	41	41	41	—
NSStringConversion.Rebridge.Medium	133	136	135	—
NSStringConversion.Rebridge.Mutable	526	526	526	—
NSStringConversion.Rebridge.UTF8	336	342	338	—

Code size: -O

Regression	OLD	NEW	DELTA	RATIO
NSStringConversion.o	4639	9243	+99.2%	0.50x

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
StringHasSuffixAscii	1130	1240	+9.7%	0.91x

Improvement	OLD	NEW	DELTA	RATIO
AngryPhonebook.ASCII	383	11	-97.1%	34.82x
AngryPhonebook.ASCII.Small	77	12	-84.4%	6.42x
AngryPhonebook	1722	273	-84.1%	6.31x
RemoveWhereSwapInts	35	31	-11.4%	1.13x (?)
Array2D	4144	3696	-10.8%	1.12x (?)
MapReduce	237	215	-9.3%	1.10x (?)
FlattenListLoop	2364	2146	-9.2%	1.10x (?)
MapReduceAnyCollection	254	233	-8.3%	1.09x (?)
RandomShuffleLCG2	400	368	-8.0%	1.09x (?)
RemoveWhereFilterInts	25	23	-8.0%	1.09x (?)
ArraySetElement	283	262	-7.4%	1.08x (?)

Added	MIN	MAX	MEAN	MAX_RSS
NSStringConversion.Medium	211	211	211	—
NSStringConversion.Rebridge	138	145	141	—
NSStringConversion.Rebridge.Long	134	134	134	—
NSStringConversion.Rebridge.LongUTF8	40	40	40	—
NSStringConversion.Rebridge.Medium	133	133	133	—
NSStringConversion.Rebridge.Mutable	525	525	525	—
NSStringConversion.Rebridge.UTF8	335	335	335	—

Code size: -Osize

Regression	OLD	NEW	DELTA	RATIO
NSStringConversion.o	4324	8043	+86.0%	0.54x

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
ObjectiveCBridgeStubToNSDate2	1000	1100	+10.0%	0.91x (?)

Improvement	OLD	NEW	DELTA	RATIO
AngryPhonebook.ASCII	383	11	-97.1%	34.82x
AngryPhonebook.ASCII.Small	167	101	-39.5%	1.65x
AngryPhonebook	3787	2296	-39.4%	1.65x

Added	MIN	MAX	MEAN	MAX_RSS
NSStringConversion.Medium	1407	1465	1426	—
NSStringConversion.Rebridge	152	152	152	—
NSStringConversion.Rebridge.Long	139	139	139	—
NSStringConversion.Rebridge.LongUTF8	41	42	41	—
NSStringConversion.Rebridge.Medium	139	139	139	—
NSStringConversion.Rebridge.Mutable	549	549	549	—
NSStringConversion.Rebridge.UTF8	349	349	349	—

Code size: -swiftlibs

✅	Benchmark Check Report

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: 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: 64 GB

milseman

Need to fix nul-termination, other than that LGTM

stdlib/public/core/String.swift

stdlib/public/core/StringStorage.swift

stdlib/public/core/StringCreate.swift

stdlib/public/core/StringStorage.swift

Catfish-Man · 2019-07-08T23:57:24Z

@swift-ci please test

Catfish-Man · 2019-07-08T23:57:52Z

@swift-ci please benchmark

swift-ci · 2019-07-08T23:59:34Z

Build failed
Swift Test Linux Platform
Git Sha - 68eac61ac5e8debdc066491b0f36bf272e06d43d

swift-ci · 2019-07-09T00:01:19Z

Build failed
Swift Test OS X Platform
Git Sha - 68eac61ac5e8debdc066491b0f36bf272e06d43d

swift-ci · 2019-07-09T00:45:06Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListLoop	4706	5191	+10.3%	0.91x (?)
SortLettersInPlace	515	562	+9.1%	0.92x (?)
EqualSubstringSubstring	39	42	+7.7%	0.93x (?)
EqualSubstringSubstringGenericEquatable	39	42	+7.7%	0.93x

Improvement	OLD	NEW	DELTA	RATIO
AngryPhonebook.ASCII	528	17	-96.8%	31.06x
AngryPhonebook.ASCII.Small	121	16	-86.8%	7.56x
AngryPhonebook	2751	371	-86.5%	7.42x
Array2D	8112	7520	-7.3%	1.08x (?)
RemoveWhereSwapInts	70	65	-7.1%	1.08x
MapReduce	425	397	-6.6%	1.07x (?)
MapReduceAnyCollection	426	398	-6.6%	1.07x (?)

Added	MIN	MAX	MEAN	MAX_RSS
NSStringConversion.Medium	399	399	399	—
NSStringConversion.Rebridge	243	248	245	—
NSStringConversion.Rebridge.Long	186	188	187	—
NSStringConversion.Rebridge.LongUTF8	56	58	57	—
NSStringConversion.Rebridge.Medium	185	187	186	—
NSStringConversion.Rebridge.Mutable	909	914	912	—
NSStringConversion.Rebridge.UTF8	480	482	481	—

Code size: -O

Regression	OLD	NEW	DELTA	RATIO
NSStringConversion.o	4639	9243	+99.2%	0.50x

Performance: -Osize

Improvement	OLD	NEW	DELTA	RATIO
AngryPhonebook.ASCII	528	17	-96.8%	31.06x
AngryPhonebook	2730	371	-86.4%	7.36x
AngryPhonebook.ASCII.Small	121	17	-85.9%	7.12x
FlattenListLoop	4432	4065	-8.3%	1.09x (?)
RemoveWhereMoveInts	37	34	-8.1%	1.09x (?)
Array2D	7520	6912	-8.1%	1.09x
RemoveWhereSwapInts	67	62	-7.5%	1.08x (?)
MapReduce	433	404	-6.7%	1.07x

Added	MIN	MAX	MEAN	MAX_RSS
NSStringConversion.Medium	400	400	400	—
NSStringConversion.Rebridge	276	288	281	—
NSStringConversion.Rebridge.Long	208	216	212	—
NSStringConversion.Rebridge.LongUTF8	63	64	63	—
NSStringConversion.Rebridge.Medium	208	210	209	—
NSStringConversion.Rebridge.Mutable	916	919	918	—
NSStringConversion.Rebridge.UTF8	527	537	531	—

Code size: -Osize

Regression	OLD	NEW	DELTA	RATIO
NSStringConversion.o	4324	8043	+86.0%	0.54x

Performance: -Onone

Improvement	OLD	NEW	DELTA	RATIO
AngryPhonebook.ASCII	528	17	-96.8%	31.06x
AngryPhonebook	5663	3199	-43.5%	1.77x
AngryPhonebook.ASCII.Small	246	139	-43.5%	1.77x
Histogram	8882	8028	-9.6%	1.11x (?)
ArrayOfPOD	1146	1065	-7.1%	1.08x (?)

Added	MIN	MAX	MEAN	MAX_RSS
NSStringConversion.Medium	3399	3461	3420	—
NSStringConversion.Rebridge	268	272	269	—
NSStringConversion.Rebridge.Long	216	217	216	—
NSStringConversion.Rebridge.LongUTF8	61	63	62	—
NSStringConversion.Rebridge.Medium	215	222	217	—
NSStringConversion.Rebridge.Mutable	900	900	900	—
NSStringConversion.Rebridge.UTF8	512	515	514	—

Code size: -swiftlibs

✅	Benchmark Check Report

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 · 2019-07-09T02:44:29Z

@swift-ci please test

swift-ci · 2019-07-09T02:50:34Z

Build failed
Swift Test OS X Platform
Git Sha - 1c9bfe22d58a0e1441baeab4eb0e56cb8315f91c

palimondo · 2019-07-09T03:25:34Z

You should split the new benchmarks and the code improvements (the optimizations) into separate PRs so that you can measure the latter. Otherwise you are seeing only the “After” state in the Added.

It also looks like we have a small workload sizing issues after the excellent improvements (🤓😱👏) on AngryPhonebook.ASCII and other long ones. Dipping under 20 μs can lead to fragile measurement, because any single digit change is over 5% change threshold. Since these are fresh additions I’ll double the workload size in a separate PR.

xwu · 2019-07-09T03:22:35Z

stdlib/public/core/String.swift

+  ///           specified number of UTF-8 code units.
+  @inline(__always)
+  internal init(
+    uninitializedCapacity capacity: Int,


I think this requires an underscored prefix as per stdlib naming convention, does it not?

It's intended to be made public; there's a S-E pitch thread.

xwu · 2019-07-09T03:23:46Z

stdlib/public/core/String.swift

+  @inline(__always)
+  internal init(
+    uninitializedCapacity capacity: Int,
+    initializingUTF8With initializer: (


Nit

Suggested change

initializingUTF8With initializer: (

initializingUTF8With initializer: (

Catfish-Man · 2019-07-09T06:02:37Z

The new benchmarks are for when we adopt the new initializer in Foundation later; this PR won't change them at all.

…ninitialized storage (swiftlang/swift-evolution#1022) and use it to speed up uppercased() and lowercased()

Catfish-Man · 2019-07-09T22:05:22Z

@swift-ci please test

swift-ci · 2019-07-09T22:07:10Z

Build failed
Swift Test OS X Platform
Git Sha - 1c9bfe22d58a0e1441baeab4eb0e56cb8315f91c

swift-ci · 2019-07-09T22:07:37Z

Build failed
Swift Test Linux Platform
Git Sha - 1c9bfe22d58a0e1441baeab4eb0e56cb8315f91c

milseman

LGTM, trivial suggestions.

milseman · 2019-07-10T00:12:26Z

stdlib/public/core/String.swift

+      if _fastPath(smol.isASCII) {
+        self = String(_StringGuts(smol))
+      } else {
+        //We succeeded in making a _SmallString, but may need to repair UTF8


Space after //

milseman · 2019-07-10T00:14:26Z

stdlib/public/core/StringCreate.swift

+      )
+      return result.asString
+    case .error(let initialRange):
+      //This could be optimized to use excess tail capacity


Space after //

milseman · 2019-07-10T00:16:32Z

stdlib/public/core/StringStorage.swift

+    let buffer = UnsafeMutableBufferPointer(start: storage.mutableStart,
+                                            count: capacity)
+    let count = try initializer(buffer)
+


Shouldn't this just end in a call to _updateCountAndFlags or something like that?

_updateCountAndFlags checks invariants, and this particular initializer doesn't guarantee that all logically invariant things (for example "has ascii contents and claims to be ascii") hold, which is why I didn't use it in this one place.

One day I'll refactor all this logic to flow more reasonably, make the stored properties private, etc., and then have a dedicated bit for "this has passed through some sanctioned create call".

Catfish-Man requested a review from milseman July 8, 2019 22:04

Catfish-Man self-assigned this Jul 8, 2019

Catfish-Man force-pushed the uninitialized-initialize-uppercase branch from 393a1df to 68eac61 Compare July 8, 2019 22:23

milseman requested changes Jul 8, 2019

View reviewed changes

milseman mentioned this pull request Jul 8, 2019

[benchmark] SR-10855: Added non-ASCII characters to String benchmarking #25309

Merged

Catfish-Man force-pushed the uninitialized-initialize-uppercase branch 4 times, most recently from 1e70d94 to 1c9bfe2 Compare July 8, 2019 23:56

xwu reviewed Jul 9, 2019

View reviewed changes

Add a private implementation of a String initializer with access to u…

b06137b

…ninitialized storage (swiftlang/swift-evolution#1022) and use it to speed up uppercased() and lowercased()

Catfish-Man force-pushed the uninitialized-initialize-uppercase branch from 1c9bfe2 to b06137b Compare July 9, 2019 22:05

milseman approved these changes Jul 10, 2019

View reviewed changes

Catfish-Man merged commit 68687e7 into swiftlang:master Jul 10, 2019

BasThomas mentioned this pull request Jul 12, 2019

[139] Issue #139 - July 25, 2019 SwiftWeekly/swiftweekly.github.io#468

Closed

Catfish-Man mentioned this pull request Jul 14, 2019

[SR-11069] Slow case conversion of a longer ASCII String #53461

Closed

	initializingUTF8With initializer: (
	initializingUTF8With initializer: (

Add a private implementation of a String initializer with access to uninitialized storage (https://github.com/apple/swift-evolution/pull/1022) and use it to speed up uppercased() and lowercased() #26007

Add a private implementation of a String initializer with access to uninitialized storage (https://github.com/apple/swift-evolution/pull/1022) and use it to speed up uppercased() and lowercased() #26007

Conversation

Catfish-Man commented Jul 8, 2019

Uh oh!

Catfish-Man commented Jul 8, 2019

Uh oh!

Catfish-Man commented Jul 8, 2019

Uh oh!

swift-ci commented Jul 8, 2019

Uh oh!

Catfish-Man commented Jul 8, 2019

Uh oh!

Catfish-Man commented Jul 8, 2019

Uh oh!

swift-ci commented Jul 8, 2019

Uh oh!

swift-ci commented Jul 8, 2019

Uh oh!

swift-ci commented Jul 8, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Catfish-Man commented Jul 8, 2019

Uh oh!

Catfish-Man commented Jul 8, 2019

Uh oh!

swift-ci commented Jul 8, 2019

Uh oh!

swift-ci commented Jul 9, 2019

Uh oh!

swift-ci commented Jul 9, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

Catfish-Man commented Jul 9, 2019

Uh oh!

swift-ci commented Jul 9, 2019

Uh oh!

palimondo commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Jul 9, 2019

Uh oh!

Catfish-Man commented Jul 9, 2019

Uh oh!

swift-ci commented Jul 9, 2019

Uh oh!

swift-ci commented Jul 9, 2019

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!