Repository navigation
Conversation
… 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 704cb96)
Member
Author
|
Closed without merging: the fork's develop already has this change through the sync with upstream (#70), which brought untoldengine#1302. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The fix of untoldengine#1302 on the fork's
develop.Scene.assignwrote a new component into its slot with a plain store andScene.getread the slot with a plain load, outside the scene lock, so a reader whose copy of the scene says the slot is in use could load the new pointer with nothing ordering that load after the object's initialization. A component that is an object now goes into its slot with an atomic compare and exchange and is read with an acquire load. The cause, the hardware test, the measurements and the options that were weighed are in the upstream PR.One commit
The commit of untoldengine#1302, cherry-picked with
-x. The fork already releases components (#68, upstream untoldengine#1300), and the two changes meet inScene.assign,Scene.getandComponentPool. Resolved this way:Scene.assign: the component the entity had goes to the quarantine first, as before, and the new one is then stored atomically throughComponentSlot.ComponentPool: whether a slot holds one reference to an object is kept in the chunk list's header, beside the quarantine, so the pool stays one reference.slot(at:)hands out the slot's address with it, andcomponent(at:as:)reads throughComponentSlot.Scene.get: asks the pool for the slot and lets the pool go before it reads. A pool kept across the atomic load is retained and released on every call (4 ns); the scene the caller holds keeps the pool's storage, so what the quarantine holds back stays held back.This is the form the upstream branch takes when it is rebased over untoldengine#1300, or untoldengine#1300 over it.
Verified on this branch
swift test --filter UntoldEngineTests: 1,546 run. The only failure isExternalRenderExtensionPackageTests, which fails locally whenever the checkout folder is not namedUntoldEngine; it passes from one that is.UntoldComponentKitTests: 59 passed.developeach reports the data race.scripts/strict-concurrency-guardrails.sh); no new SwiftFormat findings in the changed files.develop(aba6ce4) built against this branch: 1,071 tests passed.scene.get57.2 ns against 58.2 ns (release build, medians of 7 alternating runs, measured on the same merge over upstreamdevelop).