Efficient C++ Resource Manager

Question

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 <memory>
#include <vector>
#include <unordered_map>
#include "../render/Shader.h"
#include "../render/Mesh.h"
#include "../render/Texture.h"

namespace Spiky
{

class ResourceManager
{
    public:

        using ShaderRepo = std::unordered_map<const char*, std::unique_ptr<CShader>>;
        using MeshRepo = std::unordered_map<const char*, std::unique_ptr<CMesh>>;
        using TextureRepo = std::unordered_map<const char*, std::unique_ptr<CTexture>>;

        //Shader
        static const CShader* LoadShader(const char* ID, const char* vs, const char* fs)
        {
            shaderObjects.insert(std::pair<const char*, std::unique_ptr<CShader>>(ID, std::make_unique<CShader>(
                                                                                      (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<const char*, std::unique_ptr<CShader>>(ID, std::make_unique<CShader>(
                                                                                      (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());
        }

        //Mesh
        static const CMesh* LoadMesh(const char* ID, Vertex* vertices, unsigned int numVertices, unsigned int* indeces, unsigned int numIndices)
        {
            meshObjects.insert(std::pair<const char*, std::unique_ptr<CMesh>>(ID, std::make_unique<CMesh>(
                                                                                  vertices,
                                                                                  numVertices,
                                                                                  indeces,
                                                                                  numIndices)));
            return (meshObjects.at(ID).get());
        }

        static const CMesh* LoadMesh(const char* ID, const char* fileName)
        {
            meshObjects.insert(std::pair<const char*, std::unique_ptr<CMesh>>(ID, std::make_unique<CMesh>(
                                                                                  (meshRootDir + std::string(fileName)).c_str())));
            return (meshObjects.at(ID).get());
        }

        //Texture
        static const CTexture* LoadTexture(const char* ID, const char* texturePath, GLenum texTarget = GL_TEXTURE_2D, GLfloat filter = GL_LINEAR,
            GLfloat pattern = GL_REPEAT, GLenum attachment = GL_NONE)
        {
            textureObjects.insert(std::pair<const char*, std::unique_ptr<CTexture>>(ID, std::unique_ptr<CTexture>(new CTexture(
                (textureRootDir + std::string(texturePath)).c_str(), texTarget, filter, pattern, attachment))));
            return (textureObjects.at(ID).get());
        }

        static const CTexture* LoadTexture(const char* ID, int width, int height, unsigned char* data = nullptr, GLenum texTarget = GL_TEXTURE_2D,
            GLfloat filter = GL_LINEAR, GLfloat pattern = GL_REPEAT, GLenum attachment = GL_NONE)
        {
            textureObjects.insert(std::pair<const char*, std::unique_ptr<CTexture>>(ID, std::unique_ptr<CTexture>(new CTexture(
                width, height, data, texTarget, filter, pattern, attachment))));
            return (textureObjects.at(ID).get());
        }

        static const CTexture* LoadTextureCustomPath(const char* ID, const char* texturePath, GLenum texTarget = GL_TEXTURE_2D, GLfloat filter = GL_LINEAR, GLfloat pattern = GL_REPEAT, GLenum attachment = GL_NONE)
        {
            textureObjects.insert(std::pair<const char*, std::unique_ptr<CTexture>>(ID, std::unique_ptr<CTexture>(new CTexture(
                texturePath, texTarget, filter, pattern, attachment))));
            return (textureObjects.at(ID).get());
        }

        static const* CTexture GetTexture(const char* ID)
        {
            return (textureObjects.at(ID).get());
        }



    private:
        static ShaderRepo shaderObjects;
        static MeshRepo meshObjects;
        static TextureRepo textureObjects;
        static std::string shaderRootDir;
        static std::string meshRootDir;
        static std::string textureRootDir;
    };
}

ResourceManager.cpp:

#include "../core/ResourceManager.h"

namespace Spiky
{
    ResourceManager::ShaderRepo ResourceManager::shaderObjects = ShaderRepo();
    ResourceManager::MeshRepo ResourceManager::meshObjects = MeshRepo();
    ResourceManager::TextureRepo ResourceManager::textureObjects = TextureRepo();
    std::string ResourceManager::shaderRootDir = std::string("assets/shaders/");
    std::string ResourceManager::meshRootDir = std::string("assets/models/");
    std::string ResourceManager::textureRootDir = std::string("assets/images/");
}

Answer 1

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

2. 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<class T>
   static const CShader* LoadShader(T&& ID, const char* vs, const char* fs) {
       auto res = shaderObjects.emplace(std::forward<T>(ID));
       if(res.second)
           try {
               res.first->second = std::make_unique<CShader>((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<class T>
static const CShader* LoadShader(T&& ID, const char* vs, const char* fs) {
    auto res = shaderObjects.emplace(std::forward<T>(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.