Skip to content
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

Use Meyers singletons #1222

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
+46 −106
Open

Use Meyers singletons #1222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/sst/core/elemLoader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

loader->load();
}
}
Expand Down
43 changes: 8 additions & 35 deletions src/sst/core/eli/elementbuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,7 @@ class BuilderLibrary

BuilderLibrary(const std::string& name) : name_(name) {}

BaseBuilder* getBuilder(const std::string& name)
{
auto iter = factories_.find(name);
if ( iter == factories_.end() ) { return nullptr; }
else {
return iter->second;
}
}
BaseBuilder* getBuilder(const std::string& name) { return factories_[name]; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


const std::map<std::string, BaseBuilder*>& getMap() const { return factories_; }

Expand Down Expand Up @@ -88,29 +81,16 @@ class BuilderLibraryDatabase

static Library* getLibrary(const std::string& name)
{
if ( !libraries ) { libraries = new std::map<std::string, Library*>; }
auto iter = libraries->find(name);
if ( iter == libraries->end() ) {
auto* info = new Library(name);
(*libraries)[name] = info;
return info;
}
else {
return iter->second;
}
static Map libraries; // Database
auto& lib = libraries[name];
if ( !lib ) lib = new Library(name);
return lib;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

template <class NewBase>
using ChangeBase = BuilderLibraryDatabase<NewBase, CtorArgs...>;

private:
// Database - needs to be a pointer for static init order
static Map* libraries;
};

template <class Base, class... CtorArgs>
typename BuilderLibraryDatabase<Base, CtorArgs...>::Map* BuilderLibraryDatabase<Base, CtorArgs...>::libraries = nullptr;

template <class Base, class Builder, class... CtorArgs>
struct BuilderLoader : public LibraryLoader
{
Expand Down Expand Up @@ -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>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

};

template <class Base, class T>
const bool InstantiateBuilder<Base, T>::loaded = Base::Ctor::template add<T>();

template <class Base, class T, class Enable = void>
struct Allocator
{
Expand All @@ -169,14 +146,10 @@ struct CachedAllocator
template <class... Args>
Base* operator()(Args&&... ctorArgs)
{
if ( !cached_ ) { cached_ = new T(std::forward<Args>(ctorArgs)...); }
return cached_;
static T cached(std::forward<Args>(ctorArgs)...);
return &cached;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;

}

static Base* cached_;
};
template <class Base, class T>
Base* CachedAllocator<Base, T>::cached_ = nullptr;

template <class T, class Base, class... Args>
struct DerivedBuilder : public Builder<Base, Args...>
Expand Down
56 changes: 21 additions & 35 deletions src/sst/core/eli/elementinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,32 +59,24 @@ class DataBase
public:
static T* get(const std::string& elemlib, const std::string& elem)
{
if ( !infos_ ) return nullptr;

auto libiter = infos_->find(elemlib);
if ( libiter != infos_->end() ) {
auto libiter = infos().find(elemlib);
if ( libiter != infos().end() ) {
auto& submap = libiter->second;
auto elemiter = submap.find(elem);
if ( elemiter != submap.end() ) { return elemiter->second; }
}
return nullptr;
}

static void add(const std::string& elemlib, const std::string& elem, T* info)
{
if ( !infos_ ) {
infos_ = std::unique_ptr<std::map<std::string, std::map<std::string, T*>>>(
new std::map<std::string, std::map<std::string, T*>>);
}

(*infos_)[elemlib][elem] = info;
}
static void add(const std::string& elemlib, const std::string& elem, T* info) { infos()[elemlib][elem] = info; }

private:
static std::unique_ptr<std::map<std::string, std::map<std::string, T*>>> infos_;
static auto& infos()
{
static std::map<std::string, std::map<std::string, T*>> infos_;
return infos_;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another Meyers singleton.

};
template <class T>
std::unique_ptr<std::map<std::string, std::map<std::string, T*>>> DataBase<T>::infos_;

template <class Policy, class... Policies>
class BuilderInfoImpl : public Policy, public BuilderInfoImpl<Policies...>
Expand Down Expand Up @@ -166,7 +158,7 @@ class InfoLibrary
return count;
}

const std::map<std::string, BaseInfo*>& getMap() const { return infos_; }
const auto& getMap() const { return infos_; }

void readdInfo(const std::string& name, BaseInfo* info)
{
Expand Down Expand Up @@ -208,36 +200,30 @@ class InfoLibraryDatabase

std::vector<std::string> ret;
// First iterate over libraries
for ( auto x : (*libraries) ) {
for ( auto& y : x.second->getMap() ) {
ret.push_back(x.first + "." + y.first);
for ( auto& [name, lib] : libraries() ) {
for ( auto& [elemlib, info] : lib->getMap() ) {
ret.push_back(name + "." + elemlib);
}
}
return ret;
}

static Library* getLibrary(const std::string& name)
{
if ( !libraries ) { libraries = new Map; }
auto iter = libraries->find(name);
if ( iter == libraries->end() ) {
auto* info = new Library(name);
(*libraries)[name] = info;
return info;
}
else {
return iter->second;
}
auto& lib = libraries()[name];
if ( !lib ) lib = new Library(name);
return lib;
}

private:
// Database - needs to be a pointer for static init order
static Map* libraries;
// Database
static Map& libraries()
{
static Map libs;
return libs;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again.

};

template <class Base>
typename InfoLibraryDatabase<Base>::Map* InfoLibraryDatabase<Base>::libraries = nullptr;

template <class Base, class Info>
struct InfoLoader : public LibraryLoader
{
Expand Down
26 changes: 5 additions & 21 deletions src/sst/core/eli/elibase.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,32 +20,16 @@ namespace SST {
**************************************************************************/
namespace ELI {

std::unique_ptr<LoadedLibraries::LibraryMap> LoadedLibraries::loaders_ {};

bool
LoadedLibraries::addLoader(
const std::string& lib, const std::string& name, const std::string& alias, LibraryLoader* loader)
{
if ( !loaders_ ) { loaders_ = std::unique_ptr<LibraryMap>(new LibraryMap); }
(*loaders_)[lib][name].push_back(loader);
if ( !alias.empty() ) (*loaders_)[lib][alias].push_back(loader);
return true;
}
std::shared_ptr<LibraryLoader> shared_loader(loader);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


const LoadedLibraries::LibraryMap&
LoadedLibraries::getLoaders()
{
if ( !loaders_ ) { loaders_ = std::unique_ptr<LibraryMap>(new LibraryMap); }
return *loaders_;
}

bool
LoadedLibraries::isLoaded(const std::string& name)
{
if ( loaders_ ) { return loaders_->find(name) != loaders_->end(); }
else {
return false; // nothing loaded yet
}
auto& library = getLoaders()[lib];
if ( !alias.empty() && alias != name ) library[alias].push_back(shared_loader);
library[name].push_back(std::move(shared_loader));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return true;
}

} // namespace ELI
Expand Down
25 changes: 11 additions & 14 deletions src/sst/core/eli/elibase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,9 @@

#include <algorithm>
#include <cstring>
#include <functional>
#include <list>
#include <deque>
#include <map>
#include <memory>
#include <set>
#include <string>
#include <type_traits>
#include <vector>
Expand Down Expand Up @@ -124,28 +122,27 @@ combineEliInfo(std::vector<T>& base, const std::vector<T>& add)

struct LibraryLoader
{
virtual void load() = 0;
virtual ~LibraryLoader() {}
virtual void load() = 0;
virtual ~LibraryLoader() = default;
};

class LoadedLibraries
{
public:
using InfoMap = std::map<std::string, std::list<LibraryLoader*>>;
using InfoMap = std::map<std::string, std::deque<std::shared_ptr<LibraryLoader>>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

using LibraryMap = std::map<std::string, InfoMap>;

static bool isLoaded(const std::string& name);
static bool isLoaded(const std::string& name) { return getLoaders().count(name) != 0; }

/**
@return A boolean indicated successfully added
*/
// @return A boolean indicated successfully added
static bool
addLoader(const std::string& lib, const std::string& name, const std::string& alias, LibraryLoader* loader);

static const LibraryMap& getLoaders();

private:
static std::unique_ptr<LibraryMap> loaders_;
static LibraryMap& getLoaders()
{
static LibraryMap loaders;
return loaders;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meyers singleton.

};

// Template used to get aliases. Needed because the ELI_getAlias()
Expand Down
Loading