Creating a cache manager

I am purely new to C++ memory management. Am I on the right path, or should I employ a different design strategy or a different memory manager policy (such as weak_ptr)?

```
#include
#include
#include
#include
#include
#include
#include

//
// Requirements:
// 1: the bitmap could be used by multiple thread safely.(std::shared_ptr could?)
// 2: cache the bitmap and do not always increase memeory
//@NotThreadSfe
struct Bitmap {
public:
Bitmap(const std::string& filePath) {
filePath_ = filePath;
printf("foo %x ctor %s\n", this, filePath_.c_str());
}
~Bitmap() {
printf("foo %x dtor %s\n", this, filePath_.c_str());
}
std::string filePath_;
};

//@ThreadSafe
struct BitmapCache {
public:
static std::shared_ptr loadBitmap(const std::string& filePath) {
mutex_.lock();

//whether in the cache
auto iter = cache_.find(filePath);
if (iter != cache_.end()) {
if ((*iter).second) {
return (*iter).second;
} else {
std::shared_ptr newPtr(new Bitmap(filePath));
(*iter).second = newPtr;
return newPtr;
}
}

//try remove unused elements if possible
if (cache_.size() >= kSlotThreshold) {
std::unordered_map>::iterator delIter = cache_.end();
for (auto iter = cache_.begin(); iter != cache_.end(); ++iter) {
auto& item = *iter;
if (item.second && item.second.use_count() == 1) {
delIter = iter;
break;
}
}
if (cache_.end() != delIter) {
(*delIter).second.reset();
cache_.erase(delIter);
}
}

//create new and insert to the cache

The major question you should be asking is "what am I going to do with these bitmaps"? Threading is an issue when you have multiple threads writing the same data - multiple threads reading the same data (and only reading it) don't cause such problems. Whether you are going about this the correct way or not depends heavily on the answer to that question.

However, some of this code can be cleaned up and improved somewhat, regardless of that answer.

Calling mutex_.lock() and then explicitly calling mutex_.unlock() is dangerous. If anything in between those calls throws an exception, then your mutex will never be unlocked, and you'll have a deadlock. Solving this is easy, and just requires using what is known as RAII (Resource Acquisition is Initialization). In this case, this is done by using a std::lock_guard:

static std::shared_ptr loadBitmap(const std::string& filePath) {
      std::lock_guard guard(mutex_);
      ...
  } // mutex_ is unlocked as soon as it goes out of scope

This ensures that mutex_ will always be unlocked, no matter how the function exists (whether normally or through an exception).

You use auto in a few places, so you might as well simplify:

std::unordered_map>::iterator delIter = cache_.end();

to:

auto delIter = cache_.end();

The loadBitmap function itself has dubious design. It isn't just loading a bitmap, but is also trying to clean out the cache. Performing this second step should absolutely be broken out into a separate function:

void remove_from_cache()
{
    if (cache_.size() >= kSlotThreshold) {
    auto delIter = cache_.end();
    for (auto iter = cache_.begin(); iter != cache_.end(); ++iter) {
        auto& item = *iter;
        if (item.second && item.second.use_count() == 1) {
            delIter = iter;
            break;
        }
    }
    if (cache_.end() != delIter) {
        (*delIter).second.reset();
        cache_.erase(delIter);
    }
}

The current cache size is very small (20 elements). If you expect this to remain small (>> (which could really do with a using statement or 3)), then erasing values is simply:

using pair_t = std::pair>;
using container_t = std::vector;
container_t cache;

void remove_from_cache()
{
    if(cache.size() > kSlotThreshold) {
        cache.erase(std::remove_if(cache.begin(), cache.end(),
             [](const pair_t& p) { return p.second.use_count() == 1; }),
             cache.end());
    }
}

Whether this removal algorithm is any good or not is up for some debate, you might want to look at something like an LRU (least recently used) cache instead - currently, there is no guarantee that your algorithm will remove things from the cache. Also, this would require a (very minor) change to your lookup code:

auto iter = cache_.find(filePath);

would become:

auto iter = std::find_if(cache.begin(), cache.end(), 
     [&](const pair_t& p) { return p.first == filePath; });

A few final small points:

Prefer to use std::make_shared instead of constructing the shared pointer directly, for example:

std::shared_ptr newPtr(new Bitmap(filePath));

should be:

auto newPtr = std::make_shared(filePath);

If you're defining a struct just to give things a scope to live in (and hence using static for everything), use a namespace` instead.

StackExchange Code Review Q#55084, answer score: 2