SR-10555 foreignCopyUTF8 should do bulk access #24289

Catfish-Man · 2019-04-25T21:05:17Z

Fixes https://bugs.swift.org/browse/SR-10555 and rdar://50220164

Catfish-Man · 2019-04-25T21:11:05Z

@swift-ci please smoke benchmark

milseman

Want to make sure we trigger the precondition failure. Might be worth adding a test case to StringTraps.swift for this.

stdlib/public/core/StringBridge.swift

Catfish-Man · 2019-04-25T22:18:39Z

@swift-ci please smoke benchmark

Catfish-Man · 2019-04-25T22:18:45Z

@swift-ci please smoke test

Catfish-Man · 2019-04-25T22:53:03Z

Oops, forgot to make that conditional for ObjC

milseman

LGTM, let's see if it's covered by a benchmark

milseman · 2019-04-26T00:33:55Z

https://ci.swift.org/view/Pull%20Request/job/swift-PR-osx-smoke-perf/902/

swift-ci · 2019-04-26T00:38:43Z

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
EqualSubstringSubstring	34	37	+8.8%	0.92x (?)
EqualStringSubstring	34	37	+8.8%	0.92x (?)
EqualSubstringSubstringGenericEquatable	34	37	+8.8%	0.92x (?)
LessSubstringSubstring	35	38	+8.6%	0.92x (?)

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
DataReplaceLarge	18200	19800	+8.8%	0.92x (?)
LessSubstringSubstring	35	38	+8.6%	0.92x (?)
EqualSubstringSubstringGenericEquatable	35	38	+8.6%	0.92x (?)
EqualSubstringString	35	38	+8.6%	0.92x (?)
LessSubstringSubstringGenericComparable	35	38	+8.6%	0.92x

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

Catfish-Man · 2019-04-26T00:39:13Z

Alas, will have to write some new benchmarks

Catfish-Man · 2019-04-27T02:13:37Z

@swift-ci please smoke benchmark

swift-ci · 2019-04-27T05:06:03Z

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
LessSubstringSubstring	39	43	+10.3%	0.91x (?)
DataReplaceLarge	20400	22400	+9.8%	0.91x (?)
ObjectiveCBridgeStubToNSDate2	1370	1480	+8.0%	0.93x (?)
EqualSubstringSubstring	38	41	+7.9%	0.93x (?)
EqualStringSubstring	38	41	+7.9%	0.93x (?)
EqualSubstringSubstringGenericEquatable	38	41	+7.9%	0.93x (?)
LessSubstringSubstringGenericComparable	39	42	+7.7%	0.93x (?)
Improvement
ObjectiveCBridgeASCIIStringFromFile	171	66	-61.4%	2.59x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
LessSubstringSubstringGenericComparable	39	43	+10.3%	0.91x
EqualSubstringSubstring	38	41	+7.9%	0.93x (?)
EqualSubstringSubstringGenericEquatable	39	42	+7.7%	0.93x (?)
EqualSubstringString	39	42	+7.7%	0.93x (?)
Improvement
ObjectiveCBridgeASCIIStringFromFile	171	66	-61.4%	2.59x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
EqualSubstringSubstringGenericEquatable	44	49	+11.4%	0.90x (?)
Improvement
ObjectiveCBridgeASCIIStringFromFile	171	66	-61.4%	2.59x

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-04-27T05:09:16Z

2.6x win, that’s more like it!

milseman · 2019-04-27T19:44:27Z

This seems related:

[ RUN      ] StringUTF8Tests.Contiguous Access
stdout>>> abcd
stdout>>> abcdefghijklmnop
stdout>>> abcdéfghijk
stderr>>> Fatal error: invalid Collection: less than 'count' elements in collection: file /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/stdlib/public/core/ContiguousArrayBuffer.swift, line 643
stderr>>> Current stack trace:
stderr>>> 0    libswiftCore.dylib                 0x0000000109dfb020 _swift_stdlib_reportFatalErrorInFile + 116
stderr>>> 1    libswiftCore.dylib                 0x0000000109d14b80 specialized closure #1 in closure #1 in closure #1 in _fatalErrorMessage(_:_:file:line:flags:) + 304
stderr>>> 2    libswiftCore.dylib                 0x0000000109d14dc0 specialized closure #1 in closure #1 in _fatalErrorMessage(_:_:file:line:flags:) + 110
stderr>>> 3    libswiftCore.dylib                 0x0000000109cd2ff0 specialized closure #1 in _fatalErrorMessage(_:_:file:line:flags:) + 124
stderr>>> 4    libswiftCore.dylib                 0x0000000109cd3420 specialized _fatalErrorMessage(_:_:file:line:flags:) + 643
stderr>>> 5    libswiftCore.dylib                 0x0000000109cda0a0 specialized _copyCollectionToContiguousArray<A>(_:) + 900
stderr>>> 6    libswiftCore.dylib                 0x0000000109bbdf50 _StringGuts._foreignGrow(_:) + 56
stderr>>> 7    libswiftCore.dylib                 0x0000000109bbe170 _StringGuts.prepareForAppendInPlace(otherUTF8Count:) + 1323
stderr>>> 8    libswiftCore.dylib                 0x0000000109bbebf0 _StringGuts.append(_:) + 324
stderr>>> 9    libswiftCore.dylib                 0x0000000109cdb050 specialized _StringGuts.append(_:) + 230
stderr>>> 10   libswiftCore.dylib                 0x0000000109a5d8c0 static String.+= infix(_:_:) + 21
stderr>>> 11   String                             0x00000001090eb250 String.makeNative() + 49
stderr>>> 12   String                             0x00000001090eb3e0 closure #1 in  + 3736
stderr>>> 13   libswiftStdlibUnittest.dylib       0x00000001096fa890 specialized TestSuite._runTest(name:parameter:) + 584
stderr>>> 14   libswiftStdlibUnittest.dylib       0x00000001096b8ef0 _childProcess() + 846
stderr>>> 15   String                             0x00000001090ead30 main + 667
stderr>>> 16   libdyld.dylib                      0x00007fff62cc9ed8 start + 1
stderr>>> CRASHED: SIGILL
the test crashed unexpectedly
[     FAIL ] StringUTF8Tests.Contiguous Access
StringUTF8Tests: Some tests failed, aborting
UXPASS: []
FAIL: ["Contiguous Access"]
SKIP: []
To debug, run:
$ /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/buildbot_incremental/swift-macosx-x86_64/validation-test-macosx-x86_64/stdlib/Output/StringUTF8.swift.tmp/String --stdlib-unittest-in-process --stdlib-unittest-filter "Contiguous Access"

Catfish-Man · 2019-04-28T17:39:15Z

Yeah, definitely something wrong around the edges of this. Will figure out what and land it on Monday :)

My guess is that we’re ending up in a situation where CFStringGetBytes reports a different number of “characters” converted than what we calculated with .utf8.count

Catfish-Man · 2019-04-29T22:56:03Z

@swift-ci please test

swift-ci · 2019-04-29T23:10:14Z

Build failed
Swift Test Linux Platform
Git Sha - cf78d2499a2c65f6c08947b50057e48f6b4ec7eb

Catfish-Man · 2019-04-29T23:24:19Z

@swift-ci please test

swift-ci · 2019-04-29T23:28:27Z

Build failed
Swift Test OS X Platform
Git Sha - cf78d2499a2c65f6c08947b50057e48f6b4ec7eb

swift-ci · 2019-04-29T23:53:41Z

Build failed
Swift Test Linux Platform
Git Sha - cf78d2499a2c65f6c08947b50057e48f6b4ec7eb

Catfish-Man · 2019-04-30T03:08:45Z

Linux failure is a known unrelated hang

Catfish-Man · 2019-04-30T03:43:49Z

@swift-ci please smoke test linux platform

Catfish-Man · 2019-04-30T03:44:57Z

@swift-ci please test linux platform

Catfish-Man · 2019-04-30T03:47:47Z

oops. Guess those both worked >.<

milseman · 2019-04-30T21:27:23Z

What was the bug and why did this fix it? Also, can you test and make sure it gives the same result if the foreign string has unpaired surrogates?

Catfish-Man self-assigned this Apr 25, 2019

Catfish-Man force-pushed the bulk-discount branch 2 times, most recently from b67972b to c90534e Compare April 25, 2019 21:09

Catfish-Man requested a review from milseman April 25, 2019 21:18

milseman reviewed Apr 25, 2019

View reviewed changes

stdlib/public/core/StringBridge.swift Show resolved Hide resolved

Catfish-Man force-pushed the bulk-discount branch from c90534e to 11eae01 Compare April 25, 2019 22:18

milseman approved these changes Apr 26, 2019

View reviewed changes

Catfish-Man force-pushed the bulk-discount branch from 11eae01 to cf78d24 Compare April 29, 2019 22:55

SR-10555 foreignCopyUTF8 should do bulk access

fd0d4d8

Catfish-Man force-pushed the bulk-discount branch from cf78d24 to fd0d4d8 Compare April 29, 2019 23:24

Catfish-Man merged commit b6d0362 into swiftlang:master Apr 30, 2019

Catfish-Man mentioned this pull request May 9, 2019

[5.1] SR-10555 foreignCopyUTF8 should do bulk access #24648

Merged

palimondo mentioned this pull request Jul 19, 2019

Add a benchmark for bridging String(contentsOfFile:...) #24326

Merged

Catfish-Man mentioned this pull request May 9, 2019

[SR-10555] foreignCopyUTF8 should do bulk access #52955

Closed

SR-10555 foreignCopyUTF8 should do bulk access #24289

SR-10555 foreignCopyUTF8 should do bulk access #24289

Uh oh!

Conversation

Catfish-Man commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Catfish-Man commented Apr 25, 2019

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Catfish-Man commented Apr 25, 2019

Uh oh!

Catfish-Man commented Apr 25, 2019

Uh oh!

Catfish-Man commented Apr 25, 2019

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

milseman commented Apr 26, 2019

Uh oh!

swift-ci commented Apr 26, 2019

Performance: -O

Performance: -Osize

Uh oh!

Catfish-Man commented Apr 26, 2019

Uh oh!

Catfish-Man commented Apr 27, 2019

Uh oh!

swift-ci commented Apr 27, 2019

Performance: -O

Performance: -Osize

Performance: -Onone

Uh oh!

Catfish-Man commented Apr 27, 2019

Uh oh!

milseman commented Apr 27, 2019

Uh oh!

Catfish-Man commented Apr 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Catfish-Man commented Apr 29, 2019

Uh oh!

swift-ci commented Apr 29, 2019

Uh oh!

Catfish-Man commented Apr 29, 2019

Uh oh!

swift-ci commented Apr 29, 2019

Uh oh!

swift-ci commented Apr 29, 2019

Uh oh!

Catfish-Man commented Apr 30, 2019

Uh oh!

Catfish-Man commented Apr 30, 2019

Uh oh!

Catfish-Man commented Apr 30, 2019

Uh oh!

Catfish-Man commented Apr 30, 2019

Uh oh!

milseman commented Apr 30, 2019

Uh oh!

Uh oh!

Catfish-Man commented Apr 25, 2019 •

edited

Loading

Catfish-Man commented Apr 28, 2019 •

edited

Loading