[stdlib] Fix Set/Dictionary casting issues #23683

lorentey · 2019-03-30T03:29:55Z

This PR fixes Set/Dictionary casts to correctly handle casting between String and NSString keys. (String has a more permissive concept of equality than NSString, which can lead to duplicate entries when we convert NSString-keyed hash tables to String.)

Bridging was fixed to handle this in the Swift 4 era, but the same issue can occur during casting between Swift-native Set and Dictionary types.

Set upcasts have been failing since Swift 4.2:

import Foundation
let s: Set<NSString> = [
    "cafe\u{301}",
    "café",
]
let s2 = s as Set<String>

⟹ Fatal error: Duplicate elements of type 'String' were found in a Set.
⟹ This usually means either that the type violates Hashable's requirements, or
⟹ that members of such a set were mutated after insertion.

(The trap is not guaranteed to happen immediately, or at all.)

Dictionary upcasts worked well up to and including Swift 5.0, but since #20911, they have the same issue:

import Foundation
let d: Dictionary<NSString, NSObject> = [
    "cafe\u{301}": NSObject(),
    "café": NSObject(),
]
let d2 = d as Dictionary<String, NSObject>

⟹ Fatal error: Duplicate elements of type 'String' were found in a Dictionary.
⟹ This usually means either that the type violates Hashable's requirements, or
⟹ that members of such a set were mutated after insertion.

These traps can also occur in downcasting situations, e.g., when casting from NSObject to String keys.

Additionally, this PR changes the forced-downcast code for sets and dictionaries to fail with a clear runtime error instead of a generic nil unwrap.

let s1: Set<AnyHashable> = ["Gordon", "William", "Katherine", "Lynn", "Brian", 1756]
let s2 = s1 as! Set<String>

⟹ before: Fatal error: Unexpectedly found nil while unwrapping an Optional value
⟹ after: Could not cast value of type 'Swift.AnyHashable' (0x110c09c48) to 'Swift.String' (0x110c0c160).

rdar://49444002

- Fix Set/Dictionary up/downcasting with String keys. - Improve error handling.

lorentey · 2019-03-30T03:30:08Z

@swift-ci test

swift-ci · 2019-03-30T05:01:34Z

Build failed
Swift Test Linux Platform
Git Sha - ceb703b

lorentey · 2019-03-30T05:13:01Z

00:01:31 Test Case 'TestXMLDocument.test_xpath' started at 2019-03-30 05:01:30.894
00:01:31 
00:01:31 
00:01:31 0% tests passed, 1 tests failed out of 1
00:01:31 
00:01:31 Total Test time (real) =  23.25 sec
00:01:31 
00:01:31 The following tests FAILED:
00:01:31 	  1 - TestFoundation (SEGFAULT)
00:01:31 Errors while running CTest

🤔

lorentey · 2019-03-30T05:13:29Z

@swift-ci please test linux platform

lorentey · 2019-03-30T21:09:16Z

@swift-ci benchmark

Catfish-Man

This looks great overall (sorry for breaking part of it!). Two questions/comments, but not things that need to block merging it.

stdlib/public/core/DictionaryCasting.swift

Catfish-Man · 2019-03-30T21:34:19Z

stdlib/public/core/NativeDictionary.swift

+  /// Insert a new element into uniquely held storage.  Storage must be uniquely
+  /// referenced with adequate capacity.  If `allowingDuplicates` is false,
+  /// `element` must not be already present in the dictionary.
+  @_alwaysEmitIntoClient @inlinable // Introduced in 5.1


Does this get specialized enough that the unused code path gets DCE'd if the type is known to the client? It'd be a shame to inline stuff they don't need.

The flag gets folded into a constant, but the branch on it doesn't get eliminated unless the insertWithGuaranteedCapacity call is inlined -- unfortunately it normally isn't.

In the latest commit, I reorganized the code to move the branch out of the loop, ensuring dead code elimination will trigger.

swift-ci · 2019-03-30T22:01:41Z

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
DictionaryBridgeToObjC_Access	924	997	+7.9%	0.93x (?)
Improvement
ObjectiveCBridgeFromNSSetAnyObject	54800	42200	-23.0%	1.30x
LessSubstringSubstring	44	39	-11.4%	1.13x
EqualStringSubstring	43	39	-9.3%	1.10x
EqualSubstringString	43	39	-9.3%	1.10x
LessSubstringSubstringGenericComparable	43	39	-9.3%	1.10x (?)
FlattenListLoop	4336	3976	-8.3%	1.09x
SortStringsUnicode	3565	3270	-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
FlattenListFlatMap	6551	6070	-7.3%	1.08x (?)
MapReduce	397	368	-7.3%	1.08x
EqualSubstringSubstring	42	39	-7.1%	1.08x
EqualSubstringSubstringGenericEquatable	42	39	-7.1%	1.08x
MapReduceAnyCollection	397	369	-7.1%	1.08x
ObjectiveCBridgeFromNSStringForced	2565	2385	-7.0%	1.08x (?)

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
Hanoi	3660	4010	+9.6%	0.91x (?)
ObjectiveCBridgeToNSSet	18000	19450	+8.1%	0.93x (?)
Improvement
ObjectiveCBridgeFromNSSetAnyObject	54200	42200	-22.1%	1.28x
EqualSubstringSubstring	44	39	-11.4%	1.13x (?)
ObjectiveCBridgeFromNSSetAnyObjectToString	84000	75000	-10.7%	1.12x (?)
EqualStringSubstring	43	39	-9.3%	1.10x (?)
LessSubstringSubstring	44	40	-9.1%	1.10x (?)
EqualSubstringString	44	40	-9.1%	1.10x
LessSubstringSubstringGenericComparable	44	40	-9.1%	1.10x
FlattenListLoop	4431	4070	-8.1%	1.09x (?)
RemoveWhereMoveInts	37	34	-8.1%	1.09x (?)
Array2D	7520	6912	-8.1%	1.09x (?)
SortStringsUnicode	3560	3285	-7.7%	1.08x (?)
RemoveWhereSwapInts	67	62	-7.5%	1.08x
EqualSubstringSubstringGenericEquatable	43	40	-7.0%	1.07x (?)
MapReduce	433	404	-6.7%	1.07x

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
DictionaryKeysContains.o	8108	8620	+6.3%	0.94x
DictionaryCompactMapValues.o	13461	13773	+2.3%	0.98x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
StrComplexWalk	6640	7570	+14.0%	0.88x (?)
Breadcrumbs.CopyUTF16CodeUnits.ASCII	184	208	+13.0%	0.88x (?)
Breadcrumbs.CopyUTF16CodeUnits.Mixed	217	240	+10.6%	0.90x (?)
Improvement
ObjectiveCBridgeFromNSSetAnyObject	57000	46200	-18.9%	1.23x (?)
ArrayOfGenericPOD2	1235	1065	-13.8%	1.16x (?)
ArrayOfPOD	856	776	-9.3%	1.10x (?)
ObjectiveCBridgeFromNSSetAnyObjectToString	88000	80000	-9.1%	1.10x (?)
EqualStringSubstring	50	46	-8.0%	1.09x (?)
ObjectiveCBridgeFromNSStringForced	2735	2545	-6.9%	1.07x (?)

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

airspeedswift · 2019-04-02T18:38:49Z

Is this GTM?

lorentey · 2019-04-02T20:11:18Z

@swift-ci please test

lorentey · 2019-04-02T20:11:26Z

@swift-ci benchmark

lorentey · 2019-04-02T20:15:41Z

stdlib/public/core/NativeSet.swift

@@ -351,7 +362,7 @@ extension _NativeSet { // Insertions
  /// The `element` must not be already present in the Set.
  @inlinable
  internal func _unsafeInsertNew(_ element: __owned Element) {
-    _internalInvariant(count + 1 <= capacity)


Ah, this change doesn't belong here; I'll remove it in a followup commit.

lorentey · 2019-04-02T20:29:28Z

@Catfish-Man Could you please review again? The last commit replaced most of the implementation.

Catfish-Man

Love it

lorentey · 2019-04-03T00:36:46Z

@swift-ci benchmark

lorentey · 2019-04-03T00:36:54Z

@swift-ci smoke test and merge

swift-ci · 2019-04-03T01:35:49Z

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
InsertCharacterEndIndexNonASCII	58	63	+8.6%	0.92x (?)
Breadcrumbs.IdxToUTF16.longMixed	737	793	+7.6%	0.93x (?)
Improvement
ObjectiveCBridgeFromNSSetAnyObject	54200	40000	-26.2%	1.35x
ObjectiveCBridgeFromNSSetAnyObjectToString	84500	73500	-13.0%	1.15x (?)
ObjectiveCBridgeStubFromNSDateRef	4630	4060	-12.3%	1.14x (?)

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
DictionaryCompactMapValues.o	17259	18939	+9.7%	0.91x
DictionaryCopy.o	10102	10934	+8.2%	0.92x
DictTest2.o	14696	15544	+5.8%	0.95x
DictionaryRemove.o	15752	16488	+4.7%	0.96x
DictTest.o	19769	20649	+4.5%	0.96x
DictTest3.o	21960	22824	+3.9%	0.96x
DictionarySwap.o	21266	21986	+3.4%	0.97x
RGBHistogram.o	26698	27466	+2.9%	0.97x
DictionarySubscriptDefault.o	27962	28714	+2.7%	0.97x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
Dict.CopyKeyValue.28k	3038	3330	+9.6%	0.91x (?)
Dict.CopyKeyValue.16k	1573	1720	+9.3%	0.91x (?)
Dict.CopyKeyValue.20k	1740	1895	+8.9%	0.92x
Dict.CopyKeyValue.24k	1951	2101	+7.7%	0.93x (?)
Improvement
ObjectiveCBridgeFromNSSetAnyObject	54400	40600	-25.4%	1.34x
ObjectiveCBridgeFromNSSetAnyObjectToString	84000	74000	-11.9%	1.14x (?)
StringComparison_fastPrenormal	1060	980	-7.5%	1.08x (?)
FloatingPointPrinting_Float_description_small	5832	5400	-7.4%	1.08x (?)

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
DictionaryCompactMapValues.o	13461	14133	+5.0%	0.95x
ReversedCollections.o	9850	9986	+1.4%	0.99x
Improvement
DictTest2.o	9865	9705	-1.6%	1.02x
DictTest.o	12617	12457	-1.3%	1.01x
DictionaryRemove.o	11211	11083	-1.1%	1.01x
DictTest3.o	14105	13945	-1.1%	1.01x
DictionaryCopy.o	7942	7862	-1.0%	1.01x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
CharacterPropertiesStashedMemo	4230	4760	+12.5%	0.89x (?)
CharacterPropertiesStashed	3250	3630	+11.7%	0.90x (?)
DataAppendDataSmallToSmall	4280	4640	+8.4%	0.92x (?)
Improvement
ObjectiveCBridgeFromNSSetAnyObject	57200	44800	-21.7%	1.28x
ArrayOfPOD	853	779	-8.7%	1.09x (?)
CharIteration_ascii_unicodeScalars_Backwards	389160	356560	-8.4%	1.09x (?)
CharIteration_russian_unicodeScalars_Backwards	323280	298160	-7.8%	1.08x (?)
CharIteration_punctuatedJapanese_unicodeScalars_Backwards	66840	61720	-7.7%	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

lorentey · 2019-04-03T02:05:05Z

@swift-ci smoke test and merge

lorentey · 2019-04-05T21:25:27Z

@swift-ci smoke test linux platform

airspeedswift · 2019-04-11T23:28:23Z

@swift-ci please test

lorentey · 2019-04-12T21:44:56Z

Linux failure was unrelated; it's fixed on latest master.

@swift-ci please test and merge

lorentey · 2019-04-15T03:36:57Z

@swift-ci test linux platform

swift-ci · 2019-04-15T04:28:09Z

Build failed
Swift Test Linux Platform
Git Sha - ea6ae67

lorentey · 2019-04-15T05:08:34Z

23:28:05 <unknown>:0: warning: missing submodule 'Dispatch'
23:28:05 
<unknown>:0: error: cannot load underlying module for 'Dispatch'

😔

@swift-ci smoke test linux platform

atrick · 2019-04-15T06:30:21Z

@lorentey All the linux incremental bots are failing with "cannot load underlying module". But I think a clean test will work.

atrick · 2019-04-15T06:30:46Z

@swift-ci clean test linux platform

swift-ci · 2019-04-15T07:15:10Z

Build failed
Swift Test Linux Platform
Git Sha - ea6ae67

lorentey · 2019-04-15T17:56:33Z

Merging. While the full tests have unrelated issues on Linux, the smoke test did pass there, too. I intend to cherry-pick this on 5.1, and we can run the tests there.

lorentey added 2 commits March 29, 2019 19:15

[stdlib] Fix Set/Dictionary casting issues

2bd5da5

- Fix Set/Dictionary up/downcasting with String keys. - Improve error handling.

[test] Add new Set/Dictionary casting tests

ceb703b

lorentey requested a review from Catfish-Man March 30, 2019 03:30

Catfish-Man approved these changes Mar 30, 2019

View reviewed changes

[stdlib] Set, Dictionary: Unify casting operations

1ddf99a

This comment has been minimized.

Sign in to view

lorentey commented Apr 2, 2019

View reviewed changes

lorentey requested a review from Catfish-Man April 2, 2019 20:26

This comment has been minimized.

Sign in to view

Catfish-Man approved these changes Apr 2, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

[stlib] _NativeSet: Revert spurious change

ea6ae67

This comment has been minimized.

Sign in to view

lorentey merged commit e08b219 into swiftlang:master Apr 15, 2019

lorentey deleted the casting-improvements branch April 15, 2019 18:06

lorentey mentioned this pull request Apr 15, 2019

[5.1][stdlib] Fix Set/Dictionary casting issues #24037

Merged

[stdlib] Fix Set/Dictionary casting issues #23683

[stdlib] Fix Set/Dictionary casting issues #23683

Uh oh!

Conversation

lorentey commented Mar 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorentey commented Mar 30, 2019

Uh oh!

swift-ci commented Mar 30, 2019

Uh oh!

lorentey commented Mar 30, 2019

Uh oh!

lorentey commented Mar 30, 2019

Uh oh!

lorentey commented Mar 30, 2019

Uh oh!

Catfish-Man left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Catfish-Man Mar 30, 2019

Choose a reason for hiding this comment

Uh oh!

lorentey Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Mar 30, 2019

Performance: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Uh oh!

airspeedswift commented Apr 2, 2019

Uh oh!

lorentey commented Apr 2, 2019

Uh oh!

lorentey commented Apr 2, 2019

Uh oh!

This comment has been minimized.

lorentey Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

lorentey commented Apr 2, 2019

Uh oh!

This comment has been minimized.

Catfish-Man left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

lorentey commented Apr 3, 2019

Uh oh!

lorentey commented Apr 3, 2019

Uh oh!

swift-ci commented Apr 3, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Uh oh!

lorentey commented Apr 3, 2019

Uh oh!

lorentey commented Apr 5, 2019

Uh oh!

airspeedswift commented Apr 11, 2019

Uh oh!

This comment has been minimized.

lorentey commented Apr 12, 2019

Uh oh!

lorentey commented Apr 15, 2019

Uh oh!

swift-ci commented Apr 15, 2019

Uh oh!

lorentey commented Apr 15, 2019

Uh oh!

atrick commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atrick commented Apr 15, 2019

lorentey commented Mar 30, 2019 •

edited

Loading

atrick commented Apr 15, 2019 •

edited

Loading