From 55637febe4952c8655aa63d57e16ef2b28be2f23 Mon Sep 17 00:00:00 2001 From: Elijah Semyonov Date: Fri, 27 Sep 2024 14:57:39 +0200 Subject: [PATCH] Revisit indexing operators and fix leaks and crashes (#548) * Fixes #543 (Godot String via Variant subscript operator leaks) and #531 (Variant crash) * Reduce performance checks iterations x10. * Fix dictionary leaks and a crash when dictionary[key] = nil --- ...53914494-654F-4AE6-BFE1-6777D0E5FBA3.plist | 6 +- Generator/Generator/BuiltinGen.swift | 38 +- Sources/SwiftGodot/Core/GArray.swift | 13 +- Sources/SwiftGodot/Variant.swift | 3 +- Tests/SwiftGodotTests/MarshalTests.swift | 6 +- Tests/SwiftGodotTests/MemoryLeakTests.swift | 516 ++++++++++++++++-- 6 files changed, 523 insertions(+), 59 deletions(-) diff --git a/.swiftpm/xcode/xcshareddata/xcbaselines/SwiftGodotTests.xcbaseline/53914494-654F-4AE6-BFE1-6777D0E5FBA3.plist b/.swiftpm/xcode/xcshareddata/xcbaselines/SwiftGodotTests.xcbaseline/53914494-654F-4AE6-BFE1-6777D0E5FBA3.plist index 84f3981ac..448d781db 100644 --- a/.swiftpm/xcode/xcshareddata/xcbaselines/SwiftGodotTests.xcbaseline/53914494-654F-4AE6-BFE1-6777D0E5FBA3.plist +++ b/.swiftpm/xcode/xcshareddata/xcbaselines/SwiftGodotTests.xcbaseline/53914494-654F-4AE6-BFE1-6777D0E5FBA3.plist @@ -11,7 +11,7 @@ com.apple.XCTPerformanceMetric_WallClockTime baselineAverage - 2.190000 + 0.223000 baselineIntegrationDisplayName Local Baseline @@ -21,7 +21,7 @@ com.apple.XCTPerformanceMetric_WallClockTime baselineAverage - 4.504656 + 0.446000 baselineIntegrationDisplayName Local Baseline @@ -31,7 +31,7 @@ com.apple.XCTPerformanceMetric_WallClockTime baselineAverage - 3.270000 + 0.332172 baselineIntegrationDisplayName Local Baseline diff --git a/Generator/Generator/BuiltinGen.swift b/Generator/Generator/BuiltinGen.swift index 714a0df27..0587b76b6 100644 --- a/Generator/Generator/BuiltinGen.swift +++ b/Generator/Generator/BuiltinGen.swift @@ -508,28 +508,30 @@ func generateBuiltinMethods (_ p: Printer, p.staticVar (visibility: "private ", name: "keyed_checker", type: "GDExtensionPtrKeyedChecker") { p ("return gi.variant_get_ptr_keyed_checker (\(variantType))!") } - p ("public subscript (key: Variant) -> Variant?") { - p ("get") { - p ("let keyCopy = key") - p ("var result = Variant.zero") - p ("if Self.keyed_checker (&content, &keyCopy.content) != 0") { - p ("Self.keyed_getter (&content, &keyCopy.content, &result)") - p ("return Variant(copying: result)") - } - p ("else") { - p ("return nil") + p(""" + public subscript(key: Variant) -> Variant? { + get { + var result = Variant.zero + if Self.keyed_checker(&content, &key.content) != 0 { + Self.keyed_getter (&content, &key.content, &result) + // Returns unowned handle + return Variant(takingOver: result) + } else { + return nil } } - p ("set") { - p ("let keyCopy = key") - p ("if let newCopy = newValue") { - p ("Self.keyed_setter (&content, &keyCopy.content, &newCopy.content)") - } - p ("else") { - p ("Self.keyed_setter (&content, &keyCopy.content, nil)") - } + + set { + if let newValue { + Self.keyed_setter(&content, &key.content, &newValue.content) + } else { + var nilContent = Variant.zero + // nil will cause a crash, needs a pointer to Nil Variant content instead + Self.keyed_setter(&content, &key.content, &nilContent) + } } } + """) } if let returnType = bc.indexingReturnType, !bc.isKeyed, !bc.name.hasSuffix ("Array"), bc.name != "String" { let godotType = getGodotType (JGodotReturnValue (type: returnType, meta: nil)) diff --git a/Sources/SwiftGodot/Core/GArray.swift b/Sources/SwiftGodot/Core/GArray.swift index d55a93544..bcda7f665 100644 --- a/Sources/SwiftGodot/Core/GArray.swift +++ b/Sources/SwiftGodot/Core/GArray.swift @@ -28,6 +28,8 @@ extension GArray { return Variant() } let ptr = ret.assumingMemoryBound(to: Variant.ContentType.self) + + // We are making a copy of the variant managed by the array. Array is managing its copy, we are managing ours return Variant(copying: ptr.pointee) } set { @@ -35,7 +37,16 @@ extension GArray { return } let ptr = ret.assumingMemoryBound(to: Variant.ContentType.self) - ptr.pointee = newValue.content + + guard ptr.pointee != newValue.content else { + return + } + + // We are taking the variant from the array at the `index` and assuming control over it. Since we don't need it, we just destroy it + gi.variant_destroy(ptr) + + // We are giving array a copy of `newValue` Variant to manage + gi.variant_new_copy(ptr, &newValue.content) } } diff --git a/Sources/SwiftGodot/Variant.swift b/Sources/SwiftGodot/Variant.swift index dfa0e61fb..3076c9ac9 100644 --- a/Sources/SwiftGodot/Variant.swift +++ b/Sources/SwiftGodot/Variant.swift @@ -273,7 +273,8 @@ public class Variant: Hashable, Equatable, CustomDebugStringConvertible { if valid == 0 || oob != 0 { return nil } - return Variant(copying: _result) + + return Variant(takingOver: _result) } set { guard let newValue else { diff --git a/Tests/SwiftGodotTests/MarshalTests.swift b/Tests/SwiftGodotTests/MarshalTests.swift index af2dfe807..a5aae5537 100644 --- a/Tests/SwiftGodotTests/MarshalTests.swift +++ b/Tests/SwiftGodotTests/MarshalTests.swift @@ -39,7 +39,7 @@ final class MarshalTests: GodotTestCase { let child = TestVariant() measure { - for _ in 0..<10000000 { + for _ in 0..<1_000_000 { tv.addChild(node: child) tv.removeChild(node: child) } @@ -58,7 +58,7 @@ final class MarshalTests: GodotTestCase { let weight = 0.23 measure { - for _ in 0..<10000000 { + for _ in 0..<1_000_000 { let _ = a.cubicInterpolate(b: b, preA: preA, postB: preB, weight: weight) } } @@ -68,7 +68,7 @@ final class MarshalTests: GodotTestCase { let randomValues = (0..<10).map { _ in Variant(Float.random(in: -1.0...1.0)) } measure { - for _ in 0..<1000000 { + for _ in 0..<100_000 { let _ = GD.max(arg1: randomValues[0], arg2: randomValues[1], randomValues[2], randomValues[3], randomValues[4], randomValues[5], randomValues[6], randomValues[7], randomValues[8], randomValues[9]) } } diff --git a/Tests/SwiftGodotTests/MemoryLeakTests.swift b/Tests/SwiftGodotTests/MemoryLeakTests.swift index f4ded5417..293ce50eb 100644 --- a/Tests/SwiftGodotTests/MemoryLeakTests.swift +++ b/Tests/SwiftGodotTests/MemoryLeakTests.swift @@ -2,7 +2,397 @@ import SwiftGodot import SwiftGodotTestability import XCTest +struct Foo: Codable { + var myInt: Int + var myText: String + var myIntArray: [Int] +} + +struct FooN: Codable { + var myInt: Int + var myText: String + var myFoo: [Foo] +} + +protocol GodotEncodingContainer { + var data: Variant { get } +} + +extension Vector2: Codable { + enum CodingKeys: String, CodingKey { + case x + case y + } + + public init (from decoder: Decoder) throws { + let values = try decoder.container(keyedBy: CodingKeys.self) + let x = try values.decode(Float.self, forKey: .x) + let y = try values.decode(Float.self, forKey: .y) + self.init (x: x, y: y) + } + + public func encode(to encoder: any Encoder) throws { + var container = encoder.container(keyedBy: CodingKeys.self) + try container.encode(x, forKey: .x) + try container.encode(x, forKey: .y) + } +} + +/// Notes on encoding: +/// - Int8 is encoded as an Int +class GodotEncoder: Encoder { + var codingPath: [any CodingKey] = [] + var userInfo: [CodingUserInfoKey : Any] = [:] + var container: GodotEncodingContainer? = nil + + init () { + + } + + func container(keyedBy type: Key.Type) -> KeyedEncodingContainer where Key : CodingKey { + + var container = GodotKeyedContainer(codingPath: codingPath, userInfo: userInfo) + self.container = container + return KeyedEncodingContainer(container) + } + + var data: Variant { + return container?.data ?? Variant() + } + + func encode(key codingKey: [CodingKey], value: Variant) { + let key = codingKey.map { $0.stringValue }.joined(separator: ".") + fatalError() + //dict [key] = value + } + + func unkeyedContainer() -> any UnkeyedEncodingContainer { + let container = GodotUnkeyedContainer(codingPath: codingPath, userInfo: userInfo) + self.container = container + return container + } + + func singleValueContainer() -> any SingleValueEncodingContainer { + let container = GodotSingleValueContainer(codingPath: codingPath, userInfo: userInfo) + self.container = container + return container + } + + class GodotKeyedContainer: KeyedEncodingContainerProtocol, GodotEncodingContainer { + func encodeNil(forKey key: Key) throws { + var container = self.nestedSingleValueContainer(forKey: key) + fatalError() + //try container.encode(Variant()) + } + + var codingPath: [any CodingKey] = [] + var userInfo: [CodingUserInfoKey: Any] + var storage: [String:GodotEncodingContainer] = [:] + + var data: Variant { + let dict = GDictionary() + for (k,v) in storage { + dict[k] = Variant(v.data) + } + return Variant(dict) + } + + init (codingPath: [any CodingKey], userInfo: [CodingUserInfoKey: Any]) { + self.codingPath = codingPath + self.userInfo = userInfo + } + + func encode(_ value: String, forKey key: Key) throws { + var container = self.nestedSingleValueContainer(forKey: key) + try container.encode(value) + self.storage[key.stringValue] = container + } + + func encode(_ value: Int, forKey key: Key) throws { + let container = self.nestedSingleValueContainer(forKey: key) + try container.encode(value) + self.storage[key.stringValue] = container + } + + func nestedSingleValueContainer(forKey key: Key) + -> GodotSingleValueContainer + { + let container = GodotSingleValueContainer( + codingPath: self.codingPath + [key], + userInfo: self.userInfo + ) + return container + } + + func encode(_ value: T, forKey key: Key) throws where T : Encodable { + let container = self.nestedSingleValueContainer(forKey: key) + try container.encode(value) + self.storage[key.stringValue] = container + + } + + func nestedContainer(keyedBy keyType: NestedKey.Type, forKey key: Key) -> KeyedEncodingContainer where NestedKey : CodingKey { + fatalError() + } + + func nestedUnkeyedContainer(forKey key: Key) -> any UnkeyedEncodingContainer { + fatalError() + } + + func superEncoder() -> any Encoder { + fatalError() + } + + func superEncoder(forKey key: Key) -> any Encoder { + fatalError() + } + } + + class GodotUnkeyedContainer: UnkeyedEncodingContainer, GodotEncodingContainer { + var codingPath: [any CodingKey] = [] + var userInfo: [CodingUserInfoKey: Any] + + var storage = GArray() + + var data: Variant { + return Variant(storage) + } + + init (codingPath: [any CodingKey], userInfo: [CodingUserInfoKey: Any]) { + self.codingPath = codingPath + self.userInfo = userInfo + } + + var count: Int { + Int(storage.size()) + } + + func encode(_ value: String) throws { + fatalError() + } + + func encode(_ value: Double) throws { + fatalError() + } + + func encode(_ value: Float) throws { + fatalError() + } + + func encode(_ value: Int) throws { + fatalError() + } + + func encode(_ value: Int8) throws { + fatalError() + } + + func encode(_ value: Int16) throws { + fatalError() + } + + func encode(_ value: Int32) throws { + fatalError() + } + + func encode(_ value: Int64) throws { + fatalError() + } + + func encode(_ value: UInt) throws { + fatalError() + } + + func encode(_ value: UInt8) throws { + fatalError() + } + + func encode(_ value: UInt16) throws { + fatalError() + } + + func encode(_ value: UInt32) throws { + fatalError() + } + + func encode(_ value: UInt64) throws { + fatalError() + } + + func encode(_ value: T) throws where T: Encodable { + let base = Int(storage.size()) + + + if let v = value as? Array { + _ = storage.resize(size: storage.size () + Int64(v.count)) + for i in 0..(keyedBy keyType: NestedKey.Type) -> KeyedEncodingContainer where NestedKey : CodingKey { + fatalError() + } + + func nestedUnkeyedContainer() -> any UnkeyedEncodingContainer { + fatalError() + } + + func superEncoder() -> any Encoder { + fatalError() + } + + + } + + class GodotSingleValueContainer: SingleValueEncodingContainer, GodotEncodingContainer { + func encode(_ value: T) throws where T : Encodable { + if value is Array { + var container = GodotUnkeyedContainer (codingPath: codingPath, userInfo: userInfo) + try container.encode(value) + self.value = container.data + } else if let i = value as? Int { + try self.encode(i) + } else if let str = value as? String { + try self.encode(str) + } else { + var nested = GodotEncoder() + try value.encode(to: nested) + self.value = nested.container?.data + } + } + + enum foo: Error { + case nope + } + + var codingPath: [any CodingKey] = [] + var userInfo: [CodingUserInfoKey: Any] + var value: Variant? + + var data: Variant { + value ?? Variant() + } + + init (codingPath: [any CodingKey], userInfo: [CodingUserInfoKey: Any]) { + self.codingPath = codingPath + self.userInfo = userInfo + } + + func encodeNil() throws { + fatalError() + } + + func encode(_ value: Bool) throws { + fatalError() + } + + func encode(_ value: String) throws { + self.value = Variant(value) + } + + func encode(_ value: Double) throws { + fatalError() + } + + func encode(_ value: Float) throws { + fatalError() + } + + func encode(_ value: Int) throws { + self.value = Variant(Int(value)) + } + + func encode(_ value: Int8) throws { + fatalError() + } + + func encode(_ value: Int16) throws { + fatalError() + } + + func encode(_ value: Int32) throws { + fatalError() + } + + func encode(_ value: Int64) throws { + fatalError() + } + + func encode(_ value: UInt) throws { + fatalError() + } + + func encode(_ value: UInt8) throws { + fatalError() + } + + func encode(_ value: UInt16) throws { + fatalError() + } + + func encode(_ value: UInt32) throws { + fatalError() + } + + func encode(_ value: UInt64) throws { + fatalError() + } + } +} + final class MemoryLeakTests: GodotTestCase { + /// Check that `body` doesn't leak. Or ensure that something is leaking, if `useUnoReverseCard` is true + func checkLeaks(useUnoReverseCard: Bool = false, _ body: () -> Void) { + let before = Performance.getMonitor(.memoryStatic) + body() + let after = Performance.getMonitor(.memoryStatic) + + if useUnoReverseCard { + XCTAssertNotEqual(before, after, "It should leak!") + } else { + XCTAssertEqual(before, after, "Leaked \(after - before) bytes") + } + } + + func testThatItLeaksIndeed() { + let array = GArray() + + checkLeaks(useUnoReverseCard: true) { + array.append(Variant(10)) + } + } // https://github.com/migueldeicaza/SwiftGodot/issues/513 func test_513_leak1() { @@ -18,17 +408,12 @@ final class MemoryLeakTests: GodotTestCase { // Warm-up the code path in case it performs any one-time permanent allocations. oneIteration(object: object) - - let before = Performance.getMonitor(.memoryStatic) - let count = 1_000 - - for _ in 0 ..< count { - oneIteration(object: object) + + checkLeaks { + for _ in 0 ..< 1_000 { + oneIteration(object: object) + } } - - let after = Performance.getMonitor(.memoryStatic) - - XCTAssertEqual(before, after, "Leaked \(Int((after - before) / Double(count))) bytes per iteration.") } // https://github.com/migueldeicaza/SwiftGodot/issues/513 @@ -48,31 +433,20 @@ final class MemoryLeakTests: GodotTestCase { // Warm-up the code path in case it performs any one-time permanent allocations. oneIteration(bytes: bytes) - let before = Performance.getMonitor(.memoryStatic) - let count = 1_000 - - for _ in 0 ..< count { - oneIteration(bytes: bytes) + checkLeaks { + for _ in 0 ..< 1_000 { + oneIteration(bytes: bytes) + } } - - let after = Performance.getMonitor(.memoryStatic) - - XCTAssertEqual(before, after, "Leaked \(Int((after - before) / Double(count))) bytes per iteration.") } // https://github.com/migueldeicaza/SwiftGodot/issues/541 func test_541_leak() { - let before = Performance.getMonitor(.memoryStatic) - - for _ in 0...10000000 { - autoreleasepool { + checkLeaks { + for _ in 0...1_000 { let _ = Variant("daosdoasodasoda") } } - - let after = Performance.getMonitor(.memoryStatic) - - XCTAssertEqual(before, after, "Leaked \(Int(after - before))") } // https://github.com/migueldeicaza/SwiftGodot/issues/544 @@ -83,14 +457,90 @@ final class MemoryLeakTests: GodotTestCase { // https://docs.godotengine.org/en/stable/classes/class_string.html#class-string-method-left let methodName = StringName("left") - let before = Performance.getMonitor(.memoryStatic) + checkLeaks { + for _ in 0 ..< 2_000 { + let _ = variant.call(method: methodName, Variant(2)) + } + } + } + + // https://github.com/migueldeicaza/SwiftGodot/issues/543 + func test_543_leak() { + let string = "Hello, World!" + let variant = Variant(string) + + checkLeaks { + for _ in 0 ..< 2000 { + _ = variant[0] + } + } + } + + func test_array_leaks() { + let array = GArray() + array.append(Variant("S")) + array.append(Variant("M")) - for _ in 0 ..< 2000 { - let _ = variant.call(method: methodName, Variant(2)) + checkLeaks { + XCTAssertEqual(array[0], Variant("S")) + + for _ in 0 ..< 1_000 { + array[0] = Variant("T") + _ = array[1] + } } - let after = Performance.getMonitor(.memoryStatic) - - XCTAssertEqual(before, after, "Leaked \(Int(after - before)) bytes") + XCTAssertEqual(array[0], Variant("T")) + + let variant = Variant(array) + + checkLeaks { + XCTAssertEqual(variant[0], Variant("T")) + + for _ in 0 ..< 1_000 { + variant[0] = Variant("U") + variant[1] = Variant("K") + } + + XCTAssertEqual(variant[1], Variant("K")) + } + + XCTAssertEqual(variant[0], Variant("U")) + } + + func test_531_crash_or_leak() { + checkLeaks { + let g = GodotEncoder() + let foon = FooN(myInt: 9, myText: "nine", myFoo: [ + Foo(myInt: 10, myText: "Nested1", myIntArray: [1,1,1]), + Foo(myInt: 30, myText: "Nested2", myIntArray: [2,2,2])]) + try? foon.encode(to: g) + + print(g.data.description) + } + } + + func test_dictionary_leaks() { + checkLeaks { + let dictionary = GDictionary() + + for i in 0 ..< 1_000 { + let variant = Variant("value\(i)") + dictionary["key\(i * 2)"] = variant + dictionary["key\(i * 2 + 1)"] = variant + } + + for i in 0 ..< 1_000 { + let key = "key\(Int.random(in: 0 ..< 2_000))" + let value = dictionary[key] + dictionary["key\(i * 2)"] = value + } + + for i in 0 ..< 1_000 { + _ = dictionary.erase(key: Variant("\(i * 2)")) + } + + dictionary.clear() + } } }