From 47c77e0ec8358637f6b68f6fe23dc6ee09806bc4 Mon Sep 17 00:00:00 2001 From: Javier Segura Date: Sun, 4 Oct 2026 11:17:00 +0200 Subject: [PATCH] [Bugfix] Close the data race between Scene.assign and a reader of the slot The scene is read without its lock: `scene` hands out a copy of the Scene under the lock, and the copy is read after it. Scene.assign wrote a new component into its slot with a plain store and Scene.get read the slot with a plain load, so when a reader's copy said the slot was in use, nothing ordered the read after the new object's initialization. A slot in use gets a new component when an entity is given a component it already has (loaders register the transform and scenegraph components twice), when a component is removed and given again (a mesh that is replaced), and when the index of a destroyed entity is taken by a new one. Thread Sanitizer reports it in the three cases. It is not only a formal race: the stores of one core reach another out of order on Apple silicon. Two plain stores, a field and then the pointer to it, read from a second thread that follows the pointer, showed the pointer without the field 136 times in 20 million tries on an M4 Max, and never once the pointer is stored with a release store or a compare and exchange. A component that is an object now goes into its slot with an atomic compare and exchange, which comes after everything its initializer wrote, and is read with an acquire load. A reader gets the component the slot had or the new one, and either one whole. What assign does is unchanged: an entity given a component again still gets a new one. Reads cost what they did, within a nanosecond: the acquire load is one instruction in place of another. Scene.get lets its copy of the pool go before it reads the slot, because a pool kept across the atomic load is retained and released on every call (4 ns). A component that is a value is not one reference: it is written and read in place, as before. On the fork, which already releases components (#68): a component that leaves its slot goes to the quarantine first, and the new one is then stored atomically. Whether a slot holds one reference is kept in the chunk list's header, beside the quarantine. Scene.get asks the pool for the slot and reads it; the scene the caller holds keeps the pool's storage. (cherry picked from commit 704cb9688b2d6273b5ac49bbf5063b076304e9a3) --- Sources/UntoldEngine/ECS/ComponentPool.swift | 101 ++++++- Sources/UntoldEngine/ECS/Scenes.swift | 19 +- .../ComponentPublicationTests.swift | 248 ++++++++++++++++++ 3 files changed, 348 insertions(+), 20 deletions(-) create mode 100644 Tests/UntoldEngineTests/ComponentPublicationTests.swift diff --git a/Sources/UntoldEngine/ECS/ComponentPool.swift b/Sources/UntoldEngine/ECS/ComponentPool.swift index 4e479751c..70303f54d 100644 --- a/Sources/UntoldEngine/ECS/ComponentPool.swift +++ b/Sources/UntoldEngine/ECS/ComponentPool.swift @@ -95,6 +95,60 @@ public protocol Component { init() // Requires a default initializer } +/// How a component gets into its slot and out of it. +/// +/// A slot is read without the scene's lock: the scene is handed out by value under the +/// lock and the copy is read after it, and a render pass may keep a copy for as long as +/// it runs. While a copy says a slot holds a component, another thread may put a new +/// one there: the entity is given the component again, or it lost the component and +/// gets it back, or it was destroyed and a new entity took its index. +/// +/// So a component that is an object, as the engine's are, goes into its slot with an +/// atomic store that comes after everything its initializer wrote, and comes out with +/// an atomic load that takes that order. A reader gets the component the slot had or +/// the new one, and either one whole. +/// +/// A component that is a value is not one reference and cannot be swapped in one step. +/// It is written and read in place: do not give one again to an entity that another +/// thread may be reading. +enum ComponentSlot { + /// Whether a slot of `T` holds one reference to an object. + static func holdsReference(_: T.Type) -> Bool { + T.self is AnyObject.Type + } + + /// Puts `component` in `slot`, which owns it from here on. What the slot held is + /// written over, not released. + @inline(__always) + static func store(_ component: T, in slot: UnsafeMutableRawPointer, asReference: Bool) { + guard asReference else { + slot.bindMemory(to: T.self, capacity: 1).initialize(to: component) + return + } + let reference = unsafeBitCast(component, to: UnsafeRawPointer.self) + // The slot's own reference to the object. + _ = Unmanaged.fromOpaque(reference).retain() + // Before macOS 15 and iOS 18 (Synchronization) the standard library has an atomic + // compare and exchange of a pointer and no plain atomic store. A scene is + // written under its lock, so the exchange has nobody to lose to. + var held = slot.load(as: UnsafeRawPointer?.self) + let word = slot.assumingMemoryBound(to: UnsafeRawPointer?.self) + while !_stdlib_atomicCompareExchangeStrongPtr(object: word, expected: &held, desired: reference) {} + } + + /// The component in `slot`, read as a `T`. Only for a slot that holds one. + @inline(__always) + static func load(from slot: UnsafeMutableRawPointer, as _: T.Type, asReference: Bool) -> T? { + guard asReference else { + return slot.assumingMemoryBound(to: T.self).pointee + } + guard let reference = _stdlib_atomicAcquiringLoadARCRef(object: slot.assumingMemoryBound(to: AnyObject?.self)) else { + return nil + } + return unsafeBitCast(reference.toOpaque(), to: T.self) + } +} + /// The components that left a pool and are not released yet. /// /// A component leaves its slot when it is removed from its entity, when its entity is @@ -147,6 +201,8 @@ public struct ComponentPool { private struct Header { let chunkCount: Int let elementSize: Int + /// Whether a slot holds one reference to an object (see ComponentSlot). + let holdsReferences: Bool var quarantine: ComponentQuarantine } @@ -160,19 +216,26 @@ public struct ComponentPool { private var departures: UInt64 = 0 init(for type: T.Type) { - storage = Self.makeStorage(chunks: [], elementSize: MemoryLayout.stride, quarantine: ComponentQuarantine(of: type)) + storage = Self.makeStorage( + chunks: [], elementSize: MemoryLayout.stride, holdsReferences: ComponentSlot.holdsReference(T.self), + quarantine: ComponentQuarantine(of: type) + ) } private static func makeStorage( - chunks: [UnsafeMutableRawPointer], elementSize: Int, quarantine: ComponentQuarantine + chunks: [UnsafeMutableRawPointer], elementSize: Int, holdsReferences: Bool, quarantine: ComponentQuarantine ) -> Storage { let storage = Storage.create(minimumCapacity: chunks.count) { _ in - Header(chunkCount: chunks.count, elementSize: elementSize, quarantine: quarantine) + Header(chunkCount: chunks.count, elementSize: elementSize, holdsReferences: holdsReferences, quarantine: quarantine) } storage.withUnsafeMutablePointerToElements { $0.initialize(from: chunks, count: chunks.count) } return storage } + private var holdsReferences: Bool { + storage.withUnsafeMutablePointerToHeader { $0.pointee.holdsReferences } + } + private var chunks: [UnsafeMutableRawPointer] { storage.withUnsafeMutablePointers { header, chunks in Array(UnsafeBufferPointer(start: chunks, count: header.pointee.chunkCount)) @@ -196,7 +259,7 @@ public struct ComponentPool { for chunk in chunks { chunk.deallocate() } - storage = Self.makeStorage(chunks: [], elementSize: elementSize, quarantine: quarantine) + storage = Self.makeStorage(chunks: [], elementSize: elementSize, holdsReferences: holdsReferences, quarantine: quarantine) } /// Adds chunks until `index` has storage. @@ -211,7 +274,7 @@ public struct ComponentPool { ) ) } - storage = Self.makeStorage(chunks: chunks, elementSize: elementSize, quarantine: quarantine) + storage = Self.makeStorage(chunks: chunks, elementSize: elementSize, holdsReferences: holdsReferences, quarantine: quarantine) } public func get(_ index: Int) -> UnsafeMutableRawPointer? { @@ -221,16 +284,30 @@ public struct ComponentPool { } } + /// Where the slot at `index` is, and whether it holds one reference to an object: + /// what ComponentSlot needs to read it or write it. Read and write a component + /// through ComponentSlot, not through the pointer `get` returns. + @inline(__always) + func slot(at index: Int) -> (address: UnsafeMutableRawPointer, holdsReference: Bool)? { + storage.withUnsafeMutablePointers { header, chunks in + guard index >= 0, index < header.pointee.chunkCount * Self.chunkCapacity else { return nil } + let address = chunks[index / Self.chunkCapacity].advanced(by: (index % Self.chunkCapacity) * header.pointee.elementSize) + return (address, header.pointee.holdsReferences) + } + } + /// The component in the slot at `index`, read as a `T`. Only for a slot the entity's /// mask says holds one. /// - /// Read a slot through this: the pool is kept until the component is in the caller's - /// hands, and a pool that is kept holds back the release of what left it. + /// For a reader that holds a pool on its own: the pool is kept until the component is + /// in the caller's hands, and a pool that is kept holds back the release of what left + /// it. A reader that holds the scene (Scene.get) asks for the slot and reads it: the + /// scene it holds keeps the pool. @inline(__always) func component(at index: Int, as _: T.Type) -> T? { - guard let slot = get(index) else { return nil } + guard let slot = slot(at: index) else { return nil } return withExtendedLifetime(storage) { - slot.bindMemory(to: T.self, capacity: 1).pointee + ComponentSlot.load(from: slot.address, as: T.self, asReference: slot.holdsReference) } } @@ -274,10 +351,10 @@ public struct ComponentPool { } /// Add a new component to the pool at a specified index - public mutating func add(component: T, at index: Int) { + public mutating func add(component: some Component, at index: Int) { reserve(upTo: index) - guard let pointer = get(index) else { fatalError("No storage for entity index \(index) in ComponentPool.") } - pointer.assumingMemoryBound(to: T.self).initialize(to: component) + guard let slot = slot(at: index) else { fatalError("No storage for entity index \(index) in ComponentPool.") } + ComponentSlot.store(component, in: slot.address, asReference: slot.holdsReference) } } diff --git a/Sources/UntoldEngine/ECS/Scenes.swift b/Sources/UntoldEngine/ECS/Scenes.swift index 92077bf63..f4a2c8f9c 100644 --- a/Sources/UntoldEngine/ECS/Scenes.swift +++ b/Sources/UntoldEngine/ECS/Scenes.swift @@ -180,19 +180,20 @@ public struct Scene { } // Allocate and initialize a new component in the pool - guard let componentPointer = pool.get(Int(entityIndex)) else { + guard let slot = pool.slot(at: Int(entityIndex)) else { handleError(.failedToGetComponentPointer) return nil } - let typedPointer = componentPointer.bindMemory(to: T.self, capacity: 1) - typedPointer.initialize(to: T()) + // A copy of the scene taken before this may be reading the slot (see ComponentSlot). + let component = T() + ComponentSlot.store(component, in: slot.address, asReference: slot.holdsReference) // Set the bit for this component to true entities[Int(entityIndex)].mask.set(componentId) componentIndex[componentId, default: []].insert(entityId) - return typedPointer.pointee + return component } public func get(component _: T.Type, for entityId: EntityID) -> T? { @@ -219,13 +220,15 @@ public struct Scene { return nil } - // Retrieve the specific component pool - guard let pool = componentPool[componentId] else { + // Ask the specific component pool where the component is. The pool is let go + // before the slot is read: kept across the read, it is retained and released + // on every call. This scene, which the caller holds, keeps the pool's storage. + guard let slot = componentPool[componentId]?.slot(at: Int(entityIndex)) else { return nil } - // Get the component from the pool - return pool.component(at: Int(entityIndex), as: T.self) + // Get the component from its slot + return ComponentSlot.load(from: slot.address, as: T.self, asReference: slot.holdsReference) } /// Moves the component in the slot of the entity at `entityIndex` to its pool's diff --git a/Tests/UntoldEngineTests/ComponentPublicationTests.swift b/Tests/UntoldEngineTests/ComponentPublicationTests.swift new file mode 100644 index 000000000..692395366 --- /dev/null +++ b/Tests/UntoldEngineTests/ComponentPublicationTests.swift @@ -0,0 +1,248 @@ +// +// ComponentPublicationTests.swift +// UntoldEngine +// +// Copyright (C) Untold Engine Studios +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +@testable import UntoldEngine +import XCTest + +private final class PublishedComponent: Component { + static let initial = 0x00C0_FFEE + + var value = PublishedComponent.initial + + required init() {} +} + +/// A component that is a value: nothing in `Component` says it has to be a class. +private struct ValueComponent: Component { + var first = 1 + var second = 2 + var third = 3 + + init() {} +} + +/// Whether the reader on the other thread should stop, and what it found. +private final class ReaderState: @unchecked Sendable { + private let lock = NSLock() + private var stopped = false + private var reads = 0 + private var unexpected = 0 + + func stop() { + lock.withLock { stopped = true } + } + + var isStopped: Bool { + lock.withLock { stopped } + } + + func record(reads newReads: Int, unexpected newUnexpected: Int) { + lock.withLock { + reads += newReads + unexpected += newUnexpected + } + } + + var totals: (reads: Int, unexpected: Int) { + lock.withLock { (reads, unexpected) } + } +} + +/// A copy of the scene is read after the scene's lock is released, and a slot the copy +/// says is in use may get a new component meanwhile. A component used to be written +/// into its slot with a plain store and read with a plain load, so nothing ordered the +/// read after the new object's initialization: a data race. +/// +/// The threaded tests hold one copy on a reader thread while this thread puts new +/// components in the slots the copy reads. They say the most under Thread Sanitizer: +/// +/// swift test --sanitize=thread --filter ComponentPublicationTests +@MainActor +final class ComponentPublicationTests: XCTestCase { + override func setUp() async throws { + resetEngineTestState() + } + + override func tearDown() async throws { + resetEngineTestState() + } + + // MARK: - A slot + + func testASlotOwnsItsObjectOnceAndReadsLeaveItSo() { + let slot = UnsafeMutableRawPointer.allocate( + byteCount: MemoryLayout.stride, alignment: MemoryLayout.alignment + ) + defer { slot.deallocate() } + weak var watched: PublishedComponent? + do { + let component = PublishedComponent() + watched = component + ComponentSlot.store(component, in: slot, asReference: true) + } + XCTAssertNotNil(watched, "the slot keeps the component") + + for _ in 0 ..< 3 { + let read = ComponentSlot.load(from: slot, as: PublishedComponent.self, asReference: true) + XCTAssertTrue(read === watched) + } + + // The slot's reference is the only one left: without it the component goes. + Unmanaged.fromOpaque(slot.load(as: UnsafeRawPointer.self)).release() + XCTAssertNil(watched) + } + + func testOnlyAComponentThatIsAnObjectIsKeptAsOneReference() { + XCTAssertTrue(ComponentSlot.holdsReference(PublishedComponent.self)) + XCTAssertFalse(ComponentSlot.holdsReference(ValueComponent.self)) + } + + func testAComponentThatIsAValueIsKeptInPlace() { + let entity = createEntity() + XCTAssertEqual(scene.assign(to: entity, component: ValueComponent.self)?.second, 2) + + let read = scene.get(component: ValueComponent.self, for: entity) + + XCTAssertEqual(read?.first, 1) + XCTAssertEqual(read?.second, 2) + XCTAssertEqual(read?.third, 3) + } + + func testTheSceneReadsTheComponentItWasGivenLast() { + let entity = createEntity() + let first = scene.assign(to: entity, component: PublishedComponent.self) + XCTAssertNotNil(first) + XCTAssertTrue(scene.get(component: PublishedComponent.self, for: entity) === first) + + let second = scene.assign(to: entity, component: PublishedComponent.self) + + XCTAssertNotNil(second) + XCTAssertFalse(second === first, "an entity given the component again gets a new one") + XCTAssertTrue(scene.get(component: PublishedComponent.self, for: entity) === second) + } + + func testACopyOfTheSceneReadsTheComponentItsEntityWasGivenAgain() { + // A copy shares the scene's slots: it reads what the slot holds now. + let entity = createEntity() + _ = scene.assign(to: entity, component: PublishedComponent.self) + let copy = scene + + let second = scene.assign(to: entity, component: PublishedComponent.self) + + XCTAssertTrue(copy.get(component: PublishedComponent.self, for: entity) === second) + } + + // MARK: - A reader on another thread + + /// Runs `change` on this thread, `rounds` times, while a reader thread reads the + /// component of every entity over and over through one copy of the scene, taken + /// before the first change. The reader stops after the last change. + private func readThroughOneCopy( + of entities: [EntityID], rounds: Int, while change: (Int) -> Void + ) -> (reads: Int, unexpected: Int) { + let copyTaken = DispatchSemaphore(value: 0) + let state = ReaderState() + let finished = expectation(description: "the reader stopped") + + Thread.detachNewThread { + let copy = scene + copyTaken.signal() + var reads = 0 + var unexpected = 0 + while !state.isStopped { + for entity in entities { + guard let component = copy.get(component: PublishedComponent.self, for: entity) else { continue } + reads += 1 + if component.value != PublishedComponent.initial { + unexpected += 1 + } + } + } + state.record(reads: reads, unexpected: unexpected) + finished.fulfill() + } + + copyTaken.wait() + for round in 0 ..< rounds { + change(round) + } + state.stop() + wait(for: [finished], timeout: 60) + return state.totals + } + + func testAReaderHoldingACopyReadsWhileItsEntitiesAreGivenTheComponentAgain() { + // What a loader does to the transform components: the entity has the component, + // and it is registered once more. + let entities = (0 ..< 16).map { _ in createEntity() } + for entity in entities { + registerComponent(entityId: entity, componentType: PublishedComponent.self) + } + let rounds = 200 + + let totals = readThroughOneCopy(of: entities, rounds: rounds) { _ in + for entity in entities { + registerComponent(entityId: entity, componentType: PublishedComponent.self) + } + } + + XCTAssertGreaterThan(totals.reads, 0) + XCTAssertEqual(totals.unexpected, 0) + } + + func testAReaderHoldingACopyReadsWhileTheComponentIsRemovedAndGivenAgain() { + // What replacing an entity's mesh does to its render component: the copy still + // says the entity has the component, and its slot gets a new one. + let entities = (0 ..< 16).map { _ in createEntity() } + for entity in entities { + registerComponent(entityId: entity, componentType: PublishedComponent.self) + } + let rounds = 200 + + let totals = readThroughOneCopy(of: entities, rounds: rounds) { _ in + for entity in entities { + scene.remove(component: PublishedComponent.self, from: entity) + registerComponent(entityId: entity, componentType: PublishedComponent.self) + } + } + + XCTAssertGreaterThan(totals.reads, 0) + XCTAssertEqual(totals.unexpected, 0) + } + + func testAReaderHoldingACopyReadsWhileItsEntitiesAreDestroyedAndTheirIndicesReused() { + // The copy still holds the entities that were destroyed, and new entities take + // their indices and their slots. + var entities = (0 ..< 16).map { _ in createEntity() } + for entity in entities { + registerComponent(entityId: entity, componentType: PublishedComponent.self) + } + let readerEntities = entities + let rounds = 200 + + let totals = readThroughOneCopy(of: readerEntities, rounds: rounds) { _ in + for entity in entities { + destroyEntity(entityId: entity) + } + finalizePendingDestroys() + entities = entities.map { _ in createEntity() } + for entity in entities { + registerComponent(entityId: entity, componentType: PublishedComponent.self) + } + } + + XCTAssertEqual( + Set(entities.map { getEntityIndex($0) }), Set(readerEntities.map { getEntityIndex($0) }), + "the new entities took the indices of the destroyed ones" + ) + XCTAssertGreaterThan(totals.reads, 0) + XCTAssertEqual(totals.unexpected, 0) + } +}