SFML Development Book question regarding the resource holder class.

[Veritas]

So I started reading the sfml development book to look at the possible structure of a game and to gain some more knowledge of the sfml library. There are some points in the book that I don't understand WHY they chose to implement something in a particular way. An example is the resource class in the beginning of the book. When I looked at it something struck me as weird but I didn't realize what it was until a fellow stack overflow member reminded me that the [] operator of the std::map container creates an object if the key doesn't exist.

The book's implementation:

template <typename Resource, typename Identifier>
void ResourceHolder<Resource, Identifier>::load(Identifier id, const std::string& filename)
{
        std::unique_ptr<Resource> resource(new Resource());
        if (!resource->loadFromFile(filename))
        {
                 throw std::runtime_error("ResourceHolder::load - Failed to load " + filename);
        }
        auto inserted = mResourceMap.insert(std::make_pair(id, std::move(resource)));
        assert(inserted.second);
}
 

Another possible implementation:

template <typename Resource, typename Identifier>
void ResourceHolder<Resource, Identifier>::load(Identifier id, const std::string& filename)
{
        assert(mResourceMap.find(id) == mResourceMap.end());
        if (!mResourceMap[id].loadFromFile(file))
        {
                mResourceMap.erase(id);
                throw std::runtime_error("ResourceHolder::load - Failed to load " + file);
        }
}
 

Are there any advantages in the book's implementation?

EDIT: Just noticed that this is the help section's general category.

[Nexus]

The second code is not equivalent, as operator[] uses the default constructor, which creates a null pointer. You still need a call to the new operator.

Furthermore, the book's implementation is more conservative with respect to rollback semantics. If loading fails, the resource is not inserted -- instead of inserted and erased again. More important than the small performance benefit (insertion and removal in a map requires dynamic memory management), the code is exception-safe: if loadFromFile() throws an exception, the map is not left in an inconsistent state (null resource). Even though loadFromFile() does not throw documented exceptions, there's the possibility of std::bad_alloc when no big enough memory block can be allocated for the resource memory. And it's generally not a bad idea to be prepared for exceptions -- which becomes mostly an easy task, thanks to RAII.

[Veritas]

Well the alternative version was written assuming that the map was changed to contain resource objects instead of pointers but I can see your point about exceptions. I did think of bad_alloc throwing but I kind of underestimated the possibility of it happening. Can you elaborate on the part about efficiency?

btw: why not use emplace instead of insert to avoid the make_pair middleman ?

[Nexus]

I did think of bad_alloc throwing but I kind of underestimated the possibility of it happening.

It happens rarely, especially in today's RAM capacities, so people tend to forget about this exception. But it's still of importance when you don't know the resource size and hardware in advance. Keep in mind that there could be a problem even if there is enough free memory, but it's not contiguous (due to memory fragmentation, for example).

If you are able to write code transparently without explicitly handling the error, that's even better. Resource management is something really beautiful in C++, you almost never have try/catch blocks -- whereas languages like Java force you to clutter the whole code with them (and even worse, finally).

Can you elaborate on the part about efficiency?

std::map is a tree-based data structure, so it typically allocates and deallocates memory for every single node (i.e. element). But as already stated, the effect is minor, especially in comparison with expensive resource loading.

[Veritas]

I agree, the try/catch block to handle loadFromFile looks very ugly actually.
By the way I am curious as to why std::map allocating memory hurts performance, I mean either way you are making one sf::Texture object, the difference is that in one version we create it as a member of the std::pair while in the other one we don't. In both versions std::map takes care of it's std::pair members.

[Ixrec]

Inserting objects into a map always involves the map allocating memory for a new node and copying an object into that node.  Whether you do the insertion with insert() or operator[] doesn't matter (unless the key already exists, obviously).  In fact I think operator[] involves a default construction before the copy assignment, so insert() might be more efficient.

In both versions std::map takes care of it's std::pair members.

That's the point.  To "take care of" them it has to dynamically allocate memory and do copy construction.

Of course, the exception safety issue is much more important.

[Veritas]

What I mean is that when doing:

std::unique_ptr<Resource> resource(new Resource());

We make a new Resource object using the default constructor and we set the pointer to it.
We then move construct an std::pair and insert it to the map.

When we have a map:

std::map<std::string, Resource> resources

and we call,

resources[id]

if id does not exist it simply probably makes a node for the std::pair using it's appropriate constructor. I am not sure how they have implemented the RBT but I don't think they would have implemented it so naively that it needs an extra copy.. Again the default constructor is used for the Resource object in the pair. There shouldn't be any copy-constructors invoked.
I am probably missing something here, thank you for your patience.

[Ixrec]

I should have been saying move instead of copy, you're right.  And I'd forgotten that the default construction happens either way so that point was irrelevant.

Anyway, the point was that if loadFromFile fails, in the book's code the insertion (with dyanmic allocation and move construction) never gets performed, and no corresponding deletion (with destruction) needs to be performed either.

Again, the real benefit of doing this is the exception safety (the map remains unchanged if loadFromFile fails), and the performance benefit of not inserting/deleting a node when failure occurs is just a nice side effect.  Modern RAII approaches to exception safety tend to produce a lot of these little performance bonuses.

In case there was some confusion: I don't believe there is any performance difference at all when no failure occurs.

[Veritas]

I understand that, the alternative version is also less efficient because it does an extra search when using the [] operator. Nexus said that memory management makes a difference (or was he referring to the insertion/deletion ?) and it's that part I don't understand , probably a result of me not knowing how the map class is implemented in the stl.

[Nexus]

Nexus said that memory management makes a difference (or was he referring to the insertion/deletion ?)

Yes, because insertion/deletion implies memory management.

[Veritas]

Ok I got it, I thought you were reffering to the actual memory management done when using insert and the [] operator since the insertion/deletion was an algorithm design error.

[Jesper Juhl]

... probably a result of me not knowing how the map class is implemented in the stl.

std::map is commonly implemented as a red-black tree. But you'd have to check your specific STL implementation since the standard doesn't mandate a specific data structure.

[Veritas]

Yea I know that they use a RBT structure I just haven't looked at the actual code. Thanks though!