Skip to content

[stdlib] Rewriting native hashed collection indices (rebased) #5291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Nov 4, 2016
Merged

[stdlib] Rewriting native hashed collection indices (rebased) #5291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
76c6281
[stdlib] Rewriting native hashed collection indices
Oct 5, 2016
b08732c
[stdlib] Update Dictionary comments to reflect new design
Oct 13, 2016
953e51a
[stdlib] Handle some outstanding Dictionary indexing-model FIXMEs/TODOs
Oct 13, 2016
e8f4cb6
update reflection tests with new type layouts for Dictionary
Oct 14, 2016
b7e200e
properly expose == to objc in test
Oct 17, 2016
d179655
refactor Dictionary to support an empty singleton
Oct 17, 2016
052e94a
refactor HashedCollections to support verbatim bridging
Oct 19, 2016
f12d791
update comments in HashedCollections to reflect new design
Oct 19, 2016
a43c1df
Some minor cleanups in HashedCollections:
Oct 19, 2016
3144870
add new toll-free bridge fast-paths to HashedCollections
Oct 19, 2016
41fb15f
rename Buffer <---> Storage in HashedCollections to match conventions
Nov 3, 2016
b1425ce
remove _'s from some variables that don't need it
Nov 3, 2016
8eea522
rename storage type to be more useful
Nov 3, 2016
7ec26fc
fallout of renaming on tests which rely on internals
Nov 3, 2016
8b44a07
remove nonsensical identity tests from test suite
Nov 3, 2016
bb0b4be
Update comment to clarify why it's interesting, link a relevant impl.
Nov 3, 2016
731aa14
Properly if-def out bridging code.
Nov 3, 2016
b4d9777
more comment cleanups
Nov 4, 2016
a62f6c6
Fix incorrect bitcasts, refactor code to guard against this error.
Nov 4, 2016
428cdac
leave FIXME for future dictionary bucaneers
Nov 4, 2016
5e776e5
cleanup initializers in HashedCollections
Nov 4, 2016
931f7ba
actually used the cached keys/values pointer
Nov 4, 2016
3cdc8ec
remove buffer header to eliminate optional traps
Nov 4, 2016
e4c5e15
remove useless constant redefinition
Gankra Nov 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2,345 changes: 1,208 additions & 1,137 deletions stdlib/public/core/HashedCollections.swift.gyb
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions test/stdlib/Inputs/DictionaryKeyValueTypesObjC.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public func convertNSDictionaryToDictionary<

func isNativeDictionary<KeyTy : Hashable, ValueTy>(
_ d: Dictionary<KeyTy, ValueTy>) -> Bool {
switch d._variantStorage {
switch d._variantBuffer {
case .native:
return true
case .cocoa:
Expand All @@ -35,7 +35,9 @@ func isCocoaDictionary<KeyTy : Hashable, ValueTy>(

func isNativeNSDictionary(_ d: NSDictionary) -> Bool {
let className: NSString = NSStringFromClass(type(of: d)) as NSString
return className.range(of: "_NativeDictionaryStorageOwner").length > 0
return ["_SwiftDeferredNSDictionary", "NativeDictionaryStorage"].contains {
className.range(of: $0).length > 0
}
}

func isCocoaNSDictionary(_ d: NSDictionary) -> Bool {
Expand Down
18 changes: 16 additions & 2 deletions test/stdlib/TestUserInfo.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,29 @@ import StdlibUnittest
class TestUserInfoSuper : NSObject { }
#endif

struct SubStruct {
struct SubStruct: Equatable {
var i: Int
var str: String

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it might be changing what we're testing, since the default for classes (and thus, presumably, id-as-Any boxes) is to compare by object identity.
In any case, the commit message (at least) should describe what practical effect this change has. Was the test checking the wrong thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I determined the test wanted to check for value-equality, and not object-equality. It's literally the only way this assertion makes sense: https://github.com/apple/swift/blob/master/test/stdlib/TestUserInfo.swift#L105

Unless the test really intended to rely on the pointer-stability-even-from-different-bridgings fact, in which case it's just testing for something we no longer guarantee.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Just please try to spell that kind of thing out in commit messages and/or comments

static func ==(lhs: SubStruct, rhs: SubStruct) -> Bool {
return lhs.i == rhs.i &&
lhs.str == rhs.str
}
}

struct SomeStructure {
struct SomeStructure: Hashable {
var i: Int
var str: String
var sub: SubStruct

static func ==(lhs: SomeStructure, rhs: SomeStructure) -> Bool {
return lhs.i == rhs.i &&
lhs.str == rhs.str &&
lhs.sub == rhs.sub
}

// FIXME: we don't care about this, but Any only finds == on Hashables
var hashValue: Int { return i }
}

/*
Expand Down
12 changes: 4 additions & 8 deletions validation-test/Reflection/reflect_Dictionary.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,12 @@ reflect(object: obj)
// CHECK-64: (class_instance size=25 alignment=16 stride=32 num_extra_inhabitants=0
// CHECK-64: (field name=t offset=16
// CHECK-64: (struct size=9 alignment=8 stride=16 num_extra_inhabitants=0
// CHECK-64: (field name=_variantStorage offset=0
// CHECK-64: (field name=_variantBuffer offset=0
// CHECK-64: (multi_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=0
// CHECK-64: (field name=native offset=0
// CHECK-64: (reference kind=strong refcounting=native))
// CHECK-64: (field name=cocoa offset=0
// CHECK-64: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=2147483647
// CHECK-64: (field name=cocoaDictionary offset=0
// CHECK-64: (reference kind=strong refcounting=unknown)))))))))
// CHECK-64: (reference kind=strong refcounting=native)))))))

// CHECK-32: Reflecting an object.
// CHECK-32: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
Expand All @@ -44,14 +42,12 @@ reflect(object: obj)
// CHECK-32: (class_instance size=17 alignment=16 stride=32 num_extra_inhabitants=0
// CHECK-32: (field name=t offset=12
// CHECK-32: (struct size=5 alignment=4 stride=8 num_extra_inhabitants=0
// CHECK-32: (field name=_variantStorage offset=0
// CHECK-32: (field name=_variantBuffer offset=0
// CHECK-32: (multi_payload_enum size=5 alignment=4 stride=8 num_extra_inhabitants=0
// CHECK-32: (field name=native offset=0
// CHECK-32: (reference kind=strong refcounting=native))
// CHECK-32: (field name=cocoa offset=0
// CHECK-32: (struct size=4 alignment=4 stride=4 num_extra_inhabitants=4096
// CHECK-32: (field name=cocoaDictionary offset=0
// CHECK-32: (reference kind=strong refcounting=unknown)))))))))
// CHECK-32: (reference kind=strong refcounting=native)))))))

doneReflecting()

Expand Down
12 changes: 4 additions & 8 deletions validation-test/Reflection/reflect_Set.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,12 @@ reflect(object: obj)
// CHECK-64: (class_instance size=25 alignment=16 stride=32 num_extra_inhabitants=0
// CHECK-64: (field name=t offset=16
// CHECK-64: (struct size=9 alignment=8 stride=16 num_extra_inhabitants=0
// CHECK-64: (field name=_variantStorage offset=0
// CHECK-64: (field name=_variantBuffer offset=0
// CHECK-64: (multi_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=0
// CHECK-64: (field name=native offset=0
// CHECK-64: (reference kind=strong refcounting=native))
// CHECK-64: (field name=cocoa offset=0
// CHECK-64: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=2147483647
// CHECK-64: (field name=cocoaSet offset=0
// CHECK-64: (reference kind=strong refcounting=unknown)))))))))
// CHECK-64: (reference kind=strong refcounting=native)))))))

// CHECK-32: Reflecting an object.
// CHECK-32: Instance pointer in child address space: 0x{{[0-9a-fA-F]+}}
Expand All @@ -44,14 +42,12 @@ reflect(object: obj)
// CHECK-32: (class_instance size=17 alignment=16 stride=32 num_extra_inhabitants=0
// CHECK-32: (field name=t offset=12
// CHECK-32: (struct size=5 alignment=4 stride=8 num_extra_inhabitants=0
// CHECK-32: (field name=_variantStorage offset=0
// CHECK-32: (field name=_variantBuffer offset=0
// CHECK-32: (multi_payload_enum size=5 alignment=4 stride=8 num_extra_inhabitants=0
// CHECK-32: (field name=native offset=0
// CHECK-32: (reference kind=strong refcounting=native))
// CHECK-32: (field name=cocoa offset=0
// CHECK-32: (struct size=4 alignment=4 stride=4 num_extra_inhabitants=4096
// CHECK-32: (field name=cocoaSet offset=0
// CHECK-32: (reference kind=strong refcounting=unknown)))))))))
// CHECK-32: (reference kind=strong refcounting=native)))))))

doneReflecting()

Expand Down
16 changes: 6 additions & 10 deletions validation-test/Reflection/reflect_multiple_types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,12 @@ reflect(object: obj)
// CHECK-64: (builtin size=8 alignment=8 stride=8 num_extra_inhabitants=2147483647))))))
// CHECK-64: (field name=t03 offset=48
// CHECK-64: (struct size=9 alignment=8 stride=16 num_extra_inhabitants=0
// CHECK-64: (field name=_variantStorage offset=0
// CHECK-64: (field name=_variantBuffer offset=0
// CHECK-64: (multi_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=0
// CHECK-64: (field name=native offset=0
// CHECK-64: (reference kind=strong refcounting=native))
// CHECK-64: (field name=cocoa offset=0
// CHECK-64: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=2147483647
// CHECK-64: (field name=cocoaDictionary offset=0
// CHECK-64: (reference kind=strong refcounting=unknown))))))))
// CHECK-64: (reference kind=strong refcounting=native))))))
// CHECK-64: (field name=t04 offset=64
// CHECK-64: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=0
// CHECK-64: (field name=_value offset=0
Expand Down Expand Up @@ -188,14 +186,12 @@ reflect(object: obj)
// CHECK-64: (reference kind=strong refcounting=unknown))
// CHECK-64: (field name=t15 offset=144
// CHECK-64: (struct size=9 alignment=8 stride=16 num_extra_inhabitants=0
// CHECK-64: (field name=_variantStorage offset=0
// CHECK-64: (field name=_variantBuffer offset=0
// CHECK-64: (multi_payload_enum size=9 alignment=8 stride=16 num_extra_inhabitants=0
// CHECK-64: (field name=native offset=0
// CHECK-64: (reference kind=strong refcounting=native))
// CHECK-64: (field name=cocoa offset=0
// CHECK-64: (struct size=8 alignment=8 stride=8 num_extra_inhabitants=2147483647
// CHECK-64: (field name=cocoaSet offset=0
// CHECK-64: (reference kind=strong refcounting=unknown))))))))
// CHECK-64: (reference kind=strong refcounting=native))))))
// CHECK-64: (field name=t16 offset=160
// CHECK-64: (struct size=24 alignment=8 stride=24 num_extra_inhabitants=0
// CHECK-64: (field name=_core offset=0
Expand Down Expand Up @@ -270,7 +266,7 @@ reflect(object: obj)
// CHECK-32: (builtin size=8 alignment=8 stride=8 num_extra_inhabitants=2147483647))))))
// CHECK-32: (field name=t03 offset=32
// CHECK-32: (struct size=5 alignment=4 stride=8 num_extra_inhabitants=0
// CHECK-32: (field name=_variantStorage offset=0
// CHECK-32: (field name=_variantBuffer offset=0
// CHECK-32: (multi_payload_enum size=5 alignment=4 stride=8 num_extra_inhabitants=0
// CHECK-32: (field name=native offset=0
// CHECK-32: (reference kind=strong refcounting=native))
Expand Down Expand Up @@ -316,7 +312,7 @@ reflect(object: obj)
// CHECK-32: (reference kind=strong refcounting=unknown))
// CHECK-32: (field name=t15 offset=92
// CHECK-32: (struct size=5 alignment=4 stride=8 num_extra_inhabitants=0
// CHECK-32: (field name=_variantStorage offset=0
// CHECK-32: (field name=_variantBuffer offset=0
// CHECK-32: (multi_payload_enum size=5 alignment=4 stride=8 num_extra_inhabitants=0
// CHECK-32: (field name=native offset=0
// CHECK-32: (reference kind=strong refcounting=native))
Expand Down
44 changes: 22 additions & 22 deletions validation-test/stdlib/Dictionary.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,15 +719,15 @@ DictionaryTestSuite.test("COW.Slow.RemoveValueForKeyDoesNotReallocate") {
DictionaryTestSuite.test("COW.Fast.RemoveAllDoesNotReallocate") {
do {
var d = getCOWFastDictionary()
let originalCapacity = d._variantStorage.asNative.capacity
let originalCapacity = d._variantBuffer.asNative.capacity
assert(d.count == 3)
assert(d[10]! == 1010)

d.removeAll()
// We cannot assert that identity changed, since the new buffer of smaller
// size can be allocated at the same address as the old one.
var identity1 = d._rawIdentifier()
assert(d._variantStorage.asNative.capacity < originalCapacity)
assert(d._variantBuffer.asNative.capacity < originalCapacity)
assert(d.count == 0)
assert(d[10] == nil)

Expand All @@ -740,19 +740,19 @@ DictionaryTestSuite.test("COW.Fast.RemoveAllDoesNotReallocate") {
do {
var d = getCOWFastDictionary()
var identity1 = d._rawIdentifier()
let originalCapacity = d._variantStorage.asNative.capacity
let originalCapacity = d._variantBuffer.asNative.capacity
assert(d.count == 3)
assert(d[10]! == 1010)

d.removeAll(keepingCapacity: true)
assert(identity1 == d._rawIdentifier())
assert(d._variantStorage.asNative.capacity == originalCapacity)
assert(d._variantBuffer.asNative.capacity == originalCapacity)
assert(d.count == 0)
assert(d[10] == nil)

d.removeAll(keepingCapacity: true)
assert(identity1 == d._rawIdentifier())
assert(d._variantStorage.asNative.capacity == originalCapacity)
assert(d._variantBuffer.asNative.capacity == originalCapacity)
assert(d.count == 0)
assert(d[10] == nil)
}
Expand Down Expand Up @@ -781,7 +781,7 @@ DictionaryTestSuite.test("COW.Fast.RemoveAllDoesNotReallocate") {
do {
var d1 = getCOWFastDictionary()
var identity1 = d1._rawIdentifier()
let originalCapacity = d1._variantStorage.asNative.capacity
let originalCapacity = d1._variantBuffer.asNative.capacity
assert(d1.count == 3)
assert(d1[10] == 1010)

Expand All @@ -792,7 +792,7 @@ DictionaryTestSuite.test("COW.Fast.RemoveAllDoesNotReallocate") {
assert(identity2 != identity1)
assert(d1.count == 3)
assert(d1[10]! == 1010)
assert(d2._variantStorage.asNative.capacity == originalCapacity)
assert(d2._variantBuffer.asNative.capacity == originalCapacity)
assert(d2.count == 0)
assert(d2[10] == nil)

Expand All @@ -805,15 +805,15 @@ DictionaryTestSuite.test("COW.Fast.RemoveAllDoesNotReallocate") {
DictionaryTestSuite.test("COW.Slow.RemoveAllDoesNotReallocate") {
do {
var d = getCOWSlowDictionary()
let originalCapacity = d._variantStorage.asNative.capacity
let originalCapacity = d._variantBuffer.asNative.capacity
assert(d.count == 3)
assert(d[TestKeyTy(10)]!.value == 1010)

d.removeAll()
// We cannot assert that identity changed, since the new buffer of smaller
// size can be allocated at the same address as the old one.
var identity1 = d._rawIdentifier()
assert(d._variantStorage.asNative.capacity < originalCapacity)
assert(d._variantBuffer.asNative.capacity < originalCapacity)
assert(d.count == 0)
assert(d[TestKeyTy(10)] == nil)

Expand All @@ -826,19 +826,19 @@ DictionaryTestSuite.test("COW.Slow.RemoveAllDoesNotReallocate") {
do {
var d = getCOWSlowDictionary()
var identity1 = d._rawIdentifier()
let originalCapacity = d._variantStorage.asNative.capacity
let originalCapacity = d._variantBuffer.asNative.capacity
assert(d.count == 3)
assert(d[TestKeyTy(10)]!.value == 1010)

d.removeAll(keepingCapacity: true)
assert(identity1 == d._rawIdentifier())
assert(d._variantStorage.asNative.capacity == originalCapacity)
assert(d._variantBuffer.asNative.capacity == originalCapacity)
assert(d.count == 0)
assert(d[TestKeyTy(10)] == nil)

d.removeAll(keepingCapacity: true)
assert(identity1 == d._rawIdentifier())
assert(d._variantStorage.asNative.capacity == originalCapacity)
assert(d._variantBuffer.asNative.capacity == originalCapacity)
assert(d.count == 0)
assert(d[TestKeyTy(10)] == nil)
}
Expand Down Expand Up @@ -867,7 +867,7 @@ DictionaryTestSuite.test("COW.Slow.RemoveAllDoesNotReallocate") {
do {
var d1 = getCOWSlowDictionary()
var identity1 = d1._rawIdentifier()
let originalCapacity = d1._variantStorage.asNative.capacity
let originalCapacity = d1._variantBuffer.asNative.capacity
assert(d1.count == 3)
assert(d1[TestKeyTy(10)]!.value == 1010)

Expand All @@ -878,7 +878,7 @@ DictionaryTestSuite.test("COW.Slow.RemoveAllDoesNotReallocate") {
assert(identity2 != identity1)
assert(d1.count == 3)
assert(d1[TestKeyTy(10)]!.value == 1010)
assert(d2._variantStorage.asNative.capacity == originalCapacity)
assert(d2._variantBuffer.asNative.capacity == originalCapacity)
assert(d2.count == 0)
assert(d2[TestKeyTy(10)] == nil)

Expand Down Expand Up @@ -2034,7 +2034,7 @@ DictionaryTestSuite.test("BridgedFromObjC.Verbatim.RemoveAll") {

d.removeAll()
assert(identity1 != d._rawIdentifier())
assert(d._variantStorage.asNative.capacity < originalCapacity)
assert(d._variantBuffer.asNative.capacity < originalCapacity)
assert(d.count == 0)
assert(d[TestObjCKeyTy(10)] == nil)
}
Expand All @@ -2049,7 +2049,7 @@ DictionaryTestSuite.test("BridgedFromObjC.Verbatim.RemoveAll") {

d.removeAll(keepingCapacity: true)
assert(identity1 != d._rawIdentifier())
assert(d._variantStorage.asNative.capacity >= originalCapacity)
assert(d._variantBuffer.asNative.capacity >= originalCapacity)
assert(d.count == 0)
assert(d[TestObjCKeyTy(10)] == nil)
}
Expand All @@ -2069,7 +2069,7 @@ DictionaryTestSuite.test("BridgedFromObjC.Verbatim.RemoveAll") {
assert(identity2 != identity1)
assert(d1.count == 3)
assert((d1[TestObjCKeyTy(10)] as! TestObjCValueTy).value == 1010)
assert(d2._variantStorage.asNative.capacity < originalCapacity)
assert(d2._variantBuffer.asNative.capacity < originalCapacity)
assert(d2.count == 0)
assert(d2[TestObjCKeyTy(10)] == nil)
}
Expand All @@ -2089,7 +2089,7 @@ DictionaryTestSuite.test("BridgedFromObjC.Verbatim.RemoveAll") {
assert(identity2 != identity1)
assert(d1.count == 3)
assert((d1[TestObjCKeyTy(10)] as! TestObjCValueTy).value == 1010)
assert(d2._variantStorage.asNative.capacity >= originalCapacity)
assert(d2._variantBuffer.asNative.capacity >= originalCapacity)
assert(d2.count == 0)
assert(d2[TestObjCKeyTy(10)] == nil)
}
Expand Down Expand Up @@ -2117,7 +2117,7 @@ DictionaryTestSuite.test("BridgedFromObjC.Nonverbatim.RemoveAll") {

d.removeAll()
assert(identity1 != d._rawIdentifier())
assert(d._variantStorage.asNative.capacity < originalCapacity)
assert(d._variantBuffer.asNative.capacity < originalCapacity)
assert(d.count == 0)
assert(d[TestBridgedKeyTy(10)] == nil)
}
Expand All @@ -2132,7 +2132,7 @@ DictionaryTestSuite.test("BridgedFromObjC.Nonverbatim.RemoveAll") {

d.removeAll(keepingCapacity: true)
assert(identity1 == d._rawIdentifier())
assert(d._variantStorage.asNative.capacity >= originalCapacity)
assert(d._variantBuffer.asNative.capacity >= originalCapacity)
assert(d.count == 0)
assert(d[TestBridgedKeyTy(10)] == nil)
}
Expand All @@ -2152,7 +2152,7 @@ DictionaryTestSuite.test("BridgedFromObjC.Nonverbatim.RemoveAll") {
assert(identity2 != identity1)
assert(d1.count == 3)
assert(d1[TestBridgedKeyTy(10)]!.value == 1010)
assert(d2._variantStorage.asNative.capacity < originalCapacity)
assert(d2._variantBuffer.asNative.capacity < originalCapacity)
assert(d2.count == 0)
assert(d2[TestBridgedKeyTy(10)] == nil)
}
Expand All @@ -2172,7 +2172,7 @@ DictionaryTestSuite.test("BridgedFromObjC.Nonverbatim.RemoveAll") {
assert(identity2 != identity1)
assert(d1.count == 3)
assert(d1[TestBridgedKeyTy(10)]!.value == 1010)
assert(d2._variantStorage.asNative.capacity >= originalCapacity)
assert(d2._variantBuffer.asNative.capacity >= originalCapacity)
assert(d2.count == 0)
assert(d2[TestBridgedKeyTy(10)] == nil)
}
Expand Down
Loading