Bridge non-ASCII SmallStrings as native Swift Strings rather than by creating CFStrings #25866

Catfish-Man · 2019-06-28T21:20:53Z

No description provided.

jrose-apple · 2019-06-28T22:29:54Z

This is like three levels of wrong. 😭

(Not that it isn't necessarily necessary, but…sadness.)

jrose-apple · 2019-07-02T20:34:55Z

Yay, down to only 1 level of wrong!

test/stdlib/TestNSString.swift

Catfish-Man · 2019-07-02T22:37:25Z

test/stdlib/TestNSString.swift

These will continue to fail until we fix the non-smol issues, so are commented out for now

You can also split out this portion and XFAIL it instead, with the JIRA linked.

.xfail(.always("SR-nnnn: ..."))

Catfish-Man · 2019-07-02T22:39:24Z

@swift-ci please test

Catfish-Man · 2019-07-02T22:39:29Z

@swift-ci please benchmark

swift-ci · 2019-07-02T22:41:43Z

Build failed
Swift Test Linux Platform
Git Sha - 7310fab2fcce536729c3a7c8eababf87ab5a5d06

swift-ci · 2019-07-02T22:43:07Z

Build failed
Swift Test OS X Platform
Git Sha - 7310fab2fcce536729c3a7c8eababf87ab5a5d06

Catfish-Man · 2019-07-02T22:51:07Z

@swift-ci please benchmark

milseman

LGTM, minor improvements mentioned

stdlib/public/core/StringBridge.swift

milseman · 2019-07-02T23:12:15Z

test/stdlib/TestNSString.swift

You can also split out this portion and XFAIL it instead, with the JIRA linked.

.xfail(.always("SR-nnnn: ..."))

stdlib/public/core/StringBridge.swift

Catfish-Man · 2019-07-02T23:31:33Z

@swift-ci nominate

…creating CFStrings. This gets consistent behavior with non-smol Strings when the String contains a BOM

Catfish-Man · 2019-07-02T23:37:51Z

@swift-ci please test and merge

swift-ci · 2019-07-02T23:46:12Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
DataAppendDataLargeToLarge	59200	87800	+48.3%	0.67x (?)

Improvement	OLD	NEW	DELTA	RATIO
RandomShuffleLCG2	768	704	-8.3%	1.09x
FlattenListLoop	4331	3971	-8.3%	1.09x (?)
RemoveWhereMoveInts	37	34	-8.1%	1.09x
Array2D	7520	6928	-7.9%	1.09x
RemoveWhereSwapInts	65	60	-7.7%	1.08x (?)
MapReduce	397	368	-7.3%	1.08x
MapReduceAnyCollection	398	369	-7.3%	1.08x (?)

Code size: -O

Performance: -Osize

Regression	OLD	NEW	DELTA	RATIO
StringBuilderLong	1220	1320	+8.2%	0.92x (?)
StringBuilderWithLongSubstring	1420	1530	+7.7%	0.93x (?)

Improvement	OLD	NEW	DELTA	RATIO
RemoveWhereMoveInts	37	34	-8.1%	1.09x
Array2D	7520	6912	-8.1%	1.09x
RemoveWhereSwapInts	67	62	-7.5%	1.08x (?)
FlattenListLoop	5286	4914	-7.0%	1.08x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
DictionarySwapAt	3868	4352	+12.5%	0.89x (?)

Improvement	OLD	NEW	DELTA	RATIO
ArrayAppendOptionals	1550	1370	-11.6%	1.13x (?)
ObjectiveCBridgeStubToNSDate2	1590	1410	-11.3%	1.13x (?)
DictionaryBridgeToObjC_Access	1157	1056	-8.7%	1.10x (?)
StringMatch	57400	53000	-7.7%	1.08x (?)
DataAppendSequence	3872800	3576000	-7.7%	1.08x (?)
Data.init.Sequence.809B.Count.RE	38505	35681	-7.3%	1.08x (?)
Data.append.Sequence.64kB.Count.RE	31220	28937	-7.3%	1.08x (?)
Data.append.Sequence.809B.Count.RE.I	38574	35781	-7.2%	1.08x (?)
Data.init.Sequence.64kB.Count.RE	31059	28857	-7.1%	1.08x (?)
Data.init.Sequence.809B.Count.RE.I	38620	35883	-7.1%	1.08x (?)
Data.append.Sequence.809B.Count.RE	38609	35896	-7.0%	1.08x (?)
Data.append.Sequence.64kB.Count.RE.I	31185	28994	-7.0%	1.08x (?)
Data.append.Sequence.809B.Count0.RE	38581	35874	-7.0%	1.08x (?)
Data.init.Sequence.809B.Count0.RE.I	38446	35762	-7.0%	1.08x (?)
Data.init.Sequence.64kB.Count.RE.I	31093	28930	-7.0%	1.07x (?)
Data.append.Sequence.64kB.Count0.RE.I	30943	28831	-6.8%	1.07x (?)
Data.append.Sequence.809B.Count0.RE.I	38440	35881	-6.7%	1.07x (?)

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 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-02T23:51:33Z

@swift-ci please test and merge

Catfish-Man · 2019-07-02T23:57:28Z

@swift-ci please test and merge

Catfish-Man · 2019-07-03T00:50:39Z

@swift-ci please test and merge

Catfish-Man · 2019-07-03T04:26:20Z

@swift-ci test

swift-ci · 2019-07-03T04:28:18Z

Build failed
Swift Test OS X Platform
Git Sha - e487fafbcc4598e914d5af91d3596699ebb66570

swift-ci · 2019-07-03T04:28:32Z

Build failed
Swift Test Linux Platform
Git Sha - e487fafbcc4598e914d5af91d3596699ebb66570

Catfish-Man self-assigned this Jun 28, 2019

Catfish-Man force-pushed the someone-set-up-us-the-bom branch from 8730c8e to 5f7f795 Compare July 1, 2019 23:32

jrose-apple reviewed Jul 2, 2019

View reviewed changes

test/stdlib/TestNSString.swift Outdated Show resolved Hide resolved

Catfish-Man force-pushed the someone-set-up-us-the-bom branch from c82491d to e487faf Compare July 2, 2019 22:35

Catfish-Man changed the title ~~Unconditionally prepend a BOM when bridging smol strings to work around CF changing the length as we bridge~~ Bridge non-ASCII SmallStrings as native Swift Strings rather than by creating CFStrings Jul 2, 2019

Catfish-Man requested a review from milseman July 2, 2019 22:36

Catfish-Man marked this pull request as ready for review July 2, 2019 22:37

Catfish-Man commented Jul 2, 2019

View reviewed changes

milseman approved these changes Jul 2, 2019

View reviewed changes

Bridge non-ASCII SmallStrings as native Swift Strings rather than by …

27359bd

…creating CFStrings. This gets consistent behavior with non-smol Strings when the String contains a BOM

Catfish-Man force-pushed the someone-set-up-us-the-bom branch from e487faf to 27359bd Compare July 2, 2019 23:37

airspeedswift approved these changes Jul 3, 2019

View reviewed changes

Catfish-Man mentioned this pull request Jul 3, 2019

Bridge non-ASCII SmallStrings as native Swift Strings rather than by creating CFStrings. This gets consistent behavior with non-smol Strings when the String contains a BOM #25957

Merged

Catfish-Man merged commit 0ae4b0a into swiftlang:master Jul 3, 2019

Bridge non-ASCII SmallStrings as native Swift Strings rather than by creating CFStrings #25866

Bridge non-ASCII SmallStrings as native Swift Strings rather than by creating CFStrings #25866

Uh oh!

Conversation

Catfish-Man commented Jun 28, 2019

Uh oh!

jrose-apple commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Jul 2, 2019

Uh oh!

Uh oh!

Catfish-Man Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

milseman Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Jul 2, 2019

Uh oh!

Catfish-Man commented Jul 2, 2019

Uh oh!

swift-ci commented Jul 2, 2019

Uh oh!

swift-ci commented Jul 2, 2019

Uh oh!

Catfish-Man commented Jul 2, 2019

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

milseman Jul 2, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Catfish-Man commented Jul 2, 2019

Uh oh!

Catfish-Man commented Jul 2, 2019

Uh oh!

swift-ci commented Jul 2, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

Catfish-Man commented Jul 2, 2019

Uh oh!

Catfish-Man commented Jul 2, 2019

Uh oh!

Catfish-Man commented Jul 3, 2019

Uh oh!

Catfish-Man commented Jul 3, 2019

Uh oh!

swift-ci commented Jul 3, 2019

Uh oh!

swift-ci commented Jul 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jrose-apple commented Jun 28, 2019 •

edited

Loading