2
\$\begingroup\$

I wrote a basic C++ template to manage shared resources like textures and shaders in a 3D engine. The idea is that the cache itself holds weak references to the resources (through std::weak_ptr) and turns it into strong references (through std::shared_ptr) when the resource is fetched from the cache or constructed if it's not present. As soon as all strong references go out of scope, the given object gets deleted and removed from the cache by a custom deleter. Suggestions/improvements are welcome:

#ifndef PTRCACHE_H
#define PTRCACHE_H

#include <memory>
#include <map>
#include <functional>
#include <mutex>

namespace MyEngine
{
  template<typename Key, typename Val, typename ... Args>
  class PtrCache
  {
  public:
    PtrCache(const std::function<Val*(Args...)>& creator) : 
      _creator(creator),
      _mutex(std::make_unique<std::mutex>())
    {}
    PtrCache(const PtrCache& other) = delete;
    PtrCache& operator=(const PtrCache& other) = delete;
    PtrCache(PtrCache&& other) :
      _cache(std::move(other._cache)),
      _creator(std::move(other._creator)),
      _mutex(std::move(other._mutex))
    {}
    PtrCache& operator=(PtrCache&& other)
    {
      if (this != &other) {
        _cache = std::move(other._cache);
        _creator = std::move(other._creator);
        _mutex = std::move(other._mutex);
      }
      return *this;
    }
    /**
    * Returns true if ret was taken from the cache, false otherwise.
    */
    bool getOrCreate(const Key& key, std::shared_ptr<Val>& ret, Args... args)
    {
      std::lock_guard<std::mutex> lock(*_mutex);
      auto it = _cache.find(key);
      if (it != _cache.end()) {
        ret = std::shared_ptr<Val>(it->second); // Construct from weak_ptr
        return true;
      }
      ret = std::shared_ptr<Val>(_creator(args...), [this, key](Val* ptr) { // Construct from creator and pass custom deleter which removes the element from the cache.
        std::lock_guard<std::mutex> lock(*_mutex);
        _cache.erase(key);
        delete ptr;
      });
      _cache[key] = ret;
      return false;
    }
    inline void clear()
    {
      std::lock_guard<std::mutex> lock(*_mutex);
      _cache.clear();
    }
    inline size_t size()
    {
      std::lock_guard<std::mutex> lock(*_mutex);
      return _cache.size();
    }
  private:
    std::map<Key, std::weak_ptr<Val>> _cache;
    std::function<Val*(Args...)> _creator;
    std::unique_ptr<std::mutex> _mutex;
  };
}

#endif

Basic usage example:

PtrCache<std::string, GLTexture, const std::string&> texture_cache([] (const std::string& path) {
  auto tex = SOIL_load_OGL_texture(path.c_str(), SOIL_LOAD_AUTO, SOIL_CREATE_NEW_ID, SOIL_FLAG_MIPMAPS | SOIL_FLAG_TEXTURE_REPEATS | SOIL_FLAG_COMPRESS_TO_DXT);
  if (tex) {
    return new GLTexture(tex, GL_TEXTURE_2D);
  }
  else {
    throw std::exception((std::string("Could not create texture ") + path).c_str());
    return nullptr;
  }
});

std::shared_ptr<GLTexture> my_texture;
std::string path = "my_texture.png";
texture_cache.getOrCreate(path, my_texture, path);

The only caveat I see is that the user of this class has to ensure that the lifetime of the cache goes beyond the lifetime of any contained managed object. Otherwise, the custom deleter would obviously cause undefined behaviour.

\$\endgroup\$
1
\$\begingroup\$

The basic idea of your cache, as I understand it, is that it is a map of weak pointers connected to shared pointers with custom deleters that remove them from the map. As you noticed yourself, that's a risky pattern, because if any resources outlive the cache, you get UB.

Is it really necessary for objects to remove themselves from the cache? What if you just let them destroy themselves naturally, and leave the empty weak pointer in the cache. Then your lifetime worries fade away. From time to time you can purge the cache of entries where expired() == true if you really want, and other functions of the cache will have to take into account expired entries (like size() for example). It even makes your cache more flexible, because if you're not using the deleter you allow the possibility of users providing their own.

I also question the wisdom of keeping the "creator" function object in the cache, for a couple of reasons:

  • It makes the cache itself so much clunkier to use, because you have to specify all the arguments to the create function in the template parameter list.
  • It makes the cache less flexible, because there may be multiple ways to create (for example) a texture (such as from a file, or from a memory buffer), so you might want different create function signatures.

Rather than hard-coding the create function's signature in the type, and deciding on the create function for everything in cache at cache construction time, why not just pass an optional create function when and where it's needed?

I'll bring these points up again as I go through the code.

PtrCache(const std::function<Val*(Args...)>& creator) : 
  _creator(creator),
  _mutex(std::make_unique<std::mutex>())
{}

Single-argument constructors should pretty much always be explicit.

Since this is a pure sink function, you should really take the arguments by value, not by const&. And then you should move them into their final place:

explicit PtrCache(std::function<Val*(Args...)> creator) : 
  _creator{std::move(creator)},
  _mutex{std::make_unique<std::mutex>()}
{}

But I would suggest not having the _creator member variable, so you don't need this constructor at all. Or the Args... template parameter for the class either.

PtrCache(PtrCache&& other) :
  _cache(std::move(other._cache)),
  _creator(std::move(other._creator)),
  _mutex(std::move(other._mutex))
{}
PtrCache& operator=(PtrCache&& other)
{
  if (this != &other) {
    _cache = std::move(other._cache);
    _creator = std::move(other._creator);
    _mutex = std::move(other._mutex);
  }
  return *this;
}

There's no need to write out either of these, because they're just the default implementations. You can just do:

PtrCache(PtrCache&& other) noexcept = default;
PtrCache& operator=(PtrCache&& other) noexcept = default;

Also, both can be noexcept (and move ops always should be).

bool getOrCreate(const Key& key, std::shared_ptr<Val>& ret, Args... args)
{
  std::lock_guard<std::mutex> lock(*_mutex);
  auto it = _cache.find(key);
  if (it != _cache.end()) {
    ret = std::shared_ptr<Val>(it->second); // Construct from weak_ptr
    return true;
  }
  ret = std::shared_ptr<Val>(_creator(args...), [this, key](Val* ptr) { // Construct from creator and pass custom deleter which removes the element from the cache.
    std::lock_guard<std::mutex> lock(*_mutex);
    _cache.erase(key);
    delete ptr;
  });
  _cache[key] = ret;
  return false;
}

This is the meat of the cache, and so the best place to talk about broader problems. But the function has plenty of problems of its own.

First, out-arguments are a bad idea. They're clunky as all hell. Every use of this function - which is the main function of the cache - requires two lines:

std::shared_ptr<GLTexture> result;
texture_cache.getOrCreate("my_texture.png"s, result, "my_texture.png"s);

The natural place for the output of a function is the return value. If you want multiple return values - so you can get that bool as well - use a custom return struct, or a tuple:

std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Args... args);

Now this is much easier to use, especially since C++17:

auto [ result, created ] = texture_cache.getOrCreate("my_texture.png"s, my_texture.png"s);

At a higher level, you've hard-coded the create function into the cache, which makes it less flexible. Rather than doing that, you can take a function object in getOrCreate(), like this:

auto [ tex1, created1 ] = texture_cache.getOrCreate("texture_1", [] { return load_texture_from_file("texture_1.png"); });
auto [ tex2, created2 ] = texture_cache.getOrCreate("texture_2", [] { return generate_procedural_texture(seed_value); });
// and so on

The signature of getOrCreate() now becomes:

template <typename Func>
std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Func&& func);

and the creation line in the function is:

ret = std::make_shared<Val>(func());

Or even better, let the cache objects construct themselves:

template <typename Args...>
std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Args&&... args)
{
    // ...
    ret = std::make_shared<Val>(std::forward<Args>(args)...);
    // ...
}

// used as:
auto [ tex1, created1 ] = texture_cache.getOrCreate("texture_1", from_file, "texture_1.png");
auto [ tex2, created2 ] = texture_cache.getOrCreate("texture_2", noise_texture, seed_value);
// assumes texture_cache holds "texture" objects that have the following constructors, or something similar:
// texture::texture(from_file_t, std::filesystem::path);
// texture::texture(noise_texture_t, std::uint_fast32_t);

There are multiple ways to do all this that are way more flexible than setting the create function and its signature when the cache is created.

This function also seems a little over-complex, using find() and then []. Your map values are weak pointers. There's really no need for all that dancing around to get-or-set them in the map. You can just do:

bool getOrCreate(const Key& key, std::shared_ptr<Val>& ret, Args... args)
{
    std::lock_guard<std::mutex> lock(*_mutex);
    auto& wkptr = _cache[key];
    ret = wkptr.lock();
    if (ret)
        return true;
    // Either the value wasn't in the map, or it was but it expired
    // (so it was effectively removed from the map). Either way,
    // make a new thing and store it in the cache.
    ret = std::make_shared<Val>(_creator(args...));
    wkptr = ret;
    return false;
}

Also, along with everything I mentioned above about the create function stuff, if you're going to make this function a template - either to take a create function object or an argument parameter pack to pass to the value type's constructor - you might as well take advantage of that and use perfect forwarding for the key as well.

Also-also, you use make_unique(), and that's good... but it's not that important. It is, however, important to use make_shared(). You should aim for that in the function's design.

inline void clear()
{
    std::lock_guard<std::mutex> lock(*_mutex);
    _cache.clear();
}

The inline keyword serves no purpose here. Functions defined in the class are inline by default. What would be handy here are const and noexcept. At least const, because locking might throw an error. Same goes for size().

There's another nasty little issue with having a clear() function, though. Your cache objects erase themselves from the cache when the last one goes out of scope, right? Consider this scenario:

  1. Subsystem A creates a cache object with key "foo", and gets an instance.
  2. Subsystem (any!) calls clear() on the cache. Subsystem A's "foo" instance is still valid, of course. It just doesn't have an entry in the cache anymore.
  3. Subsystem B creates a new cache object with the key "foo", and gets an instance.
  4. Subsystem A lets its "foo" instance expire... which triggers the deleter to erase "foo" from the cache.
  5. Subsystem B asks for another "foo" object from the cache, expecting to get an alias of the one it got in step 3 because it knows no-one's called clear() since it got the "foo" object in step 3... instead it gets a whole new "foo".

In other words, the cache becomes unpredictable. Even if I'm holding an object I got from a specified key and I know for sure no one's cleared the cache since I got it, I still can't be sure I'll get the same object from the same key (or any object at all from that key).

std::map<Key, std::weak_ptr<Val>> _cache;

Is there any reason you're not using an unordered map here?

Summary

  • As you've noted, having cache objects remove themselves from the cache when they expire invites problems. There's no good reason to do that. Just leave the empty weak pointers in the cache, and cull them from time to time (or not; just leave them there and ignore them).
  • Defining the create function arguments at instantiation time, and the create function itself at construction time, is inflexible, and it complicates the class. It's much more flexible to be able define creation parameters when and where I need them. The class should be PtrCache<Key, Val>, maybe with an allocator option. Having Args... in there is clunky and restrictive.
  • The most flexible option is probably to let your cache objects' constructors do what constructors are for, and define getOrCreate() as a template function taking arguments that get perfect-forwarded to the constructor if and only if something actually gets constructed.
  • Use make_shared().
  • Don't manually redefine implicitly generated functions; just = default them.
\$\endgroup\$
  • \$\begingroup\$ Note: you cannot make clear() const as it has to modify the map (unless you make the map mutable, which I wouldn't recommend). \$\endgroup\$ – hoffmale Jul 5 '18 at 14:37
  • \$\begingroup\$ Thanks for the extensive feedback! The reason I do not use std::make_sharedis because I want to use this in combination with boost::object_poolin order to reduce memory fragmentation. Good point regarding flexibility: The signature of the method now looks like this: template<typename Creator, typename Deleter> auto getOrCreate(const Key& key, Creator&& creator, Deleter&& deleter). I also got rid of the clear() method as I actually don't need it anyway. \$\endgroup\$ – user167941 Jul 5 '18 at 20:21
  • \$\begingroup\$ std::make_shared() exists specifically to prevent memory fragmentation. A better option might be to add an allocator template parameter to the class, and use std::allocate_shared() instead of std::make_shared(). That would allow users to choose their own allocation strategies, including the standard pools like std::pmr::unsynchronized_pool_resource. \$\endgroup\$ – indi Jul 5 '18 at 20:46
  • \$\begingroup\$ std::make_shared / std::make_unique definitely can lead to memory fragmentation. I used it to construct individual nodes when building a Bounding Volume Hierarchy (BVH). Switching to boost::object_pool significantly improved performance for tree traversal. Of course, that may be due to the fact that I use std::sort during BVH construction, but I also checked the memory addresses std::make_unique is giving me in general and they aren't nearly that thightly packed compared to the pool allocation. \$\endgroup\$ – user167941 Jul 5 '18 at 22:05
  • \$\begingroup\$ Yes, make_shared() can still lead to fragmentation if you're not using it with an allocator, but not using it will lead to fragmentation even with your object pool. To prevent fragmentation with shared_ptr, you need to use a non-fragmenting allocator AND make_shared()/allocate_shared(). (With make_unique() it doesn't matter. This is only a problem with shared_ptr.) \$\endgroup\$ – indi Jul 5 '18 at 22:48
1
\$\begingroup\$

Here is an updated version based on the accepted answer:

  template<typename Key, typename Val>
  class PtrCache
  {
  public:
    PtrCache() = default;
    PtrCache(const PtrCache& other) = delete;
    PtrCache& operator=(const PtrCache& other) = delete;
    PtrCache(PtrCache&& other) noexcept = default;
    PtrCache& operator=(PtrCache&& other) noexcept = default;
    template<typename Creator, typename Deleter>
    auto getOrCreate(const Key& key, Creator&& creator, Deleter&& deleter)
    {
      LockGuard guard(*_mutex);
      auto& weak = _cache[key];
      if (auto strong = weak.lock()) {
        return Ret(strong, true);
      }
      std::shared_ptr<Val> strong(creator(), [this, key, deleter](Val* ptr) {
        LockGuard guard(*_mutex);
        _cache.erase(key);
        deleter(ptr);
      });
      weak = strong;
      return Ret(strong, false);
    }
    auto size()
    {
      LockGuard guard(*_mutex);
      return _cache.size();
    }
  private:
    using Ret = std::tuple<std::shared_ptr<Val>, bool>;
    using LockGuard = std::lock_guard<std::mutex>;
    std::map<Key, std::weak_ptr<Val>> _cache; // std::unordered_map should be fine as well.
    std::unique_ptr<std::mutex> _mutex = std::make_unique<std::mutex>();
};

And an example using boost::object_pool :

template<typename API>
class Renderer
{
public:
  using GPUMeshData = typename API::GPUMeshData;
  using MeshCache = PtrCache<std::shared_ptr<Mesh>, GPUMeshData>;
  std::shared_ptr<GPUMeshData> addMesh(const std::shared_ptr<Mesh>& mesh)
  {
    return std::get<0>(_meshCache.getOrCreate(mesh, [this, mesh]() {
      return new (_meshPool.malloc()) GPUMeshData(*mesh);
    }, [this](GPUMeshData* gmd) {
      _meshPool.destroy(gmd);
    }));
  }
private:
  // Declaration order is important!
  MeshCache _meshCache;
  boost::object_pool<GPUMeshData> _meshPool;
};
\$\endgroup\$

Your Answer

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