[stdlib] Unexport shadow protocols #20509

lorentey · 2018-11-12T16:49:10Z

We redefine some Cocoa interfaces in a set of shadow protocols to make select Cocoa APIs available to the stdlib. These protocols don't meaningfully increase type safety, but having them part of the ABI forces collection storage classes to expose their implementations as Swift entry points. This is unfortunate; these are internal implementation details that we may want to tweak later. (Also, the Swift names / argument types don't always match how ClangImporter imports these APIs to Swift.)

This was uncovered by @jrose-apple's improved diagnostics in #20485.

lorentey · 2018-11-12T16:49:25Z

@swift-ci please test

lorentey · 2018-11-12T17:03:53Z

@swift-ci benchmark

jrose-apple · 2018-11-12T18:05:23Z

stdlib/public/core/ArrayBuffer.swift

@@ -392,7 +342,7 @@ extension _ArrayBuffer {
      )
    } else {
      // ObjC arrays do their own subscript checking.
-      element = _nonNative.objectAt(i)
+      element = _nonNative[i]


Nitpick: This was probably written this way because -objectAtIndexedSubscript: is just going to call -objectAtIndex:, and you might as well save that one objc_msgSend.

This does not call -objectAtIndexedSubscript:: _nonNative now returns a _CocoaArrayWrapper, which is a Swift struct. Its opaque subscript calls objectAt(_:), which is the shadow protocol's pet name for -objectAtIndex: a.k.a. object(at:)

This does add an extra static function call compared to the original, but those are pretty cheap, and this is on a slow path anyway, so it shouldn't matter much.

Aha, my bad for not taking the whole patch at once. Thanks for the explanation.

swift-ci · 2018-11-12T18:29:24Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
IterateData	1400	1508	+7.7%	0.93x
Improvement
DictionaryGroupOfObjects	2010	1816	-9.7%	1.11x
DictionaryBridgeToObjC_Bridge	21	19	-9.5%	1.11x
ArrayOfRef	4570	4180	-8.5%	1.09x
FlattenListLoop	4332	3978	-8.2%	1.09x
Array2D	7507	6911	-7.9%	1.09x
MapReduceAnyCollection	398	369	-7.3%	1.08x
FlattenListFlatMap	6863	6404	-6.7%	1.07x (?)
MapReduce	426	398	-6.6%	1.07x

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
ClassArrayGetter.o	5105	5655	+10.8%	0.90x
NopDeinit.o	5151	5520	+7.2%	0.93x
ArrayOfGenericRef.o	14244	15036	+5.6%	0.95x
ArrayOfRef.o	11802	12338	+4.5%	0.96x
COWTree.o	13436	13956	+3.9%	0.96x
DriverUtils.o	146247	150391	+2.8%	0.97x
DictionaryGroup.o	16706	17092	+2.3%	0.98x
SortLettersInPlace.o	8708	8847	+1.6%	0.98x
DictionarySubscriptDefault.o	28967	29329	+1.2%	0.99x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
IterateData	1357	1502	+10.7%	0.90x
Improvement
DictionaryBridgeToObjC_Bridge	26	19	-26.9%	1.37x
NopDeinit	56869	44449	-21.8%	1.28x
SetIntersectionInt0	70	61	-12.9%	1.15x
SetIntersect	701	613	-12.6%	1.14x
SetIntersectionInt25	150	138	-8.0%	1.09x

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
NopDeinit.o	5564	6212	+11.6%	0.90x
ClassArrayGetter.o	5169	5689	+10.1%	0.91x
COWTree.o	13594	14498	+6.6%	0.94x
ArrayOfRef.o	12514	13162	+5.2%	0.95x
DictionaryGroup.o	15799	16272	+3.0%	0.97x
ArrayOfGenericRef.o	13404	13636	+1.7%	0.98x
MapReduce.o	26448	26811	+1.4%	0.99x
DriverUtils.o	132439	133975	+1.2%	0.99x
DictionarySubscriptDefault.o	27135	27449	+1.2%	0.99x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Improvement
CharIteration_utf16_unicodeScalars	180810	117842	-34.8%	1.53x
CharIteration_tweet_unicodeScalars	394191	279100	-29.2%	1.41x
CharIteration_korean_unicodeScalars	196338	140485	-28.4%	1.40x
CharIteration_russian_unicodeScalars	162609	119180	-26.7%	1.36x
CharIteration_ascii_unicodeScalars	192330	141690	-26.3%	1.36x
CharIteration_japanese_unicodeScalars	234413	173165	-26.1%	1.35x
CharIteration_chinese_unicodeScalars	147253	109345	-25.7%	1.35x
CharIteration_punctuated_unicodeScalars	43069	31985	-25.7%	1.35x
CharIteration_punctuatedJapanese_unicodeScalars	34095	25483	-25.3%	1.34x
DictionaryBridgeToObjC_Bridge	25	19	-24.0%	1.32x
CharIndexing_utf16_unicodeScalars	390947	316724	-19.0%	1.23x
CharIndexing_tweet_unicodeScalars	912869	754171	-17.4%	1.21x
CharIndexing_japanese_unicodeScalars	556328	460571	-17.2%	1.21x
CharIndexing_korean_unicodeScalars	445529	374531	-15.9%	1.19x
CharIndexing_punctuated_unicodeScalars	99242	84087	-15.3%	1.18x
CharIndexing_russian_unicodeScalars	376301	320160	-14.9%	1.18x
CharIndexing_ascii_unicodeScalars	449899	382828	-14.9%	1.18x
CharIndexing_punctuatedJapanese_unicodeScalars	78425	67100	-14.4%	1.17x
CharIndexing_chinese_unicodeScalars	342104	293311	-14.3%	1.17x
CharIndexing_utf16_unicodeScalars_Backwards	400436	345619	-13.7%	1.16x

Code size: -swiftlibs

TEST	OLD	NEW	DELTA	RATIO
Regression
libswiftStdlibUnittest.dylib	372736	376832	+1.1%	0.99x

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

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

lorentey · 2018-11-12T18:44:06Z

Interesting benchmark results. If anything, I'd have expected performance regressions and code size improvements, not the other way round.

My guess is that moving some of the Array slow paths to opaque members encouraged inlining where it didn't happen before.

swift-ci · 2018-11-12T19:35:38Z

Build failed
Swift Test OS X Platform
Git Sha - b5408de1c41ed439f6ea07564fceeaa9e6da43ee

lorentey · 2018-11-12T20:17:38Z

@swift-ci please test

lorentey · 2018-11-12T20:21:32Z

@swift-ci test macOS platform

lorentey · 2018-11-13T01:08:10Z

@swift-ci please test

jrose-apple · 2018-11-13T02:09:00Z

@swift-ci Please test

swift-ci · 2018-11-13T03:04:00Z

Build failed
Swift Test Linux Platform
Git Sha - 11fd8c76a645588e31a27ac0b0b18dd650dc4834

lorentey · 2018-11-13T12:21:13Z

@swift-ci test

jrose-apple · 2018-11-13T17:30:04Z

@swift-ci Please test macOS

jrose-apple · 2018-11-14T18:10:53Z

Ping? Still have my own PR to go in after this one. :-)

These shadow protocols don’t add any real type safety, and exposing them in the ABI seems unnecessary.

Also, move field declarations to the top of the struct.

It used to be a shadow protocol existential, but now it’s invariably AnyObject. I see no reason to keep supporting this unused abstraction.

These shadow protocols don’t add any real type safety, and exposing them in the ABI seems unnecessary.

… non-ObjC platforms

lorentey · 2018-11-15T13:18:18Z

I rebased to resolve the ABI diff conflicts; this is now ready to merge.

@swift-ci test and merge

lorentey · 2018-11-15T15:07:12Z

Swift(macosx-x86_64).api-digester.stability-stdlib-abi.swift:
-Struct _CocoaArrayWrapper has type witness type for Sequence.SubSequence changing from Slice<_CocoaArrayWrapper> to _SliceBuffer

That's weird; I don't think I touched that.

lorentey · 2018-11-15T15:12:58Z

FYI @nkcsgexi I had to remove a type witness change that (at first glance) seems unrelated to my changes.

lorentey · 2018-11-15T15:13:32Z

@swift-ci please smoke test

nkcsgexi · 2018-11-15T17:24:18Z

Thanks for letting me know. AFAIK, type witness can be inferred from the type. Is there any chance your other changes triggered the inferred witness change?

lorentey · 2018-11-15T20:09:17Z

On second look, it does seem very much related — and the need to remove it is explained nicely by the recent removal of Sequence.SubSequence. Everything checks out; sorry for the false alarm! 😊

lorentey requested review from airspeedswift, lancep, millenomi and Catfish-Man November 12, 2018 16:49

This comment has been minimized.

Sign in to view

jrose-apple reviewed Nov 12, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

lorentey added 4 commits November 15, 2018 09:54

[stdlib] Make _NSDictionaryCore, _NSDictionary internal

0969037

These shadow protocols don’t add any real type safety, and exposing them in the ABI seems unnecessary.

[stdlib] Make _NSSetCore, _NSSet internal

4f8eeae

These shadow protocols don’t add any real type safety, and exposing them in the ABI seems unnecessary.

[stdlib] Make protocol _NSNumber internal

c71abaa

[stdlib] Make protocol _NSArrayCore internal

ee17e97

These shadow protocols don’t add any real type safety, and exposing them in the ABI seems unnecessary.

lorentey added 5 commits November 15, 2018 09:54

[stdlib] _SliceBuffer: Make the order of fields a bit more sensible

0463230

Also, move field declarations to the top of the struct.

[stdlib] _BridgeStorage: Remove second type parameter (ObjCClass)

c4c4c17

It used to be a shadow protocol existential, but now it’s invariably AnyObject. I see no reason to keep supporting this unused abstraction.

[stdlib] Make the rest of the shadow protocols internal

93a3ebe

These shadow protocols don’t add any real type safety, and exposing them in the ABI seems unnecessary.

[stdlib] Re-add dummy versions of _NSSetCore and _NSDictionaryCore on…

d707b0b

… non-ObjC platforms

[test] Fix outdated reference to _NSArrayCore

774ff61

lorentey force-pushed the not-quite-shadowy-enough branch from 4c08094 to 774ff61 Compare November 15, 2018 13:17

[test] Remove spurious ABI change (?)

ca10375

lorentey merged commit 7df4076 into swiftlang:master Nov 15, 2018

lorentey deleted the not-quite-shadowy-enough branch November 15, 2018 20:09

[stdlib] Unexport shadow protocols #20509

[stdlib] Unexport shadow protocols #20509

Uh oh!

Conversation

lorentey commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorentey commented Nov 12, 2018

Uh oh!

lorentey commented Nov 12, 2018

Uh oh!

This comment has been minimized.

jrose-apple Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

lorentey Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

jrose-apple Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Nov 12, 2018

Build comment file:

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

lorentey commented Nov 12, 2018

Uh oh!

swift-ci commented Nov 12, 2018

Uh oh!

lorentey commented Nov 12, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

lorentey commented Nov 12, 2018

Uh oh!

This comment has been minimized.

lorentey commented Nov 13, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

jrose-apple commented Nov 13, 2018

Uh oh!

swift-ci commented Nov 13, 2018

Uh oh!

lorentey commented Nov 13, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

jrose-apple commented Nov 13, 2018

Uh oh!

jrose-apple commented Nov 14, 2018

Uh oh!

lorentey commented Nov 15, 2018

Uh oh!

lorentey commented Nov 15, 2018

Uh oh!

lorentey commented Nov 15, 2018

Uh oh!

lorentey commented Nov 15, 2018

Uh oh!

nkcsgexi commented Nov 15, 2018

Uh oh!

lorentey commented Nov 15, 2018

Uh oh!

Uh oh!

lorentey commented Nov 12, 2018 •

edited

Loading