Clean up and restructure String bridging a bit #20623

Catfish-Man · 2018-11-16T04:28:51Z

Initial local benchmarking suggests this isn't a speedup, but I'm curious what the full test suite will say.

Catfish-Man · 2018-11-16T04:29:07Z

@swift-ci please smoke benchmark

Catfish-Man · 2018-11-16T04:30:04Z

stdlib/public/core/StringUTF16View.swift

@@ -480,7 +483,7 @@ extension String.UTF16View {
  internal func _nativeGetIndex(for offset: Int) -> Index {
    // Trivial and common: start
    if offset == 0 { return startIndex }
-
+    


I had an ASCII fast path here too but I got it wrong and it was crashing. Need to debug further.

Catfish-Man · 2018-11-16T04:34:17Z

stdlib/public/core/StringStorage.swift

+          return 0
+        }
+        defer { _fixLifetime(nativeOther) }
+        let otherStart = nativeOther.start


Moving start to the protocol certainly cleaned this up :)

swift-ci · 2018-11-16T05:12:33Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
ObjectiveCBridgeStringIsEqualAllSwift	62	96	+54.8%	0.65x
NSStringConversion	590	894	+51.5%	0.66x
DictionaryBridgeToObjC_Access	1005	1521	+51.3%	0.66x
ObjectiveCBridgeStringIsEqual2	197	293	+48.7%	0.67x
ObjectiveCBridgeStubFromNSString	843	1153	+36.8%	0.73x
ObjectiveCBridgeFromNSStringForced	2353	3162	+34.4%	0.74x
ObjectiveCBridgeStubFromArrayOfNSString2	3595	4592	+27.7%	0.78x
ObjectiveCBridgeFromNSString	1147	1311	+14.3%	0.87x
Improvement
StringEqualPointerComparison	657	600	-8.7%	1.09x
InsertCharacterEndIndex	162	148	-8.6%	1.09x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
ObjectiveCBridgeStringIsEqualAllSwift	61	97	+59.0%	0.63x
NSStringConversion	588	903	+53.6%	0.65x
ObjectiveCBridgeStringIsEqual2	195	295	+51.3%	0.66x
ObjectiveCBridgeFromNSString	1028	1395	+35.7%	0.74x
Improvement
CStringLongAscii	505	423	-16.2%	1.19x
StringEqualPointerComparison	628	571	-9.1%	1.10x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
NSStringConversion	647	974	+50.5%	0.66x
ObjectiveCBridgeStringIsEqualAllSwift	63	78	+23.8%	0.81x
ObjectiveCBridgeFromNSStringForced	2442	2823	+15.6%	0.87x
ObjectiveCBridgeStringIsEqual2	196	226	+15.3%	0.87x
ArrayOfPOD	777	859	+10.6%	0.90x (?)

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 · 2018-11-16T18:30:52Z

@swift-ci please smoke benchmark

Catfish-Man · 2018-11-16T18:32:24Z

@swift-ci please smoke benchmark

swift-ci · 2018-11-16T19:28:56Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
StringEqualPointerComparison	600	657	+9.5%	0.91x
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	63	55	-12.7%	1.15x
StringBuilderSmallReservingCapacity	427	375	-12.2%	1.14x (?)
StringBuilder	400	365	-8.7%	1.10x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
NSStringConversion	594	658	+10.8%	0.90x
StringEqualPointerComparison	571	628	+10.0%	0.91x
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	61	54	-11.5%	1.13x
StrComplexWalk	6886	6429	-6.6%	1.07x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
NSStringConversion	660	734	+11.2%	0.90x
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	62	55	-11.3%	1.13x
ArrayOfPOD	853	777	-8.9%	1.10x (?)

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

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

milseman · 2018-11-16T19:45:54Z

@swift-ci please benchmark

swift-ci · 2018-11-16T20:43:11Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
NSStringConversion	528	589	+11.6%	0.90x
Improvement
UTF8Decode_InitFromBytes_ascii	547	459	-16.1%	1.19x
ObjectiveCBridgeStringIsEqualAllSwift	55	48	-12.7%	1.15x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
NSStringConversion	520	584	+12.3%	0.89x
FloatingPointPrinting_Float_description_small	4811	5230	+8.7%	0.92x
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	54	48	-11.1%	1.12x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
DictionaryKeysContainsNative	53	72	+35.8%	0.74x
CharIteration_japanese_unicodeScalars	128487	162628	+26.6%	0.79x
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	55	48	-12.7%	1.15x

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: 8-Core Intel Xeon E5
  Processor Speed: 3 GHz
  Number of Processors: 1
  Total Number of Cores: 8
  L2 Cache (per Core): 256 KB
  L3 Cache: 25 MB
  Memory: 64 GB

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

Catfish-Man · 2018-11-16T22:20:40Z

@swift-ci please benchmark

Catfish-Man · 2018-11-16T22:23:06Z

@swift-ci please benchmark

swift-ci · 2018-11-16T23:08:05Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
NSStringConversion	588	813	+38.3%	0.72x
ObjectiveCBridgeFromNSString	1031	1368	+32.7%	0.75x
ObjectiveCBridgeStubFromNSString	827	1065	+28.8%	0.78x
ObjectiveCBridgeFromNSStringForced	2312	2711	+17.3%	0.85x
ObjectiveCBridgeStringIsEqual2	200	225	+12.5%	0.89x
Improvement
StringEqualPointerComparison	657	600	-8.7%	1.09x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
ObjectiveCBridgeFromNSString	1000	1483	+48.3%	0.67x
NSStringConversion	580	814	+40.3%	0.71x
ObjectiveCBridgeStubFromNSString	856	1087	+27.0%	0.79x
ObjectiveCBridgeStringIsEqual2	195	221	+13.3%	0.88x
ObjectiveCBridgeStringIsEqualAllSwift	61	67	+9.8%	0.91x
Improvement
StringEqualPointerComparison	628	571	-9.1%	1.10x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
NSStringConversion	640	901	+40.8%	0.71x
ObjectiveCBridgeStubFromNSString	877	1116	+27.3%	0.79x
ArrayOfPOD	782	860	+10.0%	0.91x (?)
ObjectiveCBridgeStringIsEqual2	204	223	+9.3%	0.91x

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 · 2018-11-16T23:14:29Z

@swift-ci please benchmark

milseman

LGTM

milseman · 2018-11-16T19:48:58Z

stdlib/public/core/StringStorage.swift

+        return  _cocoaGetCStringTrampoline(self,
+                                           outputPtr,
+                                           maxLength,
+                                           encoding)


Style would be to line-break after opening delimiter, and try to fit these all on the next line

stdlib/public/core/StringBridge.swift

milseman · 2018-11-16T22:59:50Z

stdlib/public/core/StringStorage.swift

@@ -190,7 +187,7 @@ final internal class _StringStorage: _AbstractStringStorage {
  internal var _reserved: UInt16

  @inlinable


Huh, unrelated to your PR, but this @inlinable can be dropped.

#20651

Catfish-Man · 2018-11-16T23:33:29Z

@swift-ci please benchmark

swift-ci · 2018-11-17T00:25:49Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
ObjectiveCBridgeFromNSString	1022	1395	+36.5%	0.73x
Improvement
StringEqualPointerComparison	657	600	-8.7%	1.09x
ObjectiveCBridgeStringIsEqualAllSwift	62	57	-8.1%	1.09x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
ObjectiveCBridgeFromNSString	1153	1289	+11.8%	0.89x
PrefixWhileAnySeqCntRangeLazy	159	176	+10.7%	0.90x
Improvement
StringEqualPointerComparison	628	571	-9.1%	1.10x
ObjectiveCBridgeStringIsEqualAllSwift	61	57	-6.6%	1.07x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
ArrayOfPOD	778	857	+10.2%	0.91x (?)

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 · 2018-11-17T01:34:50Z

@swift-ci please smoke benchmark

Catfish-Man · 2018-11-17T01:36:05Z

@swift-ci please smoke benchmark

swift-ci · 2018-11-17T02:21:53Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
InsertCharacterEndIndex	146	160	+9.6%	0.91x
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	62	47	-24.2%	1.32x
ObjectiveCBridgeStringIsEqual2	198	174	-12.1%	1.14x
Hanoi	3764	3484	-7.4%	1.08x
NSStringConversion	612	570	-6.9%	1.07x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	61	46	-24.6%	1.33x
ObjectiveCBridgeStringIsEqual2	197	174	-11.7%	1.13x
NSStringConversion	609	564	-7.4%	1.08x (?)

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	64	49	-23.4%	1.31x
Histogram	5992	5149	-14.1%	1.16x
ObjectiveCBridgeStringIsEqual2	206	182	-11.7%	1.13x

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 · 2018-11-17T02:38:09Z

@swift-ci please test and merge

Catfish-Man · 2018-11-17T02:40:58Z

@swift-ci please test and merge

Catfish-Man · 2018-11-17T02:45:07Z

@swift-ci please test and merge

Catfish-Man · 2018-11-17T03:10:05Z

@swift-ci please test and merge

Catfish-Man · 2018-11-28T22:18:17Z

@swift-ci please smoke test

Catfish-Man · 2018-11-28T22:19:02Z

@swift-ci please smoke benchmark

Catfish-Man · 2018-11-28T22:20:00Z

@swift-ci please smoke test

Catfish-Man · 2018-11-28T22:20:08Z

@swift-ci please smoke benchmark

swift-ci · 2018-11-28T23:05:27Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	62	47	-24.2%	1.32x
ObjectiveCBridgeStringIsEqual2	187	164	-12.3%	1.14x
NSStringConversion	600	545	-9.2%	1.10x (?)
InsertCharacterEndIndexNonASCII	61	56	-8.2%	1.09x (?)

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	67	47	-29.9%	1.43x
ObjectiveCBridgeStringIsEqual2	189	164	-13.2%	1.15x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
ExclusivityIndependent	75	84	+12.0%	0.89x
Improvement
ObjectiveCBridgeStringIsEqualAllSwift	62	47	-24.2%	1.32x
DropFirstSequenceLazy	12021	10320	-14.2%	1.16x
DropFirstAnySequenceLazy	13119	11434	-12.8%	1.15x
ObjectiveCBridgeStringIsEqual2	188	167	-11.2%	1.13x
DropFirstSequence	11710	10428	-10.9%	1.12x
DropWhileAnySequenceLazy	14062	12843	-8.7%	1.09x
DropFirstAnySequence	12484	11435	-8.4%	1.09x
DropWhileSequence	15253	13990	-8.3%	1.09x
NSStringConversion	654	600	-8.3%	1.09x
PrefixAnySequenceLazy	9383	8617	-8.2%	1.09x
PrefixSequenceLazy	9033	8298	-8.1%	1.09x
PrefixSequence	9090	8393	-7.7%	1.08x
DropWhileSequenceLazy	13627	12619	-7.4%	1.08x
PrefixAnySequence	9801	9085	-7.3%	1.08x

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

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

• Convert _AbstractStringStorage to a protocol, and the free functions used to deduplicate implementations to extensions on that protocol. • Move 'start' into the abstract type and use that to simplify some code • Move the ASCII fast path for length into UTF16View. • Add a weirder but faster way to check which (if any) of our NSString subclasses a given object is, and adopt it

Catfish-Man · 2018-11-29T00:05:23Z

@swift-ci please smoke test and merge

Catfish-Man · 2018-11-29T00:06:32Z

the ABI diffs are due to https://bugs.swift.org/browse/SR-9372

Catfish-Man · 2018-11-29T00:08:12Z

@swift-ci please smoke test and merge

Catfish-Man · 2018-11-29T19:55:48Z

@airspeedswift we may want this for Swift 5, not so much because of the perf win (though that's nice), but because it'll make cherry-picking other bridging changes hard if we don't

Catfish-Man self-assigned this Nov 16, 2018

Catfish-Man commented Nov 16, 2018

View reviewed changes

Catfish-Man force-pushed the bridgecleanup branch 3 times, most recently from ae46f1b to bc167d4 Compare November 16, 2018 18:26

Catfish-Man force-pushed the bridgecleanup branch from bc167d4 to 1d6c786 Compare November 16, 2018 19:30

Catfish-Man changed the title ~~[DNM] Try out some ideas from Michael Ilseman for cleaning up/speeding up bridging further~~ Clean up and restructure String bridging a bit Nov 16, 2018

Catfish-Man force-pushed the bridgecleanup branch from 1d6c786 to 0cc4787 Compare November 16, 2018 19:35

Catfish-Man requested a review from milseman November 16, 2018 19:35

milseman approved these changes Nov 16, 2018

View reviewed changes

Catfish-Man force-pushed the bridgecleanup branch from 762c49c to 492cab9 Compare November 17, 2018 02:33

Catfish-Man force-pushed the bridgecleanup branch from dabea0c to 37c4967 Compare November 28, 2018 23:11

Catfish-Man force-pushed the bridgecleanup branch from 37c4967 to 8b57921 Compare November 29, 2018 00:04

swift-ci merged commit e012655 into swiftlang:master Nov 29, 2018

Catfish-Man deleted the bridgecleanup branch December 1, 2018 00:51

xwu mentioned this pull request Dec 2, 2018

[gardening] Make edits for stdlib style [NFC] #20952

Merged

		@@ -190,7 +187,7 @@ final internal class _StringStorage: _AbstractStringStorage {
		internal var _reserved: UInt16

		@inlinable

Clean up and restructure String bridging a bit #20623

Clean up and restructure String bridging a bit #20623

Uh oh!

Conversation

Catfish-Man commented Nov 16, 2018

Uh oh!

Catfish-Man commented Nov 16, 2018

Uh oh!

Catfish-Man Nov 16, 2018

Choose a reason for hiding this comment

Uh oh!

Catfish-Man Nov 16, 2018

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Nov 16, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

Catfish-Man commented Nov 16, 2018

Uh oh!

Catfish-Man commented Nov 16, 2018

Uh oh!

swift-ci commented Nov 16, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

milseman commented Nov 16, 2018

Uh oh!

swift-ci commented Nov 16, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

Catfish-Man commented Nov 16, 2018

Uh oh!

Catfish-Man commented Nov 16, 2018

Uh oh!

swift-ci commented Nov 16, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

Catfish-Man commented Nov 16, 2018

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

milseman Nov 16, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

milseman Nov 16, 2018

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Nov 16, 2018

Uh oh!

swift-ci commented Nov 17, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

Catfish-Man commented Nov 17, 2018

Uh oh!

Catfish-Man commented Nov 17, 2018

Uh oh!

swift-ci commented Nov 17, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

Catfish-Man commented Nov 17, 2018

Uh oh!

Catfish-Man commented Nov 29, 2018 •

edited

Loading