Uh oh!
There was an error while loading. Please reload this page.
Make the atom id counter atomic and the parameters singleton safe - #177
Merged
Conversation
…on safe Two races that the GIL currently hides. ForeFireAtom::instanceNRCount was a plain long incremented with ++ to give every atom its id. Concurrent construction could hand the same id to two objects. It is now std::atomic<long> with a relaxed fetch_add, which is all the ordering distinct ids need. SimulationParameters::GetInstance used the textbook unsafe lazy singleton, so two threads could both see null and both construct, then diverge with different parameter objects. It is now a function-local static, whose initialisation C++11 guarantees runs once. The unused private member is gone.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Two pieces of shared state from the table in #175, both fixed without adding any runtime machinery. Eight lines of substance.
The id counter
Every
ForeFireAtomtakes its id frominstanceNRCount++(ForeFireAtom.h:106), a plainlong. Concurrent construction can hand the same id to two objects. It becomesstd::atomic<long>withfetch_add(1, std::memory_order_relaxed)— relaxed because ids only have to be distinct, not ordered against anything else, so this compiles to the samelock xadda compiler would emit anyway and costs nothing measurable on the single-threaded path.The parameters singleton
GetInstancewas the textbook unsafe lazy singleton:Two threads can both see null, both construct, and walk away with different parameter objects. It becomes a function-local static, whose once-only initialisation C++11 already guarantees:
Still never deleted, matching what it did before — the parameters live for the whole process. The now-unused private
instancemember goes with it.What this does not do
It does not make ForeFire safe to use from several threads. I measured that: with this change and the other small fixes applied, the eight-thread stress test in #176 still crashes on all five runs. Safety needs the state refactor described as step 5 of #175, which is a real API change and is not proposed here.
What this does is remove two anti-patterns that are wrong on their own terms. The unsafe lazy singleton is a known bug shape whether or not anyone threads this code, and a shared mutable counter behind an
++is the same. Neither change adds a lock, a flag, or a branch to any hot path.Verification
runff: KML and NetCDF both match the references within tolerance. Ids are still allocated in the same order single-threaded, so the outputs are unchanged.Related: #175 (the shared-state issue), #176 (the stress test that measures it).
This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.