2
\$\begingroup\$

Currently I'm working on a way to handle resources in my game engine. It is my first iteration and I would like to expand on it in the future like asynchronous loading of resources. I am looking for some feedback and advice in order to improve it.

The API consists of the following classes.

identifier
Is responsible for creating unique identifiers from a string for usage in the cache(unordered_map). This is done at compile time by hashing a string to an unique int by using the fn1a algorithm

resource_cache
Responsibe for managing the cache, by loading/unloading resources and giving out handles to the resources.

resource_loader
An interface implemented by the user to load an specific resource.

resource_handle
Handle to an specific resource, could be containing a nullptr resource if the specific resource has not been loaded.

Questions
I am looking for some feedback and advice on the following topics.

  • Any potential memory leaks?
  • Is the API clear and concise

There is also one potential issue with dangling pointers I have yet to find an answer to. This happens when a resource is loaded and then erased, all handles given out in between will have an dangling pointer that could result in potential issues for users. I could use shared pointers but I would like to make sure a resource is unloaded once unload is called instead of keeping the resource intact till all handles are destroyed. Reference counting might sound as a nice feature but when managing large resources I would prefer to be able to unload a resource at any specific time and just reset the resource to nullptr. I might implement a specific cache for smaller resources that will implement reference counting in the future though.

Code
identifier.h

#pragma once

#include <utils/string_hash.h>

#include <cstring>

namespace engine
{
    namespace resources
    {
        struct identifier
        {
            using string_hash = engine_core::utils::fnv1a<uint32_t>;

            constexpr identifier(const char* identifier)
                : hash(string_hash::hash(identifier)), data(identifier)
            {
            }

            constexpr bool operator==(const identifier& rhs) const
            {
                return hash == rhs.hash;

            }
            constexpr bool operator==(const uint32_t key) const
            {
                return hash == key;
            }

            const bool operator==(const char* key) const
            {
                return strcmp(data, key) == 0;
            }

            const uint32_t hash;
            const char* data;
        };
    }
}

resource_loader.h

#pragma once

#include "identifier.h"

namespace engine
{
    namespace resources
    {
        template<typename T>
        class resource_cache;

        template<typename T, typename... Args>
        class resource_loader
        {
            friend class resource_cache<T>;
            virtual const T load(const identifier& key, Args... args) const = 0;
        };

    }
}

resource_handle.h

#pragma once 

#include <assert.h>

namespace engine
{
    namespace resources
    {
        template<typename T>
        class resource_handle
        {
        public:
            resource_handle() = default;
            resource_handle(T& resource)
                : m_resource(&resource)
            {
            }

            ~resource_handle() = default;

            [[nodiscard]] T& get()
            {
                assert(m_resource != nullptr);
                return *m_resource;
            }

            [[nodiscard]] const T& get() const
            {
                assert(m_resource != nullptr);
                return *m_resource;
            }

            [[nodiscard]] explicit operator const bool() const
            {
                return static_cast<const bool>(m_resource);
            }

        private:
            T* m_resource = nullptr;
        };
    }
}

resource_cache.h

#pragma once

#include "identifier.h"

#include <utility>
#include <unordered_map>
#include <cstdint>
#include <string>
#include <type_traits>

namespace engine
{
    namespace resources
    {
        template<typename T, typename... Args>
        struct resource_loader;

        template<typename T>
        struct resource_handle;

        template<typename T>
        class resource_cache
        {
        public:
            template<typename Loader, typename... Args>
            resource_handle<T> load(const identifier& identifier, Args&&... args)
            {
                static_assert(std::is_base_of<resource_loader<T, Args...>, Loader>::value, "Loader needs to be of type resource_loader");

                static Loader loader{};

                return resource_handle<T>((m_cache.emplace(identifier.hash, std::move(loader.load(identifier, std::forward<Args>(args)...))).first)->second);
            };

            void unload(const identifier& identifier)
            {
                typename std::unordered_map<uint32_t, T>::iterator it = m_cache.find(identifier.hash);

                m_cache.erase(identifier.hash);
            }

            void clear()
            {
                m_cache.clear();
            }

            [[nodiscard]] resource_handle<T> get_handle(const identifier& identifier)
            {
                typename std::unordered_map<uint32_t, T>::iterator it = m_cache.find(identifier.hash);

                if (it == m_cache.end())
                    return resource_handle<T>();

                return resource_handle<T>(it->second);
            }

            [[nodiscard]] uint64_t size()
            {
                return m_cache.size();
            }

        private:
            std::unordered_map<uint32_t, T> m_cache;
        };

    }
}

string_hash.h
Not really looking for a review on this but is needed in order to run the rest of the code.

#pragma once

#include <cstdint>

namespace engine_core
{
    namespace utils
    {
        template<typename T> struct  fnv_internal;
        template <typename T> struct fnv1;
        template <typename T> struct fnv1a;

        template <> struct fnv_internal<uint32_t>
        {
            constexpr static uint32_t default_offset_basis = 0x811C9DC5;
            constexpr static uint32_t prime = 0x01000193;
        };

        template <> struct fnv1<uint32_t> : public fnv_internal<uint32_t>
        {
            constexpr static inline uint32_t hash(char const* const aString, const uint32_t val = default_offset_basis)
            {
                return (aString[0] == '\0') ? val : hash(&aString[1], (val * prime) ^ uint32_t(aString[0]));
            }
        };

        template <> struct fnv1a<uint32_t> : public fnv_internal<uint32_t>
        {
            constexpr static inline uint32_t hash(char const* const aString, const uint32_t val = default_offset_basis)
            {
                return (aString[0] == '\0') ? val : hash(&aString[1], (val ^ uint32_t(aString[0])) * prime);
            }
        };
    }
}

Possible implementation

#include <iostream>

struct loader : public engine::resources::resource_loader<int, int>
{
    const int load(const engine::resources::identifier& key, int value) const override
    {
        return test;
    }
};

int main(int argc, char** argv)
{
    engine::resources::resource_cache<int> cache;

    auto handle = cache.load<loader>(engine::resources::identifier("test_identifier_is_mostly_an_file_path"), 2);
    
    if (handle)
        std::cout << handle.get() << "\n";

    cache.unload(engine::resources::identifier("test_identifier_is_mostly_an_file_path"));

    // possible issue is unloaded and points to dangling pointer
    if(handle)
        std::cout << handle.get() << "\n";
}
\$\endgroup\$
3
\$\begingroup\$

Enable compiler warnings and fix all warnings

My compiler produced some valid warnings about your code. In particular, if you return by value, you shouldn't make the return type const. It has no effect, other than to confuse you. So for example:

const bool operator==(const char *key) const

Should become:

bool operator==(const char *key) const

There are more instances of this. There is also a warning about the unused variable it in engine::resources::resource_cache::unload().

Why is there a namespace engine_core?

Why is there a separate namespace for the utility functions? Why not put them inside namespace engine?

Avoid repeating names

There's engine::resources::resource_cache. The word "resource" is repeated twice. It is unnecessary, you could just name it engine::resources::cache. The same goes for resource_handle and resource_loader.

Avoid unnecessary casts and use more auto

There are several places where you unnecessarily cast things, and there are places where you use long type names that can easily be deduced by the compiler. For example, in resource_cache::get_handle() you have:

typename std::unordered_map<uint32_t, T>::iterator it = m_cache.find(identifier.hash);

if (it == m_cache.end())
    return resource_handle<T>();

return resource_handle<T>(it->second);

This can be replaced with:

auto it = m_cache.find(identifier.hash);

if (it == m_cache.end())
    return {};

return it->second;

In the above, the return {} statement works because the compiler already knows the return type, and the empty initializer list {} matches the default constructor or the retun type. This also works for functions arguments, for example here:

auto handle = cache.load<loader>(engine::resources::identifier("test_identifier_is_mostly_an_file_path"), 2);

The compiler knows that the first argument has to be an engine::resources::identifier, so instead of explicitly creating such a type, you can also provide another argument that matches that of a constructor of engine::resources::identifier. Therefore, you can replace the above with:

auto handle = cache.load<loader>("test_identifier_is_mostly_an_file_path", 2);

Replace resource_handle with a bare or a smart pointer

class resource_haldle is a well written wrapper around a pointer, but it doesn't do anything else besides holding a pointer and giving access to the pointer. So it can just be replaced by a bare pointer instead. What would be better is to determine what kind of ownership semantics the pointer should have that is returned by resource_cache::load(). If, as you mentioned in the comments in main(), you are worried that a handle can turn into a dangling pointer, then you should consider using std::shared_ptr, or possibly use std::shared_ptr internally and have load() return a std::weak_ptr.

For example, in class resource_cache, do the following:

template<typename T>
class resource_cache
{
public:
    template<...>
    std::weak_ptr<T> load(...) {
        ...
        return m_cache.emplace(identifier.hash, loader.load(...)).first->second;
    }

private:
    std::unordered_map<uint32_t, std::shared_ptr<T>>;
};

This requires loader.load() to return a std::shared_ptr<T>. Then you can use it like so:

engine::resources::resource_cache<int> cache;

auto handle = cache.load<loader>("test_identifier_is_mostly_an_file_path", 2);
    
if (auto resource = handle.lock())
    std::cout << *resource << "\n";
else
    std::cerr << "resource no longer in cache\n";

Pass the loader as a function parameter

It is a bit weird that you have to pass the loader as a template parameter, and that the instantiation of the template resource_cache::load<>() will create a static variable of that type. This takes away control over the lifetime of the loader from the caller. This is why it would be better to pass the loader as a function parameter, like so:

template<typename Loader, typename... Args>
std::weak_ptr<T> load(Loader &loader, const identifier& identifier, Args&&... args) {
    static_assert(...);
    return m_cache.emplace(identifier.hash, loader.load(...)).first->second;
};

So you can call it like:

loader ldr;
auto handle = cache.load(ldr, "identifier", 2);

Or if you only need a one-shot loader:

auto handle = cache.load(loader{}, "identifier", 2);

This form also allows the loader to have a constructor that takes arguments, because it's no longer resource_cache's responsibility to create an instance of the loader.

Consider decoupling the loader from the cache

The above still looks like there is too tight a coupling between the loader and the resource cache. The only responsibility of the cache should be to store and retrieve items, nothing more. So instead ofload() and unload() member functions, I would just have insert() and erase() member functions, like so:

std::weak_ptr<T> insert(const identifier& identifier, std::shared_ptr<T> handle)
{
    return m_cache.emplace(identifier.hash, handle).first->second;
};

void erase(const identifier& identifier)
{
    m_cache.erase(identifier.hash);
}

And use it like so:

loader ldr;
auto handle = cache.insert("identifier", ldr.load(2));

Of course, now resource_cache is basically reduced to a simple wrapper around std::unordered_map, the only extra thing it does is to get the hash of the identifier. And even that is unnecessary:

Let std::unordered_map do the hashing for you

It's actually very silly to create a hash of an identifier and pass that as the key to a std::unordered_map, when the latter is already implemented as a hash table, and thus will hash the key itself! And it knows how to hash std::strings, so you can replace the cache with:

template<typename T>
class resource_cache
{
public:
    std::weak_ptr<T> insert(const char *identifier, std::shared_ptr<T> handle)
    {
        return m_cache.emplace(identifier, handle).first->second;
    };

    ...
private:
    std::unordered_map<std::string, std::shared_ptr<T>> m_cache;
};

This will also prevent any issues from cache collisions, which your class suffers from.

Make it an actual cache

You call it a cache, but it has none of the properties of a cache. Caches transparently load and unload resources for you, while your resource_cache has just manual functions to insert and remove items from a container. I would expect the following functionality:

  • A single get() function that either returns the value from the cache, or if it doesn't exist, will load it for me.
  • The cache will automatically remove (the oldest) items once a certain size is reached.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.