Use Meyers singletons #1222

leekillough · 2025-02-25T01:23:36Z

Use Meyers Singletons (static variables local to functions which are initialized on their first call and return a reference to it; avoids static initialization order fiasco) instead of pointers allocated on demand.
Use std::shared_ptr for LoadedLibraries loaded libraries so that libraries and aliases can share the same loader pointers without memory leaks. I was prompted to do this by valgrind:

==3714114== 80 bytes in 1 blocks are definitely lost in loss record 1,155 of 1,554
==3714114==    at 0x4859047: operator new(unsigned long) (vg_replace_malloc.c:472)
==3714114==    by 0x249295: addLoader (elementinfo.h:266)
==3714114==    by 0x249295: addInfo (elementinfo.h:184)
==3714114==    by 0x249295: addInfo (statbase.h:426)
==3714114==    by 0x249295: bool SST::Statistics::Statistic<void>::addDerivedInfo<SST::Statistics::NullStatistic<void> >(std::__cxx\
11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<cha\
r>, std::allocator<char> > const&) (statbase.h:426)
==3714114==    by 0x211E3F: add<SST::Statistics::NullStatistic<void> > (elementinfo.h:281)
==3714114==    by 0x211E3F: __static_initialization_and_destruction_0 (elementinfo.h:286)
==3714114==    by 0x211E3F: _GLOBAL__sub_I_main (main.cc:1281)
==3714114==    by 0x5319BBD: call_init (libc-start.c:145)
==3714114==    by 0x5319BBD: __libc_start_main@@GLIBC_2.34 (libc-start.c:347)
==3714114==    by 0x22E0A4: (below main) (in /home/lkillough/sstcore-14.1.0/libexec/sstsim.x)

==3714114== 80 bytes in 1 blocks are definitely lost in loss record 1,156 of 1,554
==3714114==    at 0x4859047: operator new(unsigned long) (vg_replace_malloc.c:472)
==3714114==    by 0x335C47: addLoader (elementinfo.h:266)
==3714114==    by 0x335C47: addInfo (elementinfo.h:184)
==3714114==    by 0x335C47: SST::RealTimeAction::addInfo(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<ch\
ar> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, SST::ELI::BuilderInfoImpl<SST\
::ELI::ProvidesDefaultInfo, void>*) (realtime.cc:90)
==3714114==    by 0x247986: addDerivedInfo<SST::ExitCleanRealTimeAction> (realtimeAction.h:32)
==3714114==    by 0x247986: bool SST::ELI::ElementsInfo<SST::RealTimeAction>::add<SST::ExitCleanRealTimeAction>() (elementinfo.h:281)
==3714114==    by 0x212037: __static_initialization_and_destruction_0 (elementinfo.h:286)
==3714114==    by 0x212037: _GLOBAL__sub_I_main (main.cc:1281)
==3714114==    by 0x5319BBD: call_init (libc-start.c:145)
==3714114==    by 0x5319BBD: __libc_start_main@@GLIBC_2.34 (libc-start.c:347)
==3714114==    by 0x22E0A4: (below main) (in /home/lkillough/sstcore-14.1.0/libexec/sstsim.x)
==3714114==

I can see other places where std::shared_ptr and std::unique_ptr should be used, but since the memory system is being overhauled, I won't touch the other places.

Use C++17 inline with static const class members so that they can be defined in-class and won't have multiple definition errors if defined in a header.

initialized on their first call and return a reference to it; avoids static initialization order fiasco) instead of pointers allocated on demand. Use std::shared_ptr for LoadedLibraries loaded libraries so that libraries and aliases can share the same loader pointers without memory leaks. Use C++17 "inline" with static const members so that they can be initialized in-class and won't have multiple definition errors if defined in a header.

github-actions · 2025-02-25T01:23:58Z

CLANG-FORMAT TEST - PASSED

github-actions · 2025-02-25T01:24:08Z

CMAKE-FORMAT TEST - PASSED

leekillough · 2025-02-25T01:24:27Z

src/sst/core/elemLoader.cc

@@ -230,7 +230,7 @@ ElemLoader::loadLibrary(const std::string& elemlib, std::ostream& err_os)
                // loop all the elements in the element lib
                for ( auto& elempair : libpair.second ) {
                    // loop all the loaders in the element
-                    for ( auto* loader : elempair.second ) {
+                    for ( auto& loader : elempair.second ) {


This allows loader to be either a regular pointer or a smart pointer. The previous code only allowed regular pointers.

leekillough · 2025-02-25T01:27:32Z

src/sst/core/eli/elementbuilder.h

-            return iter->second;
-        }
-    }
+    BaseBuilder* getBuilder(const std::string& name) { return factories_[name]; }


Since I do not see anywhere factories_ is used in such a way that adding nullptr entries would be a problem, this function can simply reduced to this subscript operator, which will create and return a nullptr entry if the name doesn't exist.

leekillough · 2025-02-25T01:31:50Z

src/sst/core/eli/elementbuilder.h

+        static Map libraries; // Database
+        auto&      lib = libraries[name];
+        if ( !lib ) lib = new Library(name);
+        return lib;


This is the Meyers singleton initializing the map on the first call to getLibrary().

It also uses a reference to the map entry and checks if it's nullptr, and if it is (such as if it did not exist before), sets it to allocated memory.

leekillough · 2025-02-25T01:35:28Z

src/sst/core/eli/elementbuilder.h

@@ -147,12 +127,9 @@ struct InstantiateBuilder
 {
    static bool isLoaded() { return loaded; }

-    static const bool loaded;
+    static inline const bool loaded = Base::Ctor::template add<T>();


inline can be used with static class data members in C++17, and they can be initialized in-class. This prevents multiple definitions which could occur in the old code if the variable was ODR-used (had its address taken), since it was defined in the header file.

static constexpr member variables are inline by default, but the initializer here is not constexpr (it possibly could be made so, if the operator() and its return value were constexpr).

leekillough · 2025-02-25T01:37:52Z

src/sst/core/eli/elementbuilder.h

-        if ( !cached_ ) { cached_ = new T(std::forward<Args>(ctorArgs)...); }
-        return cached_;
+        static T cached(std::forward<Args>(ctorArgs)...);
+        return &cached;


Another Meyers Singleton, in lieu of calling new if a pointer is nullptr (I did not see this CachedAllocator used anywhere, and I doubt any code calls delete on its returned address, so returning the address of a static is okay).

If it needs to be a pointer which can be deleted, it simply needs to be changed to:

static T* cached = new T(std::forward<Args>(ctorArgs)...); return cached;

leekillough · 2025-02-25T01:38:30Z

src/sst/core/eli/elementinfo.h

+    {
+        static std::map<std::string, std::map<std::string, T*>> infos_;
+        return infos_;
+    }


Another Meyers singleton.

leekillough · 2025-02-25T01:39:38Z

src/sst/core/eli/elementinfo.h

+    {
+        static Map libs;
+        return libs;
+    }


leekillough · 2025-02-25T01:40:16Z

src/sst/core/eli/elibase.cc

-    if ( !alias.empty() ) (*loaders_)[lib][alias].push_back(loader);
-    return true;
-}
+    std::shared_ptr<LibraryLoader> shared_loader(loader);


We create a std::shared_ptr which owns the loader pointer passed in.

leekillough · 2025-02-25T01:43:54Z

src/sst/core/eli/elibase.cc

-    }
+    auto& library = getLoaders()[lib];
+    if ( !alias.empty() && alias != name ) library[alias].push_back(shared_loader);
+    library[name].push_back(std::move(shared_loader));


On the last use of shared_loader we use std::move() so that it doesn't have the overhead of incrementing the reference count during the push_back() and then decrementing the reference count when shared_loader is destroyed. We move shared_loader to the new entry without changing reference counts, and its destruction is a no-op.

leekillough · 2025-02-25T01:49:28Z

src/sst/core/eli/elibase.h

 };

 class LoadedLibraries
 {
 public:
-    using InfoMap    = std::map<std::string, std::list<LibraryLoader*>>;
+    using InfoMap    = std::map<std::string, std::deque<std::shared_ptr<LibraryLoader>>>;


std::deque is faster than std::list and std::vector for adding an unknown number of items repeatedly to the end of a list which grows. std::list requires more dynamic memory allocations and is cache-unfriendly, and std::vector requires moving the entire vector's data to a new location if it has to increase its capacity. std::deque is also randomly accessible like std::vector, but it's not contiguous. I would look at all places std::vector and std::list are used and see if std::deque might be better.

leekillough · 2025-02-25T01:50:17Z

src/sst/core/eli/elibase.h

+    {
+        static LibraryMap loaders;
+        return loaders;
+    }


Meyers singleton.

sst-autotester · 2025-02-25T01:51:42Z

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CLANG-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Feb 25, 2025

github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CMAKE-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Feb 25, 2025

leekillough commented Feb 25, 2025

View reviewed changes

src/sst/core/eli/elementinfo.h

{

static Map libs;

return libs;

}

Copy link

Contributor Author

leekillough Feb 25, 2025

Choose a reason for hiding this comment

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

Again.

leekillough commented Feb 25, 2025

View reviewed changes

src/sst/core/eli/elibase.h

{

static LibraryMap loaders;

return loaders;

}

Copy link

Contributor Author

leekillough Feb 25, 2025

Choose a reason for hiding this comment

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

Meyers singleton.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Meyers singletons #1222

Use Meyers singletons #1222

leekillough commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

leekillough Feb 25, 2025

sst-autotester commented Feb 25, 2025

Use Meyers singletons #1222

Are you sure you want to change the base?

Use Meyers singletons #1222

Conversation

leekillough commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

github-actions bot commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sst-autotester commented Feb 25, 2025