Skip to content

Commit 699ec22

Browse files
grynspangrynspan
authored
Revert "Eagerly move attachable values to the heap. (#809)" (#810)
This reverts commit 3a537b8. @grynspan should have done more digging before implementing this change, because it turns out the compiler/runtime already refcounts large existential boxes, so this work is redundant. 🫠 ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
1 parent 3a537b8 commit 699ec22

File tree

2 files changed

+10
-153
lines changed

2 files changed

+10
-153
lines changed

‎Sources/Testing/Attachments/Test.Attachment.swift

Lines changed: 4 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,7 @@ extension Test {
6868
named preferredName: String? = nil,
6969
sourceLocation: SourceLocation = #_sourceLocation
7070
) {
71-
if _shouldMoveValueToHeap(attachableValue) {
72-
self.attachableValue = _HeapAllocatedAttachableProxy(rawValue: attachableValue)
73-
} else {
74-
self.attachableValue = attachableValue
75-
}
71+
self.attachableValue = attachableValue
7672
self.preferredName = preferredName ?? Self.defaultPreferredName
7773
self.sourceLocation = sourceLocation
7874
}
@@ -99,71 +95,10 @@ extension Test.Attachment {
9995
}
10096
}
10197

102-
// MARK: - Heap allocation optimizations
103-
104-
/// A type that stands in for an attachable value of value type (as opposed to
105-
/// reference type.)
106-
///
107-
/// We don't know the in-memory layout of an attachable value; it may be very
108-
/// expensive to make the typical copies Swift makes over the course of a test
109-
/// run if a value is large or contains lots of references to objects. So we
110-
/// eagerly move attachments of value type to the heap by ensconcing them in
111-
/// instances of this type.
112-
private final class _HeapAllocatedAttachableProxy<RawValue>: RawRepresentable, Test.Attachable, Sendable where RawValue: Test.Attachable & Sendable {
113-
let rawValue: RawValue
114-
115-
init(rawValue: RawValue) {
116-
self.rawValue = rawValue
117-
}
118-
119-
var estimatedAttachmentByteCount: Int? {
120-
rawValue.estimatedAttachmentByteCount
121-
}
122-
123-
func withUnsafeBufferPointer<R>(for attachment: borrowing Test.Attachment, _ body: (UnsafeRawBufferPointer) throws -> R) throws -> R {
124-
try rawValue.withUnsafeBufferPointer(for: attachment, body)
125-
}
126-
}
127-
128-
/// Whether or not an attachable value should be moved to the heap when adding
129-
/// it to an attachment.
130-
///
131-
/// - Parameters:
132-
/// - attachableValue: The value that is being attached.
133-
///
134-
/// - Returns: Whether or not `attachableValue` should be boxed in an instance
135-
/// of `_HeapAllocatedAttachableProxy`.
136-
private func _shouldMoveValueToHeap<T>(_ attachableValue: borrowing T) -> Bool where T: Test.Attachable {
137-
// Do not (redundantly) move existing heap-allocated objects to the heap.
138-
if T.self is AnyClass {
139-
return false
140-
}
141-
142-
// Do not move small POD value types to the heap.
143-
//
144-
// _isPOD() is defined in the standard library and is approximately equivalent
145-
// to testing for conformance to BitwiseCopyable, but it's a marker protocol
146-
// which means we can't test for conformance in a generic context.
147-
//
148-
// The size limit used is the size of an existential box's inline storage (see
149-
// NumWords_ValueBuffer in the Swift repository.) Such values can be directly
150-
// stored inside an `Attachment` and are cheap to copy.
151-
if _isPOD(T.self) && MemoryLayout<T>.stride <= (MemoryLayout<Int>.stride * 3) {
152-
return false
153-
}
154-
155-
return true
156-
}
157-
15898
// MARK: - Non-sendable and move-only attachments
15999

160-
/// A type that stands in for an attachable value that is not also sendable.
161-
///
162-
/// This type is a value type so that we can mutate its properties while
163-
/// transferring information from the original attachable value. Consequently,
164-
/// instances will be boxed in an instance of `_HeapAllocatedAttachableProxy`
165-
/// when they are used to initialize instances of ``Test/Attachment``.
166-
private struct _EagerlyCopiedAttachableProxy: Test.Attachable, Sendable {
100+
/// A type that stands in for an attachable type that is not also sendable.
101+
private struct _AttachableProxy: Test.Attachable, Sendable {
167102
/// The result of `withUnsafeBufferPointer(for:_:)` from the original
168103
/// attachable value.
169104
var encodedValue = [UInt8]()
@@ -200,7 +135,7 @@ extension Test.Attachment {
200135
named preferredName: String? = nil,
201136
sourceLocation: SourceLocation = #_sourceLocation
202137
) {
203-
var proxyAttachable = _EagerlyCopiedAttachableProxy()
138+
var proxyAttachable = _AttachableProxy()
204139
proxyAttachable.estimatedAttachmentByteCount = attachableValue.estimatedAttachmentByteCount
205140

206141
// BUG: the borrow checker thinks that withErrorRecording() is consuming

‎Tests/TestingTests/AttachmentTests.swift

Lines changed: 6 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -199,58 +199,6 @@ struct AttachmentTests {
199199
}
200200
}
201201

202-
@Test func attachRefCountedValue() throws {
203-
let attachableValue = MySendableAttachableWithRefcountedSemantics(values: (1, 2, 3))
204-
let attachment = Test.Attachment(attachableValue, named: "loremipsum")
205-
206-
#expect(attachment.attachableValue is MySendableAttachableWithRefcountedSemantics)
207-
#expect(attachment.attachableValue.estimatedAttachmentByteCount == attachableValue.estimatedAttachmentByteCount)
208-
try attachment.attachableValue.withUnsafeBufferPointer(for: attachment) { buffer in
209-
withUnsafeBytes(of: (1, 2, 3)) { buffer2 in
210-
#expect(buffer.elementsEqual(buffer2))
211-
}
212-
}
213-
}
214-
215-
@Test func attachSmallPODValue() throws {
216-
let attachableValue = MySendableAttachableWithSmallPODSemantics(values: (1, 2, 3))
217-
let attachment = Test.Attachment(attachableValue, named: "loremipsum")
218-
219-
#expect(!(type(of: attachment.attachableValue) is AnyClass))
220-
#expect(attachment.attachableValue.estimatedAttachmentByteCount == attachableValue.estimatedAttachmentByteCount)
221-
try attachment.attachableValue.withUnsafeBufferPointer(for: attachment) { buffer in
222-
withUnsafeBytes(of: (1, 2, 3)) { buffer2 in
223-
#expect(buffer.elementsEqual(buffer2))
224-
}
225-
}
226-
}
227-
228-
@Test func attachBigPODValue() throws {
229-
let attachableValue = MySendableAttachableWithBigPODSemantics(values: (1, 2, 3, 4, 5, 6))
230-
let attachment = Test.Attachment(attachableValue, named: "loremipsum")
231-
232-
#expect(type(of: attachment.attachableValue) is AnyClass)
233-
#expect(attachment.attachableValue.estimatedAttachmentByteCount == attachableValue.estimatedAttachmentByteCount)
234-
try attachment.attachableValue.withUnsafeBufferPointer(for: attachment) { buffer in
235-
withUnsafeBytes(of: (1, 2, 3, 4, 5, 6)) { buffer2 in
236-
#expect(buffer.elementsEqual(buffer2))
237-
}
238-
}
239-
}
240-
241-
@Test func attachNonPODValue() throws {
242-
let attachableValue = MySendableAttachableWithNonPODSemantics(values: (1, 2, 3))
243-
let attachment = Test.Attachment(attachableValue, named: "loremipsum")
244-
245-
#expect(type(of: attachment.attachableValue) is AnyClass)
246-
#expect(attachment.attachableValue.estimatedAttachmentByteCount == attachableValue.estimatedAttachmentByteCount)
247-
try attachment.attachableValue.withUnsafeBufferPointer(for: attachment) { buffer in
248-
withUnsafeBytes(of: (1, 2, 3)) { buffer2 in
249-
#expect(buffer.elementsEqual(buffer2))
250-
}
251-
}
252-
}
253-
254202
@Test func issueRecordedWhenAttachingNonSendableValueThatThrows() async {
255203
await confirmation("Attachment detected") { valueAttached in
256204
await confirmation("Issue recorded") { issueRecorded in
@@ -374,39 +322,13 @@ struct MySendableAttachable: Test.Attachable, Sendable {
374322
}
375323
}
376324

377-
final class MySendableAttachableWithRefcountedSemantics: Test.Attachable, Sendable {
378-
let values: (Int, Int, Int)
379-
380-
init(values: (Int, Int, Int)) {
381-
self.values = values
382-
}
383-
384-
func withUnsafeBufferPointer<R>(for attachment: borrowing Testing.Test.Attachment, _ body: (UnsafeRawBufferPointer) throws -> R) throws -> R {
385-
try withUnsafeBytes(of: values, body)
386-
}
387-
}
388-
389-
struct MySendableAttachableWithSmallPODSemantics: Test.Attachable, Sendable {
390-
var values: (Int, Int, Int)
391-
392-
func withUnsafeBufferPointer<R>(for attachment: borrowing Testing.Test.Attachment, _ body: (UnsafeRawBufferPointer) throws -> R) throws -> R {
393-
try withUnsafeBytes(of: values, body)
394-
}
395-
}
396-
397-
struct MySendableAttachableWithBigPODSemantics: Test.Attachable, Sendable {
398-
var values: (Int, Int, Int, Int, Int, Int)
399-
400-
func withUnsafeBufferPointer<R>(for attachment: borrowing Testing.Test.Attachment, _ body: (UnsafeRawBufferPointer) throws -> R) throws -> R {
401-
try withUnsafeBytes(of: values, body)
402-
}
403-
}
404-
405-
struct MySendableAttachableWithNonPODSemantics: Test.Attachable, Sendable {
406-
var values: (Int, Int, Int)
407-
var makeItComplicated = "it's complicated"
325+
struct MySendableAttachableWithDefaultByteCount: Test.Attachable, Sendable {
326+
var string: String
408327

409328
func withUnsafeBufferPointer<R>(for attachment: borrowing Testing.Test.Attachment, _ body: (UnsafeRawBufferPointer) throws -> R) throws -> R {
410-
try withUnsafeBytes(of: values, body)
329+
var string = string
330+
return try string.withUTF8 { buffer in
331+
try body(.init(buffer))
332+
}
411333
}
412334
}

0 commit comments

Comments
 (0)