Skip to content

Use std::atomic to implement OpenThreads::Atomic and AtomicPtr - #48

Open
elsid wants to merge 2 commits into
OpenMW:3.6from
elsid:3.6_std_atomic
Open

Use std::atomic to implement OpenThreads::Atomic and AtomicPtr#48
elsid wants to merge 2 commits into
OpenMW:3.6from
elsid:3.6_std_atomic

Conversation

@elsid

@elsid elsid commented Aug 16, 2026

Copy link
Copy Markdown

Current implementation of atomic load is suboptimal that is visible on ARM: https://godbolt.org/z/r4E7srWcE

This also triggers thread sanitizer warnings like:

WARNING: ThreadSanitizer: data race (pid=42478)
  Atomic write of size 4 at 0x7254001afb90 by main thread:
    #0 OpenThreads::Atomic::operator++()      OpenThreads/Atomic:110
    #1 osg::Referenced::ref()                 osg/Referenced:164
    #2 osg::ref_ptr<osg::Drawable>::ref_ptr   osg/ref_ptr:37
    #3 osgUtil::RenderLeaf::RenderLeaf        osgUtil/RenderLeaf:41
    #4 osgUtil::CullVisitor::createOrReuseRenderLeaf
    #5 osgUtil::CullVisitor::addDrawableAndDepth
  Previous read of size 4 at 0x7254001afb90 by thread T112 (mutexes: write M0):
    #0 OpenThreads::Atomic::operator unsigned int() const   OpenThreads/Atomic:225
    #1 osg::Referenced::referenceCount()                    osg/Referenced:97
    #2 Resource::GenericObjectCache<Terrain::ChunkKey>::update  components/resource/objectcache.hpp:82
    ...
    #5 Resource::GenericObjectCache<Terrain::ChunkKey>::update  components/resource/objectcache.hpp:77

Remove all related code along with unnecessary mutex based reference counter.

Current implementation of atomic load is suboptimal that is visible on
ARM: https://godbolt.org/z/r4E7srWcE

This also triggers thread sanitizer warnings like:

```
WARNING: ThreadSanitizer: data race (pid=42478)
  Atomic write of size 4 at 0x7254001afb90 by main thread:
    #0 OpenThreads::Atomic::operator++()      OpenThreads/Atomic:110
    OpenMW#1 osg::Referenced::ref()                 osg/Referenced:164
    OpenMW#2 osg::ref_ptr<osg::Drawable>::ref_ptr   osg/ref_ptr:37
    OpenMW#3 osgUtil::RenderLeaf::RenderLeaf        osgUtil/RenderLeaf:41
    OpenMW#4 osgUtil::CullVisitor::createOrReuseRenderLeaf
    OpenMW#5 osgUtil::CullVisitor::addDrawableAndDepth
  Previous read of size 4 at 0x7254001afb90 by thread T112 (mutexes: write M0):
    #0 OpenThreads::Atomic::operator unsigned int() const   OpenThreads/Atomic:225
    OpenMW#1 osg::Referenced::referenceCount()                    osg/Referenced:97
    OpenMW#2 Resource::GenericObjectCache<Terrain::ChunkKey>::update  components/resource/objectcache.hpp:82
    ...
    OpenMW#5 Resource::GenericObjectCache<Terrain::ChunkKey>::update  components/resource/objectcache.hpp:77
```

Remove all related code along with unnecessary mutex based reference
counter.
@AnyOldName3

Copy link
Copy Markdown
Member

This has a big overlap with #44, but unlike that PR, will only work when compiled as C++11 or later, and doesn't add anything to enforce that it is. We know there are distros packaging this fork rather than upstream OSG, as we've merged build fixes, so they'd have to carry those patches anyway even if they were applying them to an upstream branch, and they're not going to be passing in the flags for C++11 in an outer build system like we do.

As I think I said on the other PR, I'm happy for our fork to use std::atomic when OSG's compiled as C++11 or later (as long as the codegen's as good), but with a fallback to the old implementation when it isn't. If we want to change OSGoS to require C++11 or later, though, that's something that requires a decision, and then there are a lot of places where things could be improved by replacing their current implementation with something more modern.

@elsid

elsid commented Aug 16, 2026

Copy link
Copy Markdown
Author

C++11 is already a requirement due to rvalue references used for ref_ptr. I don't see a value of keeping old implementation.

@AnyOldName3

Copy link
Copy Markdown
Member

19c1df1 should not have been merged into this repository. It's not the only commit from that PR that's caused problems.

It does suggest that we can get away with bumping the minimum requirement to C++11, but we don't even know that for sure as it's not out of the question that someone's carrying a tiny patch to get rid of the move constructor. I don't think that's especially likely, though, as there's literally nothing else in the repo enforcing the language standard, so it just uses whatever the compiler uses by default unless something external forces something else, and anything still updating OSG is probably updating its compiler, and defaults to something way newer.

That means that I'm going to say we can YOLO it and start requiring C++11 intentionally in this repo but only as long as we add something to the CMake to actually ensure the necessary flags are set. target_compile_features(target_name_here PUBLIC cxx_std_11) on the main osg library and OpenThreads should work well, as it'll keep the old don't downgrade the language standard if it's already something higher than our minimum behaviour while ensuring that it errors out if a too-old version is forced and adds the right flag if using a compiler that defaults to an older version.

@elsid

elsid commented Aug 16, 2026

Copy link
Copy Markdown
Author

Anyone can have a patch removing C++11 requirement, restoring old atomic implementation etc. We should not care about this.

@AnyOldName3

Copy link
Copy Markdown
Member

At the moment, this repo is malformed, and can be fixed either by reverting 19c1df1 or by making the CMake changes I described above. I'm not merging anything that makes it more malformed, so if you want this to be mergeable, I suggest including those CMake changes in this PR.

* osg::ref_ptr uses rvalue reference.
* OpenThreads depends on <atomic>.
@elsid

elsid commented Aug 17, 2026

Copy link
Copy Markdown
Author

Added C++11 as requirement.

@Duron27

Duron27 commented Aug 17, 2026

Copy link
Copy Markdown

@elsid

elsid commented Aug 18, 2026

Copy link
Copy Markdown
Author

If we care about correctness of CMake to have proper builds should we bump OPENSCENEGRAPH_SOVERSION because ABI changed since last time it was bumped?

@Duron27

Duron27 commented Aug 18, 2026

Copy link
Copy Markdown

If I'm going to do thread sanitization checks based on if those patches are required I should probably based it on this mr and not #44 correct?

@AnyOldName3

Copy link
Copy Markdown
Member

If we care about correctness of CMake to have proper builds should we bump OPENSCENEGRAPH_SOVERSION because ABI changed since last time it was bumped?

I'm not sure. There have been several ABI breaks upstream since it was last bumped (e.g. several new members and functions in GLExtensions), so we're not really making anything worse by ignoring it, and I'm not sure anyone gains anything by us being diligent about it. You can't really have multiple side-by-side installs of different versions OSG where the loader can see them as everything except the plugins ends up with the same filenames, so the only thing it would help with would be third-party plugins that aren't updated in lockstep with OSG itself, and I don't think that's a realistic usecase. OpenMW doesn't do that, and distros packaging this fork of OSG are making distro packages, so will rebuild any plugins whether or not they're in this repo. Unless I'm missing something, I don't see a path for old plugins to be on the same machine as new OSG.

@AnyOldName3

Copy link
Copy Markdown
Member

If I'm going to do thread sanitization checks based on if those patches are required I should probably based it on this mr and not #44 correct?

Yeah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants