Skip to content
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.

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

Let deque have its capacity shrunk #316

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions Sources/DequeModule/Deque+Collection.swift
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,22 @@ extension Deque: RangeReplaceableCollection {
_storage.ensureUnique(minimumCapacity: minimumCapacity, linearGrowth: true)
}

/// Reduces the capacity to store at least the specified number of elements.
///
/// Use this method to request that the deque reduces its storage capacity.
/// If the capacity of the deque is larger than the target capacity then
/// the capacity _may_ be reduced. If the capacity of the deque is smaller
/// than the requested capacity, or the deque contains more elements than
/// the requested capacity, then the capacity won't be changed.
///
/// - Parameters:
/// - targetCapacity: The requested capacity of the deque.
///
/// - Complexity: O(`count`)
public mutating func shrinkCapacity(_ targetCapacity: Int) {
_storage.shrink(targetCapacity: targetCapacity)
}

/// Replaces a range of elements with the elements in the specified
/// collection.
///
Expand Down
10 changes: 9 additions & 1 deletion Sources/DequeModule/Deque+Testing.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ extension Deque {
_storage.capacity
}

@_spi(Testing)
public var _requestedCapacity: Int {
_storage.requestedCapacity
}

/// The number of the storage slot in this deque that holds the first element.
/// (Or would hold it after an insertion in case the deque is currently
/// empty.)
Expand All @@ -67,7 +72,10 @@ extension Deque {
precondition(contents.count <= capacity)
let startSlot = _Slot(at: startSlot)
let buffer = _DequeBuffer<Element>.create(minimumCapacity: capacity) { _ in
_DequeBufferHeader(capacity: capacity, count: contents.count, startSlot: startSlot)
_DequeBufferHeader(capacity: capacity,
requestedCapacity: capacity,
count: contents.count,
startSlot: startSlot)
}
let storage = Deque<Element>._Storage(unsafeDowncast(buffer, to: _DequeBuffer.self))
if contents.count > 0 {
Expand Down
42 changes: 39 additions & 3 deletions Sources/DequeModule/Deque._Storage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ extension Deque._Storage {
#else
let capacity = $0.capacity
#endif
return _DequeBufferHeader(capacity: capacity, count: 0, startSlot: .zero)
return _DequeBufferHeader(capacity: capacity,
requestedCapacity: minimumCapacity,
count: 0,
startSlot: .zero)
})
self.init(_buffer: _Buffer(unsafeBufferObject: object))
}
Expand Down Expand Up @@ -85,6 +88,12 @@ extension Deque._Storage {
_buffer.withUnsafeMutablePointerToHeader { $0.pointee.capacity }
}

@inlinable
@inline(__always)
internal var requestedCapacity: Int {
_buffer.withUnsafeMutablePointerToHeader { $0.pointee.requestedCapacity }
}

@inlinable
@inline(__always)
internal var count: Int {
Expand All @@ -94,8 +103,7 @@ extension Deque._Storage {
@inlinable
@inline(__always)
internal var startSlot: _DequeSlot {
_buffer.withUnsafeMutablePointerToHeader { $0.pointee.startSlot
}
_buffer.withUnsafeMutablePointerToHeader { $0.pointee.startSlot }
}
}

Expand Down Expand Up @@ -170,6 +178,11 @@ extension Deque._Storage {
minimumCapacity)
}

@usableFromInline
internal func _reduceCapacity(to targetCapacity: Int) -> Int {
return Swift.max(0, Swift.min(requestedCapacity, targetCapacity))
}

/// Ensure that we have a uniquely referenced buffer with enough space to
/// store at least `minimumCapacity` elements.
///
Expand Down Expand Up @@ -211,4 +224,27 @@ extension Deque._Storage {
}
}
}

@inlinable
@inline(__always)
internal mutating func shrink(targetCapacity: Int) {
if count > targetCapacity { return }
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this raises a question: for a Deque with a handful of elements that is backed by a humungous buffer, wouldn't we want to allow the buffer to get shrunk to fit by calling shrink(targetCapacity: count) (or shrink(targetCapacity: 0))?

I suppose that instead of merely comparing directly against the count and capacity, we would want to only allow reallocation when the resulting capacity delta is "large enough" -- whatever that means. (An absolute cutoff? A percentage of existing capacity? Perhaps a combination of both. Probably the percentage value should be somewhat correlated with the regular growth factor; e.g. it would be wasteful to reallocate storage to 1000 items if it is currently only at 1001...) Something along the lines of this condition, maybe?

let delta = requestedCapacity - Swift.max(count, targetCapacity)
let threshold = Swift.max(2 * capacity / 3, 16)
guard delta > threshold else { return }

The numbers above are rather arbitrary and if we go this way, finding the right ones will require building a bit of a model.

if _slowPath(targetCapacity < requestedCapacity) {
_shrink(targetCapacity: targetCapacity)
}
}

@inlinable
internal mutating func _shrink(targetCapacity: Int) {
let minimumCapacity = _reduceCapacity(to: targetCapacity)
if isUnique() {
self = self.update { source in
source.moveElements(minimumCapacity: minimumCapacity)
}
} else {
self = self.read { source in
source.copyElements(minimumCapacity: minimumCapacity)
}
}
}
}
2 changes: 2 additions & 0 deletions Sources/DequeModule/Deque._UnsafeHandle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ extension Deque._UnsafeHandle {
#endif
return _DequeBufferHeader(
capacity: capacity,
requestedCapacity: minimumCapacity,
count: count,
startSlot: .zero)
})
Expand Down Expand Up @@ -340,6 +341,7 @@ extension Deque._UnsafeHandle {
#endif
return _DequeBufferHeader(
capacity: capacity,
requestedCapacity: minimumCapacity,
count: count,
startSlot: .zero)
})
Expand Down
2 changes: 1 addition & 1 deletion Sources/DequeModule/_DequeBuffer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,6 @@ extension _DequeBuffer: CustomStringConvertible {
internal let _emptyDequeStorage = _DequeBuffer<Void>.create(
minimumCapacity: 0,
makingHeaderWith: { _ in
_DequeBufferHeader(capacity: 0, count: 0, startSlot: .init(at: 0))
_DequeBufferHeader(capacity: 0, requestedCapacity: 0, count: 0, startSlot: .init(at: 0))
})

7 changes: 6 additions & 1 deletion Sources/DequeModule/_DequeBufferHeader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,19 @@ internal struct _DequeBufferHeader {
@usableFromInline
var capacity: Int

@usableFromInline
var requestedCapacity: Int

@usableFromInline
var count: Int

@usableFromInline
var startSlot: _DequeSlot

@usableFromInline
init(capacity: Int, count: Int, startSlot: _DequeSlot) {
init(capacity: Int, requestedCapacity: Int, count: Int, startSlot: _DequeSlot) {
self.capacity = capacity
self.requestedCapacity = requestedCapacity
self.count = count
self.startSlot = startSlot
_checkInvariants()
Expand All @@ -32,6 +36,7 @@ internal struct _DequeBufferHeader {
@usableFromInline @inline(never) @_effects(releasenone)
internal func _checkInvariants() {
precondition(capacity >= 0)
precondition(requestedCapacity <= capacity)
precondition(count >= 0 && count <= capacity)
precondition(startSlot.position >= 0 && startSlot.position <= capacity)
}
Expand Down
35 changes: 35 additions & 0 deletions Tests/DequeTests/DequeTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -374,5 +374,40 @@ final class DequeTests: CollectionTestCase {
}
}

func test_shrinkCapacityWhenEmpty() {
withEvery("capacity", in: [0, 1, 2, 5, 10, 50, 100]) { capacity in
var deque = Deque<Int>(minimumCapacity: capacity)

XCTAssertGreaterThanOrEqual(deque._capacity, capacity)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please use the custom expect* assertions from _CollectionsTestSupport -- XCTAssert* does not know about the context, so its failure messages will not identify the case that failed, making debugging much harder.

XCTAssertLessThanOrEqual(deque._requestedCapacity, capacity)
Copy link
Member

Choose a reason for hiding this comment

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

Can the _requestedCapacity of the created deque ever be below the capacity we requested? (I.e., shouldn't this be expecting the two values to be equal?)


let actual = deque._capacity
let reducedCapacity = deque._requestedCapacity / 4
deque.shrinkCapacity(reducedCapacity)

print(actual, deque._capacity)
// Shrinkage isn't guaranteed.
XCTAssertGreaterThanOrEqual(deque._capacity, reducedCapacity)
XCTAssertLessThanOrEqual(deque._requestedCapacity, reducedCapacity)
}
}

func test_shrinkCapacityWhenMoreElementsThanTarget() {
withEvery("count", in: [0, 1, 2, 5, 10, 50, 100]) { count in
var deque = Deque(repeating: 0, count: count)
let capacity = deque._capacity
deque.shrinkCapacity(0)
XCTAssertEqual(deque._capacity, capacity)
}
}

func test_shrinkCapacityWhenTargetIsLargerThanCurrent() {
withEvery("capacity", in: [0, 1, 2, 5, 10, 50, 100]) { capacity in
var deque = Deque<Int>(minimumCapacity: capacity)
let capacity = deque._capacity
deque.shrinkCapacity(capacity + 1)
XCTAssertEqual(deque._capacity, capacity)
}
}
}