Skip to content

Commit

Permalink
[12782] Add new config setting LoadAllGridsOnMaps
Browse files Browse the repository at this point in the history 
With this setting one can store a list of maps that are automatically
loaded with all grids and all creatures.
Continents will be loaded on server-startup, instances when a player
enters.

NOTE: Using maps with this setting will cost huge amount of cpu and ram
power, and hence it must only be used for development purposes
  • Loading branch information
@Schmoozerd
Neo2003 authored and Schmoozerd committed Nov 13, 2014
1 parent d193f83 commit e5182a3
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 3 deletions.
17 changes: 17 additions & 0 deletions src/game/World.cpp
View file
Expand Up @@ -115,6 +115,8 @@ World::World()

for (int i = 0; i < CONFIG_BOOL_VALUE_COUNT; ++i)
m_configBoolValues[i] = false;

m_configForceLoadMapIds = NULL;
}

/// World destructor
Expand All @@ -141,6 +143,9 @@ World::~World()
VMAP::VMapFactory::clear();
MMAP::MMapFactory::clear();

if (m_configForceLoadMapIds)
delete m_configForceLoadMapIds;

This comment has been minimized.

Sign in to view

Copy link
@evil-at-wow

evil-at-wow Nov 14, 2014

Contributor

FYI: the check is not needed - it is perfectly valid C++ to call delete on a null pointer.

This comment has been minimized.

Sign in to view

Copy link
@Schmoozerd

Schmoozerd Nov 15, 2014

Member

this is true. But i prefer the other way.
call delete if and only if new is used :)

// TODO free addSessQueue
}

Expand Down Expand Up @@ -537,6 +542,18 @@ void World::LoadConfigSettings(bool reload)
setConfig(CONFIG_BOOL_ADDON_CHANNEL, "AddonChannel", true);
setConfig(CONFIG_BOOL_CLEAN_CHARACTER_DB, "CleanCharacterDB", true);
setConfig(CONFIG_BOOL_GRID_UNLOAD, "GridUnload", true);

std::string forceLoadGridOnMaps = sConfig.GetStringDefault("LoadAllGridsOnMaps", "");
if (!forceLoadGridOnMaps.empty())
{
m_configForceLoadMapIds = new std::set<uint32>;
unsigned int pos = 0;
unsigned int id;
VMAP::VMapFactory::chompAndTrim(forceLoadGridOnMaps);
while (VMAP::VMapFactory::getNextId(forceLoadGridOnMaps, pos, id))
m_configForceLoadMapIds->insert(id);
}

setConfig(CONFIG_UINT32_INTERVAL_SAVE, "PlayerSave.Interval", 15 * MINUTE * IN_MILLISECONDS);
setConfigMinMax(CONFIG_UINT32_MIN_LEVEL_STAT_SAVE, "PlayerSave.Stats.MinLevel", 0, 0, MAX_LEVEL);
setConfig(CONFIG_BOOL_STATS_SAVE_ONLY_ON_LOGOUT, "PlayerSave.Stats.SaveOnlyOnLogout", true);
Expand Down
6 changes: 6 additions & 0 deletions src/game/World.h
View file
Expand Up @@ -560,6 +560,9 @@ class World
/// Get a server configuration element (see #eConfigBoolValues)
bool getConfig(eConfigBoolValues index) const { return m_configBoolValues[index]; }

/// Get configuration about force-loaded maps
std::set<uint32>* getConfigForceLoadMapIds() const { return m_configForceLoadMapIds; }

/// Are we on a "Player versus Player" server?
bool IsPvPRealm() { return (getConfig(CONFIG_UINT32_GAME_TYPE) == REALM_TYPE_PVP || getConfig(CONFIG_UINT32_GAME_TYPE) == REALM_TYPE_RPPVP || getConfig(CONFIG_UINT32_GAME_TYPE) == REALM_TYPE_FFA_PVP); }
bool IsFFAPvPRealm() { return getConfig(CONFIG_UINT32_GAME_TYPE) == REALM_TYPE_FFA_PVP; }
Expand Down Expand Up @@ -698,6 +701,9 @@ class World
// used versions
std::string m_DBVersion;
std::string m_CreatureEventAIVersion;

// List of Maps that should be force-loaded on startup
std::set<uint32>* m_configForceLoadMapIds;

This comment has been minimized.

Sign in to view

Copy link
@evil-at-wow

evil-at-wow Nov 14, 2014

Contributor

Is there any specific reason why you've chosen this to be a pointer that needs some 'management code' (see constructor/destructor)? I expected to find the set here (which stays potentially empty) with getConfigForceLoadMapIds() returning a reference to a const set.

This comment has been minimized.

Sign in to view

Copy link
@Schmoozerd

Schmoozerd Nov 15, 2014

Member

pointer == NULL is very neat check to see if there is data. Also remark that this is the expected value for this setting.
(only in rare cases an actual set will be passed via config)

This comment has been minimized.

Sign in to view

Copy link
@cyberium

cyberium Nov 15, 2014

Member

I prefer too using reference as much as possible.
in this case we could have :
std::set m_configForceLoadMapIds;
std::set const& getConfigForceLoadMapIds() const { return m_configForceLoadMapIds; }

And then a simple
std::set const& forcedMap = getConfigForceLoadMapIds();
if (!forcedMap.emtpy()) blabla....
Wish is neat too imho.

This comment has been minimized.

Sign in to view

Copy link
@Schmoozerd

Schmoozerd Nov 15, 2014

Member

true enough -- using a set is of course also neat from code-lines, probably even slightlier neater because of less overhead.

But is it better for the default case of empty set (or here: NULL pointer) ?
Remark that this check is done on any added creature.

This comment has been minimized.

Sign in to view

Copy link
@cyberium

cyberium Nov 15, 2014

Member

Ah if performance is important then you are right.
Make the function inline also to be sure to get maximum perf.

This comment has been minimized.

Sign in to view

Copy link
@evil-at-wow

evil-at-wow Nov 15, 2014

Contributor

But is it better for the default case of empty set (or here: NULL pointer)?

No, but that's only one part of the question. The second part is: is it worse in that case?

To be honest, my own first guess was "yes, probably". If you think about it, you can't really come up with anything faster than checking if a pointer is a null pointer. It's typically going to boil down to a single CPU instruction testing an address. So I figured that, despite std::set's empty() being guaranteed to have complexity O(1) aka constant, doing the empty() function call would have a minor impact.

Now, if there's anything I've learned in the past few years of my professional career, it is that many people underestimate modern compilers and their ability to optimize stuff. I would say @cyberium's reaction is a typical and understandable reaction. But the only way to be absolutely sure is to actually test and measure things!

So I wrote a small test program to do the function call (returning a pointer in one case, and a reference in the other case) and check the result (pointer compare with NULL or calling empty() on the reference), and do this a billion times. It turns out that my first guess is only correct for a non optimizing build, because in that case std::set's empty() is actually called, so we have the function call overhead. But when optimizing, my compiler is - not entirely unexpected I'd say - smarter than I expected. It figures out that the std::set's empty() call is basically calling empty() on it's red-black tree implementation, and that in turn is nothing more than checking a member for being 0. It simply generates code that compares that particular member, which is just an address, with 0. I've actually checked the assembler it generates for my loop. When the member is a pointer and the function returns a pointer:

    mov    esi, 1000000000
.L2:
    sub    esi, 1
    je     .L10
.L7:
    mov   eax, DWORD PTR _ZL5m_set
    test  eax, eax
    je    .L2
    <code to do something when not empty>
    sub    esi, 1
    jne    .L7
.L10:
    <code to end main>

Note that my code actually called the function to get the pointer, but because it merely returned a pointer or reference to a global variable, the compiler noticed it could optimized my code and avoid the function call altogether.
Then, when using the set and returning a reference:

    mov    ebx, 1000000000
    jmp    .L7
.L2:
    sub    esi, 1
    je     .L10
.L7:
    mov   eax, DWORD PTR _ZL5m_set+20
    test  eax, eax
    je    .L2
    <code to do something when not empty>
    sub    ebx, 1
    jne    .L7
.L10:
    <code to end main>

It is interesting to see the compiler generated a jump before the loop in this case (it already knows you're going to execute the loop's body at least once) and not in the other case. But more importantly, there's no sign at all of my if ( !result.empty() ) code! It's just checking some address as before (at offset 20 in my set member).

The conclusion for my Linux system usig GCC 4.8.x: a pointer or a reference makes no difference at all performance wise (at least when you ask the compiler to optimize)! So the real trade-off is some code elegance and clarity versus a few bytes of memory (the pointer member will take 4 or 8 bytes, a set member is typically a bit larger).

To make it clear: I did the test above because I wanted to verify my own guess about the potential performance difference. The code is certainly not wrong, and I'm not asking to change the code. This code works and using a set+reference instead of a pointer to a set is not going to make it faster or more correct. Just more elegant, but I realize that's rather subjective. I wrote down my findings anyway only to share a result that might change your view - and future code - slightly: modern compilers are usually very good at optimizing code, so don't try to beat your compiler. Write the code for humans, and optimize only when you actually notice a performance issue.

This comment has been minimized.

Sign in to view

Copy link
@cyberium

cyberium Nov 15, 2014

Member

Thank you professor :)
Was asking same thing about performence and finaly i thought that compiler cannot avoid the call to empty function so overhead in reference case.
I must also start to have more trust on compiler then :)

Anyway 👍
};

extern uint32 realmID;
Expand Down
4 changes: 2 additions & 2 deletions src/game/vmap/VMapFactory.cpp
View file
Expand Up @@ -24,7 +24,7 @@ using namespace G3D;

namespace VMAP
{
void chompAndTrim(std::string& str)
void VMapFactory::chompAndTrim(std::string& str)
{
while (str.length() > 0)
{
Expand Down Expand Up @@ -58,7 +58,7 @@ namespace VMAP
//===============================================
// result false, if no more id are found

bool getNextId(const std::string& pString, unsigned int& pStartPos, unsigned int& pId)
bool VMapFactory::getNextId(const std::string& pString, unsigned int& pStartPos, unsigned int& pId)
{
bool result = false;
unsigned int i;
Expand Down
3 changes: 3 additions & 0 deletions src/game/vmap/VMapFactory.h
View file
Expand Up @@ -37,6 +37,9 @@ namespace VMAP

static void preventSpellsFromBeingTestedForLoS(const char* pSpellIdString);
static bool checkSpellForLoS(unsigned int pSpellId);

static void chompAndTrim(std::string& str);
static bool getNextId(const std::string& pString, unsigned int& pStartPos, unsigned int& pId);
};
}

Expand Down
7 changes: 7 additions & 0 deletions src/mangosd/mangosd.conf.dist.in
View file
Expand Up @@ -112,6 +112,12 @@ BindIP = "0.0.0.0"
# Default: 1 (unload grids)
# 0 (do not unload grids)
#
# LoadAllGridsOnMaps
# Load grids of maps at server startup (if you have lot memory you can try it to have a living world always loaded)
# This also allow ALL creatures on the given maps to update their grid without any player around.
# Default: "" (don't load all grids at startup)
# "mapId1[,mapId2[..]]" (DO load all grids on the given maps- Experimental and very resource consumming)
#
# GridCleanUpDelay
# Grid clean up delay (in milliseconds)
# Default: 300000 (5 min)
Expand Down Expand Up @@ -206,6 +212,7 @@ PlayerLimit = 100
SaveRespawnTimeImmediately = 1
MaxOverspeedPings = 2
GridUnload = 1
LoadAllGridsOnMaps = ""
GridCleanUpDelay = 300000
MapUpdateInterval = 100
ChangeWeatherInterval = 600000
Expand Down
2 changes: 1 addition & 1 deletion src/shared/revision_nr.h
View file
@@ -1,4 +1,4 @@
#ifndef __REVISION_NR_H__
#define __REVISION_NR_H__
#define REVISION_NR "12781"
#define REVISION_NR "12782"
#endif // __REVISION_NR_H__

0 comments on commit e5182a3

Please sign in to comment.