Efficient C++ Resource Manager

This is my final attempt of making an efficient ResourceManager class, which takes care of allocating OpenGL objects (like Shader, Textures, Meshes, ...). It stores each resource in a unique_ptr and then distributes const pointers of that type, which I call "Observant pointers." They cannot be deleted because resource allocation is only done via the ResourceManager class.

ResourceManager.h:

```
#pragma once
#include
#include
#include
#include "../render/Shader.h"
#include "../render/Mesh.h"
#include "../render/Texture.h"

namespace Spiky
{

class ResourceManager
{
public:

using ShaderRepo = std::unordered_map>;
using MeshRepo = std::unordered_map>;
using TextureRepo = std::unordered_map>;

//Shader
static const CShader LoadShader(const char ID, const char vs, const char fs)
{
shaderObjects.insert(std::pair>(ID, std::make_unique(
(shaderRootDir + std::string(vs)).c_str(),
(shaderRootDir + std::string(fs)).c_str())));
return (shaderObjects.at(ID).get());
}

static const CShader LoadShader(const char ID, const char vs, const char fs, const char* gs)
{
shaderObjects.insert(std::pair>(ID, std::make_unique(
(shaderRootDir + std::string(vs)).c_str(),
(shaderRootDir + std::string(fs)).c_str(),
(shaderRootDir + std::string(gs)).c_str())));
return (shaderObjects.at(ID).get());
}

static CShader GetShader(const char ID)
{
return (shaderObjects.at(ID).get());
}

-
Well, your choice of backing-type for your collections of shaders, meshes and textures has a few interesting results:

Your keys are pointers, not strings, and those pointers are compared/hashed.

While the compiler may freely merge string-literals, it need not.

Every self-respecting compiler in practice does it inside a single translation-unit, but outside that it fast becomes less common to never done.

There are two ways to resolve that, depending on your goals:

-
Change the key-type to std::string. That's the easiest and most versatile solution, but it has the disadvantage of a potential additional allocation.

-
Change the hash and comparison-algorithm used by the collection to treat the pointers as representing the string-value they point to. The disadvantage is that you must make sure the strings don't change until the manager releases the entry.

-
Looking at Load@, while those functions are all short, there are loads of things to optimize:

• There's std::make_pair(...) when needed.

• One can directly concatenate a c-style string and a std::string, no need for an additional temporary object.

• You can use std::emplace to avoid uselessly creating a std::pair.

• insert and emplace already return an iterator to the inserted element, or whatever prevented insertion, and a bool to indicate what happened.

• If you allow for the element already being in the collection, recreating it just to destroy it is a huge waste.

template
static const CShader* LoadShader(T&& ID, const char* vs, const char* fs) {
    auto res = shaderObjects.emplace(std::forward(ID));
    if(res.second)
        try {
            res.first->second = std::make_unique((shaderRootDir + vs).c_str(),
                (shaderRootDir + fs).c_str());
        } catch(...) {
            shaderObjects.erase(res.first);
            throw;
        }
    return res.first->second.get();
}

As an aside, I have no idea why you used typedefs for your collections...

As far as I can see, they are useless.

Also, you know you could use struct?

Things get far more efficient if you have a C++17 standard-library (and thus access to try_emplace(...):

return &shaderObjects.try_emplace(ID, (shaderRootDir + vs).c_str(),
     (shaderRootDir + fs).c_str()).first->second;

If you don't, but can arrange for CMesh, CTexture and CShader to be efficiently default-constructible and either swappable or move-assignable, that is also sufficient.

In a pinch, if your default-constructor is lightweight and cannot throw, you could even use explicit destructor-call and placement-new. That isn't nice, but it's efficient enough and it works:

template
static const CShader* LoadShader(T&& ID, const char* vs, const char* fs) {
    auto res = shaderObjects.emplace(std::forward(ID));
    if(res.second)
        try {
            res.first->second.~CShader(); // noexcept
            new((void*)&res.first->second) CShader((shaderRootDir + vs).c_str(),
                (shaderRootDir + fs).c_str());
        } catch(...) {
            new((void*)&res.first->second) CShader(); // efficient and noexcept
            shaderObjects.erase(res.first);
            throw;
        }
    return &res.first->second;
}

In that case, they should be directly in the map, as elements in a std::unordered_map stay exactly where they are until they are erased, meaning we have one dynamic allocation less.

StackExchange Code Review Q#109459, answer score: 5