5
\$\begingroup\$

I'm programming a MMO server (for a game called Tibia), this server can handle a maximum number of 1.000 players continuously, the world is in constant movement, over 35.000 monsters and over 300 NPCs.

The map interacts with items, items continuously decay into another item, items are constantly being created by purchasing items on NPCs and deleted when selling items to NPCs, when monsters are killed items are created as well, when monster corpses decay into nothing items inside are deleted as well, players splitting gold pieces creates a new item as well, as well as joining multiple items deletes the residual pieces to combine them into one, so this means that on my server there are going to be a lot of interactions of item creations and deletions.

Also thought of using an object pool because it makes it easier to manage the memory of all the stated above.

Header file:

#pragma once

class Item;
class Player;

struct ObjectType
{
    uint32_t index = 0;
};

class ObjectPool
{
public:
    static constexpr uint32_t ITEM_POOL_SIZE = 16384;
    static constexpr uint32_t MAX_PLAYER_SIZE = 1000;

    explicit ObjectPool() = default;
    ~ObjectPool();

    void allocateItems(uint32_t size = ITEM_POOL_SIZE);
    Item* createItem();
    void freeItem(Item* item);

    Player* createPlayer(const std::string& name);
private:
    Item* m_items = nullptr;
    Item* m_firstFreeItem = nullptr;
    uint32_t m_maxItems = 0;

    std::unordered_map<std::string, Player*> m_players;
};

extern ObjectPool g_objectpool;

Source file

#include "noxuspch.h"

#include "objectpool.h"
#include "item.h"
#include "player.h"

ObjectPool::~ObjectPool()
{
    delete[] m_items;
    while (m_players.size())
    {
        auto it = m_players.begin();
        delete it->second;
        it = m_players.erase(it);
    }
}

void ObjectPool::allocateItems(uint32_t size/* = DEFAULT_POOL_SIZE*/)
{
    if (m_items)
    {
        Item* newItems = new Item[m_maxItems + size];
        memcpy(newItems, m_items, sizeof(Item) * m_maxItems);
        delete[] m_items;
        m_items = newItems;

        uint32_t newMaxItems = m_maxItems + size;
        for (uint32_t index = m_maxItems; index < newMaxItems; index++)
        {
            Item* item = &m_items[index];
            item->m_objectType.index = index;
            if (index + 1 < newMaxItems)
            {
                item->m_nextItem = &m_items[index + 1];
                item->m_nextFreeItem = item->m_nextItem;
            }
        }

        Item* newFreeItem = &m_items[m_maxItems];
        Item* last = &m_items[m_maxItems - 1];
        last->m_nextItem = newFreeItem;
        last->m_nextFreeItem = newFreeItem;

        m_firstFreeItem = newFreeItem;
        m_maxItems = newMaxItems;
        return;
    }

    m_items = new Item[size];
    m_firstFreeItem = &m_items[0]; // first index item
    m_maxItems = size;

    for (uint32_t index = 0; index < size; index++)
    {
        Item* item = &m_items[index];
        item->m_objectType.index = index;

        if (index + 1 < m_maxItems)
        {
            item->m_nextItem = &m_items[index + 1];
            item->m_nextFreeItem = item->m_nextItem;
        }
    }
}

Item* ObjectPool::createItem()
{
    if (!m_firstFreeItem)
    {
        std::cout << "Warning - [ObjectPool::createItem]: Pool size small (" << m_maxItems << " -> " << m_maxItems + ITEM_POOL_SIZE << ')' << std::endl;
        allocateItems(ITEM_POOL_SIZE);
        return createItem();
    }

    Item* item = m_firstFreeItem;
    m_firstFreeItem = item->m_nextFreeItem;
    return item;
}

void ObjectPool::freeItem(Item* item)
{
    //item = new (item) Item; // placement new to restore to original state
    item->m_nextFreeItem = m_firstFreeItem;
    m_firstFreeItem = item;
}

Player* ObjectPool::createPlayer(const std::string& name)
{
    auto it = m_players.find(name);
    if (it == m_players.end())
    {
        Player* player = new Player();
        m_players[name] = player;
        return player;
    }

    return it->second;
}
\$\endgroup\$
  • \$\begingroup\$ It would be great to have a header file of the classes to provide better code review. \$\endgroup\$ – Incomputable Jul 5 '17 at 2:58
  • \$\begingroup\$ Hi, Raúl. If you want avoid memory leaks you should release all the memory, as You are doing into destructors, but not keep it into a pool awaiting for use them again because you are consumming the resourses, that is the real consecuence of memory leak. \$\endgroup\$ – Jorge Omar Medra Jul 5 '17 at 6:01
  • \$\begingroup\$ @Incomputable I have added the full source code for review and better explanation as well \$\endgroup\$ – Raúl Sanpedro Jul 5 '17 at 21:31
3
\$\begingroup\$

I see a number of things that may help you improve your program.

Make sure you have all required #includes

The header uses uint32_t but doesn't #include <cstdint>. It also requires <string> and <unordered_map> Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation. In this case, the implementation additionally requires <cstring> and <iostream>.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.

Avoid global objects

The code includes this line:

extern ObjectPool g_objectpool;

which suggests that you are creating a global object. This is usually a bad idea and should be eliminated if practical. One way to do so would be to have such an object within main and then pass a reference to this object if needed by other functions or objects.

Be careful with autogenerated code

The compiler will automatically create a copy constructor that would allow someone to write and compile this line of code:

ObjectPool pool2(g_objectpool);

The problem is that this will result in two objects (pool2 and g_objectpool) having the exact same pointers. This is almost surely going to result in disaster because both objects will attempt to delete when they go out of scope, resulting in double frees as best and a program crash is likely. Prevent this by either defining your own copy constructor or, more logical in this instance I think, delete the default operator:

ObjectPool(const ObjectPool &) = delete;

Rethink the design

The callers of createItem() get a raw Item pointer, and presumably should call freeItem() when they're done. Also, each Item apparently serves double duty also as a linked list node for both allocated and free Items. This is a very intrusive implementation that places a large additional burden on users of the code. Further, the memory pool handles both Player and Item objects, but in completely different ways. A better way to design this would be to have custom new and delete operators for both Player and Item classes, allowing the fact that they're handled by a memory pool to be completely hidden in the implementation details rather than burdening the user with an awkward interface. See this question for some thoughts on how that might look.

Measure your code

If you implement a memory pool as described above (via custom new and delete), it's very simple to measure the code with only the standard new and delete and with the custom versions to make sure that your code is actually performing as you intend. I would highly recommend writing and measuring the code first without a memory pool, and measuring and profiling the code that way. If you determine by measuring that the program would benefit from a memory pool, you can write it then, remeasure and have a solid "baseline" with which to compare any new scheme.

\$\endgroup\$
2
\$\begingroup\$

First of all, using a pool to make it easier to work with memory is a bad idea. It definitely doesn't make it easier. Your code is quite non-trivial. If you want to make it easy, just stick with standard containers and smart pointers if you really need them. Odds are, you don't need any pointers at all.

For instance, why can't m_players be a map from an std::string to Player? Why can't you just create items without dynamic memory allocations? It's not clear from the code you've posted.

A valid reason to use a custom pool would be performance. If standard containers are too slow for your application, you might need it (but again, I'd recommend implementing it without manual memory management and measuring the performance. You'll save a lot of time and effort if it turns out to be satisfactory).

You also seem to maintain some kind of linked list of free items. Why not use std::list instead? Are there any good reasons not to use it here? I don't see the point in reinventing the wheel.

This part: memcpy(newItems, m_items, sizeof(Item) * m_maxItems) might be incorrect (I can't say for sure as you didn't post your Item class). Not all objects can be copied byte by byte. Do you have any reasons not use std::copy instead?

To sum it up, I'd recommend to start with standard C++ containers and algorithms first (they are efficient for most of the practical purposes) and try to change something only if you run into performance issues with them (Again, measure it. Don't assume that they'll be too slow). It would be great if you can avoid manual memory management altogether. It's very easy to get it wrong. It's also quite possible that your custom solution turns out to be slower than standard C++.

\$\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.