Skip to content

[Deque] Work around stdlib issue with Array.withContiguousStorageIfAvailable #44

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 1 commit into from
May 28, 2021
Merged

[Deque] Work around stdlib issue with Array.withContiguousStorageIfAvailable #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
62 changes: 62 additions & 0 deletions Sources/DequeModule/Compatibility.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift Collections open source project
//
// Copyright (c) 2021 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
//
//===----------------------------------------------------------------------===//

extension Array {
/// Returns true if `Array.withContiguousStorageIfAvailable` is broken
/// in the stdlib we're currently running on.
///
/// See https://bugs.swift.org/browse/SR-14663.
@inlinable
internal static func _isWCSIABroken() -> Bool {
#if _runtime(_ObjC)
guard _isBridgedVerbatimToObjectiveC(Element.self) else {
// SR-14663 only triggers on array values that are verbatim bridged
// from Objective-C, so it cannot ever trigger for element types
// that aren't verbatim bridged.
return false
}

// SR-14663 was introduced in Swift 5.1. Check if we have a broken stdlib.

// The bug is caused by a bogus precondition inside a non-inlinable stdlib
// method, so to determine if we're affected, we need to check the currently
// running OS version.
#if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
guard #available(macOS 10.15, iOS 13, watchOS 6, tvOS 13, *) else {
// The OS is too old to be affected by this bug.
return false
}
#endif
// FIXME: When a stdlib is released that contains a fix, add a check for it.
return true

#else
// Platforms that don't have an Objective-C runtime don't have verbatim
// bridged array values, so the bug doesn't apply to them.
return false
#endif
}
}

extension Sequence {
// An adjusted version of the standard `withContiguousStorageIfAvailable`
// method that works around https://bugs.swift.org/browse/SR-14663.
@inlinable
internal func _withContiguousStorageIfAvailable_SR14663<R>(
_ body: (UnsafeBufferPointer<Element>) throws -> R
) rethrows -> R? {
if Self.self == Array<Element>.self && Array<Element>._isWCSIABroken() {
return nil
Copy link
Member Author

@lorentey lorentey May 27, 2021

Choose a reason for hiding this comment

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

If we had a reliable/easy way to test if a particular array instance is bridged, then we could let native arrays pass through here. 🤔

Copy link

@karwa karwa May 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe:

if let array = self as? Array<Element>, array._baseAddressIfContiguous == nil { return nil }

This will call down in to ArrayBuffer.firstElementAddressIfContiguous, which returns nil if not native.

Tested in a Playground:

import Foundation
var a: [NSObject] = [NSObject()]
print(a._baseAddressIfContiguous) // Optional(0x00007fdf7e40c0d0)

a = NSArray(array: [NSObject()]) as! [NSObject]
print(a._baseAddressIfContiguous) // nil

Copy link
Member Author

@lorentey lorentey May 27, 2021

Choose a reason for hiding this comment

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

Huh, that's a good find. Looking at similarly exposed members, Array._hoistableIsNativeTypeChecked() might be even better.

There are (at least) two worries with using these underscored APIs: (including _isBridgedVerbatimToObjectiveC above)

  1. Swift doesn't promise it won't change their behavior, so it's tricky to safely use them in code that is not part of the stdlib. E.g., I imagine for element types that don't require a dynamic type check at every access, we may want to change _baseAddressIfContiguous to return non-nil for bridged arrays that use specific known NSArray subclasses.

  2. These have also been made public for historical reasons, rather than any intentional design, and we may find a good reason to change them to @inlinable internal, disallowing direct calls like this in future SDKs. We wouldn't be able to remove the entry points themselves (they are part of Swift's ABI), but we might need to restrict them to only be called from inlinable functions within the stdlib.

As this package is maintained by the same people who work on the Swift stdlib, I think it's not unreasonable for this package to use these. At the first sign of trouble, we can remove the calls, or limit them to older Swift toolchains. The primary danger is that this code will spread beyond this package through vendoring or uncritical copy-n-paste.

My instinct as a stdlib engineer tells me that the risk is low with _hoistableIsNativeTypeChecked (and pretty much nonexistent for _isBridgedVerbatimToObjectiveC); on the other hand, this package is inherently averse to even low-risk stuff. Hm.

Copy link
Member Author

@lorentey lorentey May 27, 2021

Choose a reason for hiding this comment

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

I think the best thing to do is to keep this as is for now until we have a Swift release (let's call it x.y) that contains a fix for the underlying issue. In the (unlikely) case that the fix won't back deploy, then we can add a call to _hoistableIsNativeTypeChecked restricted to only be called on Swift 5.1 ..< x.y.

Copy link
Member

@stephentyrone stephentyrone May 27, 2021

Choose a reason for hiding this comment

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

I think that's fine. I would also be OK with doing the workaround here now, so long as we put a comment in the stdlib that references this bug.

}

return try self.withContiguousStorageIfAvailable(body)
}
}
8 changes: 7 additions & 1 deletion Sources/DequeModule/Deque+Extras.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ extension Deque {
/// - SeeAlso: `append(contentsOf:)`
@inlinable
public mutating func prepend<C: Collection>(contentsOf newElements: C) where C.Element == Element {
let done: Void? = newElements.withContiguousStorageIfAvailable { source in
let done: Void? = newElements._withContiguousStorageIfAvailable_SR14663 { source in
_storage.ensureUnique(minimumCapacity: count + source.count)
_storage.update { $0.uncheckedPrepend(contentsOf: source) }
}
Expand Down Expand Up @@ -166,6 +166,12 @@ extension Deque {
/// - SeeAlso: `append(contentsOf:)`
@inlinable
public mutating func prepend<S: Sequence>(contentsOf newElements: S) where S.Element == Element {
let done: Void? = newElements._withContiguousStorageIfAvailable_SR14663 { source in
Copy link
Member Author

@lorentey lorentey May 27, 2021

Choose a reason for hiding this comment

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

I couldn't resist slipping in this trivial performance improvement, if only for symmetry with the sequence-based append(contentsOf:).

_storage.ensureUnique(minimumCapacity: count + source.count)
_storage.update { $0.uncheckedPrepend(contentsOf: source) }
}
guard done == nil else { return }

let originalCount = self.count
self.append(contentsOf: newElements)
let newCount = self.count
Expand Down
6 changes: 3 additions & 3 deletions Sources/DequeModule/Deque+RangeReplaceableCollection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ extension Deque: RangeReplaceableCollection {
_storage.update { handle in
assert(handle.startSlot == .zero)
let target = handle.mutableBuffer(for: .zero ..< _Slot(at: c))
let done: Void? = elements.withContiguousStorageIfAvailable { source in
let done: Void? = elements._withContiguousStorageIfAvailable_SR14663 { source in
target._initialize(from: source)
}
if done == nil {
Expand Down Expand Up @@ -198,7 +198,7 @@ extension Deque: RangeReplaceableCollection {
/// - Complexity: Amortized O(`newElements.count`).
@inlinable
public mutating func append<S: Sequence>(contentsOf newElements: S) where S.Element == Element {
let done: Void? = newElements.withContiguousStorageIfAvailable { source in
let done: Void? = newElements._withContiguousStorageIfAvailable_SR14663 { source in
_storage.ensureUnique(minimumCapacity: count + source.count)
_storage.update { $0.uncheckedAppend(contentsOf: source) }
}
Expand Down Expand Up @@ -240,7 +240,7 @@ extension Deque: RangeReplaceableCollection {
/// - Complexity: Amortized O(`newElements.count`).
@inlinable
public mutating func append<C: Collection>(contentsOf newElements: C) where C.Element == Element {
let done: Void? = newElements.withContiguousStorageIfAvailable { source in
let done: Void? = newElements._withContiguousStorageIfAvailable_SR14663 { source in
_storage.ensureUnique(minimumCapacity: count + source.count)
_storage.update { $0.uncheckedAppend(contentsOf: source) }
}
Expand Down
55 changes: 53 additions & 2 deletions Tests/DequeTests/DequeTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,13 +301,14 @@ final class DequeTests: CollectionTestCase {
}
}

func test_prependManyFromArray() {
func test_prependManyFromContiguousArray_asCollection() {
withEveryDeque("deque", ofCapacities: [0, 1, 2, 3, 5, 10]) { layout in
withEvery("prependCount", in: 0 ..< 10) { prependCount in
withEvery("isShared", in: [false, true]) { isShared in
withLifetimeTracking { tracker in
var (deque, contents) = tracker.deque(with: layout)
let extra = tracker.instances(for: layout.count ..< layout.count + prependCount)
let extraRange = layout.count ..< layout.count + prependCount
let extra = ContiguousArray(tracker.instances(for: extraRange))
withHiddenCopies(if: isShared, of: &deque) { deque in
contents.insert(contentsOf: extra, at: 0)
deque.prepend(contentsOf: extra)
Expand All @@ -319,5 +320,55 @@ final class DequeTests: CollectionTestCase {
}
}

func test_prependManyFromContiguousArray_asSequence() {
// This calls the Sequence-based `Deque.prepend` overload, even if
// `elements` happens to be of a Collection type.
func prependSequence<S: Sequence>(
contentsOf elements: S,
to deque: inout Deque<S.Element>
) {
deque.prepend(contentsOf: elements)
}

withEveryDeque("deque", ofCapacities: [0, 1, 2, 3, 5, 10]) { layout in
withEvery("prependCount", in: 0 ..< 10) { prependCount in
withEvery("isShared", in: [false, true]) { isShared in
withLifetimeTracking { tracker in
var (deque, contents) = tracker.deque(with: layout)
let extraRange = layout.count ..< layout.count + prependCount
let extra = ContiguousArray(tracker.instances(for: extraRange))
withHiddenCopies(if: isShared, of: &deque) { deque in
contents.insert(contentsOf: extra, at: 0)
prependSequence(contentsOf: extra, to: &deque)
expectEqualElements(deque, contents)
}
}
}
}
}
}

func test_prependManyFromBridgedArray() {
Copy link
Member

@stephentyrone stephentyrone May 27, 2021

Choose a reason for hiding this comment

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

Thank you for adding the additional test cases =)

// https://github.com/apple/swift-collections/issues/27
withEveryDeque("deque", ofCapacities: [0, 1, 2, 3, 5, 10]) { layout in
withEvery("appendCount", in: 0 ..< 10) { appendCount in
withEvery("isShared", in: [false, true]) { isShared in
var contents: [NSObject] = (0 ..< layout.count).map { _ in NSObject() }
var deque = Deque(layout: layout, contents: contents)
let extra: [NSObject] = (0 ..< appendCount)
.map { _ in NSObject() }
.withUnsafeBufferPointer { buffer in
NSArray(objects: buffer.baseAddress, count: buffer.count) as! [NSObject]
}
withHiddenCopies(if: isShared, of: &deque) { deque in
contents.insert(contentsOf: extra, at: 0)
deque.prepend(contentsOf: extra)
expectEquivalentElements(deque, contents, by: ===)
}
}
}
}
}

}

49 changes: 47 additions & 2 deletions Tests/DequeTests/RangeReplaceableCollectionTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,28 @@ final class RangeReplaceableCollectionTests: CollectionTestCase {
}
}

func test_sequenceInitializer_ContiguousArray() {
withEvery("count", in: [0, 1, 2, 10, 100]) { count in
withLifetimeTracking { tracker in
let contents = ContiguousArray(tracker.instances(for: 0 ..< count))
let d1 = Deque(contents)
expectEqualElements(d1, contents)
}
}
}

func test_sequenceInitializer_bridgedArray() {
// https://github.com/apple/swift-collections/issues/27
withEvery("count", in: [0, 1, 2, 10, 100]) { count in
let contents: [AnyObject] = (0 ..< count).map { _ in NSObject() }
let array: [AnyObject] = contents.withUnsafeBufferPointer { buffer in
NSArray(objects: buffer.baseAddress, count: buffer.count) as [AnyObject]
}
let deque = Deque(array)
expectEquivalentElements(deque, contents, by: ===)
}
}

func test_replaceSubrange_withMinimalCollection() {
withEveryDeque("deque", ofCapacities: [0, 1, 2, 3, 5, 10]) { layout in
withEveryRange("range", in: 0 ..< layout.count) { range in
Expand Down Expand Up @@ -172,13 +194,14 @@ final class RangeReplaceableCollectionTests: CollectionTestCase {
}
}

func test_appendManyFromArray() {
func test_appendManyFromContiguousArray() {
withEveryDeque("deque", ofCapacities: [0, 1, 2, 3, 5, 10]) { layout in
withEvery("appendCount", in: 0 ..< 10) { appendCount in
withEvery("isShared", in: [false, true]) { isShared in
withLifetimeTracking { tracker in
var (deque, contents) = tracker.deque(with: layout)
let extra = tracker.instances(for: layout.count ..< layout.count + appendCount)
let extraRange = layout.count ..< layout.count + appendCount
let extra = ContiguousArray(tracker.instances(for: extraRange))
withHiddenCopies(if: isShared, of: &deque) { deque in
contents.append(contentsOf: extra)
deque.append(contentsOf: extra)
Expand All @@ -190,6 +213,28 @@ final class RangeReplaceableCollectionTests: CollectionTestCase {
}
}

func test_appendManyFromBridgedArray() {
// https://github.com/apple/swift-collections/issues/27
withEveryDeque("deque", ofCapacities: [0, 1, 2, 3, 5, 10]) { layout in
withEvery("appendCount", in: 0 ..< 10) { appendCount in
withEvery("isShared", in: [false, true]) { isShared in
var contents: [NSObject] = (0 ..< layout.count).map { _ in NSObject() }
var deque = Deque(layout: layout, contents: contents)
let extra: [NSObject] = (0 ..< appendCount)
.map { _ in NSObject() }
.withUnsafeBufferPointer { buffer in
NSArray(objects: buffer.baseAddress, count: buffer.count) as! [NSObject]
}
withHiddenCopies(if: isShared, of: &deque) { deque in
contents.append(contentsOf: extra)
deque.append(contentsOf: extra)
expectEquivalentElements(deque, contents, by: ===)
}
}
}
}
}

func test_insertOneElement() {
withEveryDeque("deque", ofCapacities: [0, 1, 2, 3, 5, 10]) { layout in
withEvery("offset", in: 0 ... layout.count) { offset in
Expand Down