[]patch][8182] Updating map system

[Auntie Mangos]

There you go

http://getmangos.eu/community/viewtopic.php?id=4646

And of course the endless crashdump about the same issue

http://getmangos.eu/community/viewtopic.php?id=4235

[Auntie Mangos]

when i have the crash with this path, is only "segmentation fault"

http://getmangos.eu/community/viewtopic.php?id=4579

[Ambal]

HI everyone Want to present you first patch from planned by me and mangos dev team map system changes. We plan to add around 4 patches, which will offer different performance improvements and modifications to the core. In the end there will be a multithreading patch, based on Derex' mtmaps, but offering much more scalability

What does this patch do:

1) Improves server performance by caching all world objects' map locations. This affects around 216(!) code places from grid loading to different location checks, e.g. IsInWater() etc.

2) from now on you cannot explicitly call for SetMapId()/SetINstanceId() to manipulate object's mapId information. Use WorldObject::SetMap() instead.

3) All redundant calls to MapManager::CreateMap()/GetBaseMap() were replaced by WorldObject::GetMap() calls! This was done to improve code performance.

Attention! Due to conversation with Wyk3d MapManager::CreateMap() was left as is, but MapManager::GetBaseMap() renamed to MapManager::CreateBaseMap(). Even if you have patches which compile with this patch, I insist that you changed ALL MapManager::CreateMap()/GetBaseMap() calls to WorldObject::GetMap() ! Or post them here so I can help you make them compatible with future changes...

For example:

Old code:

pObj->SetRespawnTime(GetSpellDuration(GetSpellProto())/1000);            m_target->AddGameObject(pObj);
[b]MapManager::Instance().CreateMap(pObj->GetMapId(), pObj)[/b]->Add(pObj);

New code:

pObj->SetRespawnTime(GetSpellDuration(GetSpellProto())/1000);            m_target->AddGameObject(pObj);
[b]m_target->GetMap()[/b]->Add(pObj);

patch 1.14 http://filebeam.com/986887c9755f9cc0744c09ba6d47c986

patch 1.13 rev 8158: http://filebeam.com/eae0aae3ce42022a8cb573d5465d74a0

patch 1.10 rev 8103: http://filebeam.com/e38571440a68a8bc5361235dfbd866c6

patch 1.10 rev 8099: http://filebeam.com/774bf88af37cb0c2e7edb354d2b731e4

patch 1.10: http://filebeam.com/e30967cbd6a613751407b9c464521179

patch 1.09: http://filebeam.com/1424f8f721c70e2cdcafb553a4b0f092

Patch 1.08: http://filebeam.com/e030772048e2a21a947148cd2adfb431

Patch 1.07: http://filebeam.com/1349b226379bead1b97c09b9a6db2e39

Patch 1.06: http://filebeam.com/ead5afdf126daf180797233119b41146

Patch 1.05: http://filebeam.com/3f9c1afdb03690eeff5179449fed8ccf

[raczman]

Really nice patch, congratulations. We did point 1 in trinity some time ago, and maybe it didnt bring whole lot of performance, but now calling Unit->GetMap() doesnt make me shiver

[cozzmic]

That's awesome! Nice to hear that you guys are working on this. All I can say is, great job and gonna test a.s.a.p

[leak]

Do those changes affect the no.1 crashing reason, the famous GetInstance() bug, anyhow?

[Ambal]

Do those changes affect the no.1 crashing reason, the famous GetInstance() bug, anyhow?

Can you provide any details about 'famous GetInstance()' crash? I don't know if this patch fixes it, actually I wasn't informed that such issue even exists :huh:

P.S. Updated patch, included code suggestions by Wyk3d. See the first post.

[Ambal]

The reason why this can actually happen: incorrect usage of MapManager::GetMap(), which calls for MapInstanced::GetInstance() and crashes due to assertion failed. From now on function MapInstanced::GetInstance() (in patch its name MapInstanced::CreateInstance()) does not allow creatures to create instances so theoretically this crash has to go away. And I have to get a look into SD2 code, so it would work correctly with new WorldObject::GetMap()/MapManager::CreateMap() logic...

[Wyk3d]

The reason why this can actually happen: incorrect usage of MapManager::GetMap(), which calls for MapInstanced::GetInstance() and crashes due to assertion failed. From now on function MapInstanced::GetInstance() (in patch its name MapInstanced::CreateInstance()) does not allow creatures to create instances so theoretically this crash has to go away. And I have to get a look into SD2 code, so it would work correctly with new WorldObject::GetMap()/MapManager::CreateMap() logic...

I think I fixed all of the cases where GetMap was used incorrectly, they were very easy to spot. The crash that people still have long after that is because it is used correctly but some subsystem has deep hidden bugs. There is one in the spell system but a more common one is in the object update system. That one appears like the object is left in the update set after the map unloads, like it was not properly unloaded. So if the object's mapid/instanceid was not changed then its map pointer should not change either and then it might still crash, unfortunately that is harder to detect in order to print some debug info with this patch, and it will likely appear not in the getmap function but outside it, where the invalid pointer is used.

[Ambal]

Fixed a crash related to BG/Arena spawning code. Also another crash fixed in Player::LoadFromDB(). First post updated.

[leak]

There is also the one caused by players and not sd2 spawned npcs:

http://paste2.org/p/234378

**edit

2009-05-26 10:45:46 ERROR:GetInstance: object Playername(1234567), typeId 4, in world 0, should be in map 572,1581 but that's not loaded yet.

[Ambal]

There is also the one caused by players and not sd2 spawned npcs:

Do you have any assertion failed messages in server console? If so, could you print them here? Looks like we are trying to get a map which does not exists anymore, cause player has InstanceId != 0.

P.S. Like was said before, all these crashes have the same source: map object destroyed, but players/mobs still have reference to it...

[DasBlub]

ambal/wyk3d: will your changes be commited to the mangos repo or is this 'just' another patchfile which we need to apply by ourself?

however, the description looks nice, i didn't gave a look at the patch till now.

[Ambal]

It will be applied into repo as soon as tests prove that it is free from bugs and crashes.

[nanounico]

testing with the latest rev. 7928 ^^

[Ambal]

Do not forget to post any crashdumps/callstacks/console output you get with current patch/ It would help debugging greately.

[higi]

awesome, big test will come soon . is there any aditional mangosd.conf changes like in mtmaps etc?

is multithreading part in patch yet? i need to use it for 2x quade core. i can provide crash dumps etc.. (valgrind, later our valgrind (second)test server is down:( )

finaly some performance changes, derex also making multimangos, but he didnt posted mutch info about it yet(

[Ambal]

is there any aditional mangosd.conf changes like in mtmaps etc?

is multithreading part in patch yet? finally some performance changes, derex also making multimangos, but he didn't posted much info about it yet(

No, there are no additional fields in mangos.conf yet, as well as no multithreading. There will be several patches with different improvements. Current one changes the way how every object references maps in world, it supposed to lower CPU usage.

We also have found clues, that map unloading causes crashes, repoorted by members in this topic, so we are going to fix this too in nearest time.

I think multithreading will be the last patch in between map system patches. I will present very small profiling patch to figure out approximate multithreading potential of current mangos core. Stay tuned

[leak]

I assume this patch doesn't work with the mtmaps patches flying around here?

[Ambal]

This patch can work with mtmaps, but currently mangos core crashes much faster with this patch because alot of security checks missing in code. An old way was too problem-hiding.

Regarding creature's guardians crashes: we nearly found the source of problem and trying to solve it.

[alexluana]

wow wonderfull...thank you..

I have a question...

Why did u chossed multithreading and not multiprocessing?

I don't know very well the differencies...

Is it better and does it offer more features? Easy to update in feature?

With respect,

Axel

[Ambal]

Why did u chossed multithreading and not multiprocessing?

Most devs agree that we should have MT first and then MP. Lets not start flamewars about what is better, just remember: 90% of private server admins have just 1 single machine. So MP would be waste of resources, spend on caching the same creature data etc.

BUT, we keep in mind MP when making current changes to the core. So it will not be a BIG problem, if someday team desides to move to MP

[XTZGZoReX]

BUT, we keep in mind MP when making current changes to the core. So it will not be a BIG problem, if someday team desides to move to MP

http://github.com/XTZGZoReX/multimangos/tree/master

[Ambal]

Patch updated, alot of IsInWorld checks were added to code to prevent some ops with players who are in stage of far teleporting. First post updated.

[divisorxx]

Most devs agree that we should have MT first and then MP. Lets not start flamewars about what is better, just remember: 90% of private server admins have just 1 single machine. So MP would be waste of resources, spend on caching the same creature data etc.

BUT, we keep in mind MP when making current changes to the core. So it will not be a BIG problem, if someday team desides to move to MP

100 % agree most users have 1 machine with powerfull cpu and without multithread MaNGOS just only a little peace. Thanks Ambal for your work... I test it as soon as possible