Skip to content

Fix for singleton in Profiler #694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: gz-common5
Choose a base branch
from
Open

Fix for singleton in Profiler #694

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3ee6cf6
Fix for singleton in Profiler
ntfshard Jul 10, 2025
9fdf1c8
Proposal check
ntfshard Aug 8, 2025
82e9d62
Back to static
ntfshard Aug 8, 2025
32f518c
Merge branch 'gz-common5' into singleton_profiler_fix
ntfshard Aug 21, 2025
8874523
Merge branch 'gz-common5' into singleton_profiler_fix
ntfshard Aug 29, 2025
691b79b
Merge branch 'gz-common5' into singleton_profiler_fix
ntfshard Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/gz/common/SingletonT.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ namespace gz
/// \brief Creates and returns a reference to the unique (static) instance
private: static T &GetInstance()
{
// Caution: object `t` will be duplicated in each translation unit
static T t;
return static_cast<T &>(t);
}
Expand Down
3 changes: 3 additions & 0 deletions profiler/include/gz/common/Profiler.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ namespace gz
/// \brief Detect if profiler is enabled and has an implementation
public: bool Valid() const;

/// \brief Get an instance of the singleton
public: static Profiler *Instance();

/// \brief Pointer to the profiler implementation
private: ProfilerImpl *impl;

Expand Down
7 changes: 7 additions & 0 deletions profiler/src/Profiler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,10 @@ bool Profiler::SetImplementation(std::unique_ptr<ProfilerImpl> _impl)
gzdbg << "Gazebo profiling with: " << this->impl->Name() << std::endl;
return true;
}

//////////////////////////////////////////////////
Profiler *Profiler::Instance()
{
static Profiler profiler;
return &profiler;
Comment on lines +122 to +123
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static Profiler profiler;
return &profiler;
return SingletonT<Profiler>::Instance();

I think doing this might maintain ABI compatibility. Can you give it a try?

Copy link

@mderbaso-deepx mderbaso-deepx Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIS report complaining on code, not on ABI (which I mentioned in comment above).
I believe ABI checker checking for disappearing some symbols, but here we have new symbol which should be fine

I built both versions (with and w/o change) and checked difference in symbols in libgz-common5-profiler.so.5.7.1 library.
And difference is couple o new symbols (some of them are public(capital letter), some of them private)

# diff clear_build/lib/out.txt branch/lib/out.txt 
51a52
> b _ZGVZN2gz6common8Profiler8InstanceEvE8profiler
72a74,75
> T _ZN2gz6common8Profiler8InstanceEv
> t _ZN2gz6common8Profiler8InstanceEv.cold
116a120
> b _ZZN2gz6common8Profiler8InstanceEvE8profiler
187a192,194
> U __cxa_guard_abort@CXXABI_1.3
> U __cxa_guard_acquire@CXXABI_1.3
> U __cxa_guard_release@CXXABI_1.3

command to get symbols w/o addresses: gz-common/branch/lib# nm libgz-common5-profiler.so.5.7.1 | cut -c18- > out.txt

I will try proposed code but I have big doubts about CI result change in this case

From source perspective it's big change, before symbol was inlined in every translation unit and now implementation hidden inside

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code clearly removes the SingletonT<Profiler>::__ct [C1] ( SingletonT<Profiler>const& p1 ) symbol because it no longer uses creates a SingletonT<Profiler> object, so in that sense the ABI checker is correct. However, like you said since those symbols are inlined, it's probably not a problem. But for added safety, I'd prefer if we can get our ABI checker ✅ .

Copy link
Contributor Author

@ntfshard ntfshard Aug 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting mangling, first time see this way, which platform is it? I guess it's a copy constructor for class template Singleton. Which I guess will be or private symbol, or inlined.

I pushed proposed version and got same result https://build.osrfoundation.org/job/gz_common-abichecker-any_to_any-ubuntu-jammy-amd64/133/:

Binary compatibility: 100%
Source compatibility: 99.1%

I guess it's all about hiding Instance member function from Singleton parent.

If you OK, I'd prefer to proceed with static variable. As I said before, I'd really suggest to get rid of Singleton class due to it's just making more confusion and technically trying to replace 2 line function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Thanks for trying. I was hoping we would get a green CI. But since that's not going to happen, I would suggest a slightly different approach of using the gz::utils::NeverDestroyed class https://github.com/gazebosim/gz-sim/blob/f4a3b61b333ae822d5afbd2f8cae43d9ad361004/src/ComponentFactory.cc#L22-L26

This class is designed to work well with address sanitizers and avoids the static initialization order fiasco.

Also, I totally agree that we should remove the SingletonT class. If you are interested in helping out with removing the SingletonT class from our code base, please go right ahead :) (target main branches please).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About static initialization order fiasco, it refer to objects in global scope, which initialization happens before main(). Here we have a Meyers Singleton which provide a lazy initialization on 1st access. Also it happens thread-safe from C++11. So it's not this case

https://www.modernescpp.com/index.php/c-20-static-initialization-order-fiasco/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And about NeverDestroyed (very short version). IMHO it's a very dirty hack.

  1. Valgrind will see indirect memory leak if object inside it allocates memory.
  2. AFAIU it was invented for cases of library loading with dlopen and unloaded before termination. In this case code of destructor of static object is unloaded and attempt to call it will lead to crash on termination (I saw similar case with destructor of exception on plugin loading mechanism in one of project where I worked)
  3. I guess it can be treated as UB. Can deep dive in standard to find more accurate interpretation, but it will take a lot of time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I totally agree that we should remove the SingletonT class. If you are interested in helping out with removing the SingletonT class from our code base, please go right ahead :) (target main branches please).

I guess I can, just want to close other PRs which I opened (2 in this repo, 1 in gz-sim)

}
Loading