Adopt bulk Dictionary creation in bridging #21235

Catfish-Man · 2018-12-12T01:48:31Z

Karoy's new Dictionary initializer gives us access to the uninitialized backing buffers, which we can bulk-copy the contents of the NSDictionary into.

Catfish-Man · 2018-12-12T02:48:36Z

I think I understand what's going wrong now :)

Catfish-Man · 2018-12-12T03:49:12Z

Ok maybe I don't. Somehow we're blowing up trying to retain the thing we just copied out of the NSDictionary. Before I thought we just weren't retaining it (which is probably true, but wasn't the problem).

Catfish-Man · 2018-12-12T19:12:15Z

stdlib/public/Darwin/Foundation/NSDictionary.swift

@@ -107,19 +107,84 @@ extension Dictionary : _ObjectiveCBridgeable {
    result = builder.take()
  }

+  private static func _typePun<T>(index:Int, of buffer: UnsafeBufferPointer<AnyObject>, to: T.Type) -> UnsafeMutablePointer<T> {
+    let typedBase = UnsafeMutableRawPointer(mutating: buffer.baseAddress!).assumingMemoryBound(to: T.self)


This part is a bit ridiculous, but I'm trying really hard to a) avoid binding the memory at base to T prematurely, and b) avoid doing pointer math myself when Swift can already learn the size and alignment from T itself

Catfish-Man · 2018-12-12T19:13:12Z

stdlib/public/Darwin/Foundation/NSDictionary.swift

+    _ objectBuffer: UnsafeBufferPointer<AnyObject>,
+    to: T.Type
+  ) {
+    for i in (0..<objectBuffer.count).reversed() {


This has to go in reverse because if sizeof(T) > sizeof(AnyObject), we'll be shifting each element as we bridge it

Catfish-Man · 2018-12-12T22:38:10Z

@swift-ci please benchmark

Catfish-Man · 2018-12-12T22:38:15Z

@swift-ci please test

Catfish-Man · 2018-12-12T22:44:37Z

@swift-ci please benchmark

Catfish-Man · 2018-12-12T22:44:44Z

@swift-ci please test

swift-ci · 2018-12-12T23:05:14Z

Build failed
Swift Test Linux Platform
Git Sha - cd328ef75156fe944411c294232b5c7c9be6b082

Catfish-Man · 2018-12-13T00:36:22Z

@swift-ci please test

Catfish-Man · 2018-12-13T00:36:28Z

@swift-ci please benchmark

Catfish-Man · 2018-12-13T00:45:35Z

@swift-ci please test

Catfish-Man · 2018-12-13T00:45:40Z

@swift-ci please benchmark

swift-ci · 2018-12-13T00:48:59Z

Build failed
Swift Test Linux Platform
Git Sha - 2853a450b788edab2fbae223632244e064db15af

swift-ci · 2018-12-13T01:18:45Z

Build failed
Swift Test OS X Platform
Git Sha - cd328ef75156fe944411c294232b5c7c9be6b082

swift-ci · 2018-12-13T01:32:00Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
DataSetCountSmall	137	154	+12.4%	0.89x
Improvement
ObjectiveCBridgeFromNSDictionaryAnyObject	65922	27481	-58.3%	2.40x
NSDictionaryCastToSwift	8351	5898	-29.4%	1.42x
DictionaryBridge	1144	841	-26.5%	1.36x
Breadcrumbs.CopyUTF16CodeUnits.Mixed	64	56	-12.5%	1.14x
JSONPerfDecode	499	442	-11.4%	1.13x
PlistPerfDecode	590	532	-9.8%	1.11x
Breadcrumbs.UTF16ToIdx.longMixed	386	357	-7.5%	1.08x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
DataSetCountSmall	140	157	+12.1%	0.89x
Improvement
ObjectiveCBridgeFromNSDictionaryAnyObject	72618	28178	-61.2%	2.58x
DictionaryBridge	1301	865	-33.5%	1.50x
NSDictionaryCastToSwift	7728	5415	-29.9%	1.43x
Breadcrumbs.CopyUTF16CodeUnits.Mixed	66	56	-15.2%	1.18x (?)
PlistPerfDecode	583	525	-9.9%	1.11x
JSONPerfDecode	507	457	-9.9%	1.11x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
EqualSubstringSubstring	46	50	+8.7%	0.92x
DataSetCountSmall	200	217	+8.5%	0.92x (?)
Improvement
ObjectiveCBridgeFromNSDictionaryAnyObject	70697	30549	-56.8%	2.31x
DictionaryBridge	1296	898	-30.7%	1.44x
ArrayOfGenericPOD2	1180	1067	-9.6%	1.11x (?)
ArrayOfPOD	859	780	-9.2%	1.10x
JSONPerfDecode	509	468	-8.1%	1.09x

Code size: -swiftlibs

TEST	OLD	NEW	DELTA	RATIO
Improvement
libswiftFoundation.dylib	1654784	1630208	-1.5%	1.02x

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-12-13T01:32:47Z

@atrick please feel free to ignore the dictionary machinery, it was suggested that I run the type punning/unsafe pointer games I'm playing past you to make sure they're ok

Catfish-Man · 2018-12-13T02:57:57Z

Hm. Duplicate keys test wasn't failing locally… I'll have to have a look later.

lorentey · 2018-12-13T15:07:59Z

stdlib/public/Darwin/Foundation/NSDictionary.swift

+  ) -> Bool {
+    if _isBridgedVerbatimToObjectiveC(T.self) {
+      _verbatimBridge(objectBuffer, count: count, to: T.self)
+      return true


This doesn't look right -- verbatim bridging should still perform a checked downcast, or we would allow casting an NSDictionary of NSString keys to [NSNumber: Any].

(Maybe there is room to add an unchecked downcasting variant for use in case we're totally sure.)

Oops, it looks like we don't perform this check for verbatim-bridged forced downcasts on master, either -- we only check conditional downcasts. That seems like a bug!

Except it isn't -- the check is of course still performed when we're downcasting individual items. This is by design; bridging is done lazily in these cases. Sorry for the brain hiccup!

Despite my confusion about forced casts, conditional casts are always expected to eagerly check that all items can be bridged/downcasted to the target type.

I think I have this right in the most recent update I pushed. The verbatim ones use ? and !, and the non verbatim ones use Swift._forceBridgeFromObjectiveC and Swift._conditionallyBridgeFromObjectiveC. It's possible we could merge some of the cases now, I'm not entirely sure.

lorentey

I noticed two issues: (1) downcasts can and should fail even with verbatim bridging, and (2) if an optional cast fails, we should make sure we don't leak previously bridged items. (There should be regressions tests for these -- I'll add some soon.)

The type punning looks valid to me, but I'm by no means an expert in that! Nice work

Update: It is okay not to check nsdict as! [K: V] at the point of the cast in the verbatim-bridging case -- however, nsdict as? [K: V] must still be fully verified.

stdlib/public/Darwin/Foundation/NSDictionary.swift

lorentey · 2018-12-13T15:38:57Z

stdlib/public/Darwin/Foundation/NSDictionary.swift

+      success = _conditionallyBridge(anyKeyPtr, count: numElems, to: Key.self) &&
+        _conditionallyBridge(anyValuePtr, count: numElems, to: Value.self)
+
+      initializedCount = success ? numElems : 0


Ah, this is a neat trick. 👌

…init

Catfish-Man · 2018-12-18T18:49:50Z

Figured out why I wasn't seeing this locally. I needed to rebase.

Catfish-Man · 2018-12-19T01:52:20Z

The memory leaks the new tests noticed are really looking like a compiler bug

Catfish-Man · 2018-12-21T01:05:12Z

@swift-ci please test and merge

Catfish-Man · 2018-12-21T01:05:27Z

@swift-ci please benchmark

Catfish-Man · 2018-12-21T01:17:36Z

stdlib/public/Darwin/Foundation/NSDictionary.swift

+        success = _conditionallyBridge(objectVals,
+                                       count: numElems, to: Value.self)
+        if !success {
+          (UnsafeMutableRawPointer(mutating: objectKeys).assumingMemoryBound(to:


For reference, this was the fix for the memory leak >.< cleaning up the buffer that failed worked fine, but if the second buffer failed we wouldn't clean up the first one.

Catfish-Man · 2018-12-21T01:27:33Z

@swift-ci please benchmark

Catfish-Man · 2018-12-21T01:27:42Z

@swift-ci please test and merge

swift-ci · 2018-12-21T02:03:23Z

Build comment file:

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Improvement
ObjectiveCBridgeFromNSDictionaryAnyObject	65799	40500	-38.4%	1.62x
NSDictionaryCastToSwift	6860	5062	-26.2%	1.36x
DictionaryBridge	1091	839	-23.1%	1.30x
JSONPerfDecode	452	369	-18.4%	1.22x
PlistPerfDecode	521	450	-13.6%	1.16x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Improvement
ObjectiveCBridgeFromNSDictionaryAnyObject	69098	42475	-38.5%	1.63x
NSDictionaryCastToSwift	7753	5129	-33.8%	1.51x
DictionaryBridge	1157	806	-30.3%	1.44x
JSONPerfDecode	469	364	-22.4%	1.29x
PlistPerfDecode	536	451	-15.9%	1.19x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Improvement
ObjectiveCBridgeFromNSDictionaryAnyObject	62732	45210	-27.9%	1.39x
DictionaryBridge	1131	879	-22.3%	1.29x
JSONPerfDecode	460	383	-16.7%	1.20x
PlistPerfDecode	537	461	-14.2%	1.16x

Code size: -swiftlibs

TEST	OLD	NEW	DELTA	RATIO
Improvement
libswiftFoundation.dylib	1617920	1597440	-1.3%	1.01x

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 · 2018-12-21T04:57:03Z

@swift-ci please test and merge

Catfish-Man · 2018-12-21T10:43:24Z

Now what >.<

jrose-apple · 2018-12-21T16:29:33Z

That looks like the spurious failure in the Ackermann benchmark @palimondo is fixing. Not your fault.

@swift-ci Please test macOS

swift-ci · 2018-12-21T16:33:52Z

Build failed
Swift Test OS X Platform
Git Sha - 01826d9b29b14e65eea28cfea0c3fab2bccc97ca

palimondo · 2018-12-21T20:15:57Z

@jrose-apple The fix for that spurious lit test failure involving Ackermann landed 20 hours ago and @Catfish-Man requested the "test and merge" 15 hours ago… What makes you think it was that issue again? (I can now only see the latest Build failure due to invalid sha)

jrose-apple · 2018-12-21T21:13:42Z

Hm. It was Ackermann that failed and the next run passed. Maybe it didn't get fixed all the way?

palimondo · 2018-12-21T21:21:57Z

I'd like to make sure.

@Catfish-Man Do you have a link to that failed CI build?

palimondo · 2018-12-21T21:25:31Z

@jrose-apple Could you please triple check the ERE in that fix?

Catfish-Man · 2018-12-21T22:28:24Z

@palimondo unfortunately I don't, sorry!

palimondo · 2018-12-21T23:03:06Z

@Catfish-Man All is good. @jrose-apple helped me see the error of my ways. Fix is on its way, pending CI test.

Catfish-Man requested a review from lorentey December 12, 2018 01:48

Catfish-Man self-assigned this Dec 12, 2018

Catfish-Man force-pushed the cheaper-by-the-dozen branch from 6445f15 to 7d4a2b6 Compare December 12, 2018 01:49

Catfish-Man force-pushed the cheaper-by-the-dozen branch from d186f68 to 80ef8d5 Compare December 12, 2018 08:29

Catfish-Man commented Dec 12, 2018

View reviewed changes

Catfish-Man force-pushed the cheaper-by-the-dozen branch from 80ef8d5 to 2853a45 Compare December 12, 2018 22:36

Catfish-Man changed the title ~~[DNM] Adopt bulk Dictionary creation in bridging~~ Adopt bulk Dictionary creation in bridging Dec 12, 2018

Catfish-Man force-pushed the cheaper-by-the-dozen branch 2 times, most recently from d55e8f1 to 8783e66 Compare December 13, 2018 00:33

Catfish-Man mentioned this pull request Dec 13, 2018

[DNM] Experimentally change how Dictionary casting handles Strings, and speed up String bridging a bit to compensate #21066

Closed

Catfish-Man requested a review from atrick December 13, 2018 01:32

lorentey reviewed Dec 13, 2018

View reviewed changes

lorentey requested changes Dec 13, 2018

View reviewed changes

lorentey added 4 commits December 18, 2018 09:49

[stdlib] Dictionary: Add support for non-unique keys in bulk loading …

039891c

…init

[stdlib] Process elements from back to front

f0e04f7

[stdlib] Doc updates

f862ad3

[stdlib] Keep the first duplicate key instead of the last

9d61bad

Adopt the new bulk Dictionary initializer in bridging

016ced2

Catfish-Man force-pushed the cheaper-by-the-dozen branch from 01826d9 to 016ced2 Compare December 21, 2018 00:58

Catfish-Man commented Dec 21, 2018

View reviewed changes

Catfish-Man merged commit 4e4325d into swiftlang:master Dec 21, 2018

Catfish-Man deleted the cheaper-by-the-dozen branch December 21, 2018 19:37

This was referenced Mar 29, 2019

[stdlib] Fix availability of Dictionary’s bulk-loading initializer #23643

Merged

[5.1][stdlib] Fix availability of Dictionary’s bulk-loading initializer #23646

Merged

lorentey mentioned this pull request Jul 29, 2019

[WIP][stdlib] Dictionary: Add unsafe bulk-loading initializer #21208

Closed

Catfish-Man mentioned this pull request Dec 21, 2018

[SR-9539] Use in-place bulk initialization for Dictionary bridging #51989

Closed

Adopt bulk Dictionary creation in bridging #21235

Adopt bulk Dictionary creation in bridging #21235

Uh oh!

Conversation

Catfish-Man commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Catfish-Man commented Dec 12, 2018

Uh oh!

Catfish-Man commented Dec 12, 2018

Uh oh!

Catfish-Man Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

Catfish-Man Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Dec 12, 2018

Uh oh!

Catfish-Man commented Dec 12, 2018

Uh oh!

Catfish-Man commented Dec 12, 2018

Uh oh!

Catfish-Man commented Dec 12, 2018

Uh oh!

swift-ci commented Dec 12, 2018

Uh oh!

Catfish-Man commented Dec 13, 2018

Uh oh!

Catfish-Man commented Dec 13, 2018

Uh oh!

Catfish-Man commented Dec 13, 2018

Uh oh!

Catfish-Man commented Dec 13, 2018

Uh oh!

swift-ci commented Dec 13, 2018

Uh oh!

swift-ci commented Dec 13, 2018

Uh oh!

swift-ci commented Dec 13, 2018

Build comment file:

Performance: -O

Performance: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

Catfish-Man commented Dec 13, 2018

Uh oh!

Catfish-Man commented Dec 13, 2018

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lorentey Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Catfish-Man Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

lorentey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lorentey Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

Catfish-Man commented Dec 18, 2018

Uh oh!

Catfish-Man commented Dec 19, 2018

Uh oh!

Catfish-Man commented Dec 21, 2018

Uh oh!

Catfish-Man commented Dec 21, 2018

Catfish-Man commented Dec 12, 2018 •

edited

Loading

lorentey Dec 13, 2018 •

edited

Loading

lorentey Dec 13, 2018 •

edited

Loading

lorentey left a comment •

edited

Loading