[10848] Multithreaded packet processing

[Auntie Mangos]

Has i connot see any diff (realy little server) and in this case this patch has only risk to create any new unknow situation with some new development.

On small servers you basically won't see any performance bonuses with any patches just because load is very low. Also, currently packet processing takes alot of time only if you use vmaps/mmaps and any other CPU intensive code. W/o them packet processing is relatively low CPU consumer and with this patch http://getmangos.eu/community/topic/15594/performance-visibility-updates-and-relocations/ it will take only ~10% of overall server load in single-threaded mode.

Cheers

[Ambal]

Hi everyone.

Another essential patch for mangos core. This time we are going to implement packet processing schedule algorithm which allows us to handle some packets in multi-threaded mode, e.g. in Map::Update() function (ofc if you have mtmaps patch installed). Patch idea goes to mangos core team and implementation goes to me

Why do we need this? If packet is thread-safe to process it in multi-threaded mode - why not do it? For example, movement packets turned to be ~60% of overall packets processed by the server and they are not only HUGE CPU consumers (vmaps, mmaps, relocations, creature AI etc), they are also THREAD-SAFE because their handlers are safe to run in parallel.

So to achieve this task, packet scheduling classification was added:

enum PacketProcessing

{

PROCESS_INPLACE = 0, //process packet whenever we receive it - mostly for non-handled or non-implemented packets

PROCESS_THREADUNSAFE, //packet is not thread-safe - process it in World::UpdateSessions()

PROCESS_THREADSAFE //packet is thread-safe - process it in Map::Update() if player is in world

};

In Opcode.cpp file you will find how packets are set to be processed. Currently, there are mostly movement opcodes are set to be processed in Map::Update() function calls. We did not tried to inspect ALL existing packet handlers for their safety since it is nearly impossible to achieve it in small amount of time. So you are free to investigate further and submit your opinions about packet thread-safety

It is highly recommended to have a mtmaps installed in order to test current patch, but if you have plenty of players - you can go on w/o installing mtmaps. All we need is to find out if player teleports/logouts do not cause any problems when happen in Map::Update() function calls. Feel free to post you comments, test results and suggestions.

UPDATE: please, note, even if packet is set to be PROCESS_THREADSAFE but player is not in world (for example, is teleporting) - this packet will be processed like PROCESS_THREADUNSAFE in World::UpdateSessions()!

Have fun

Repo: https://github.com/Ambal/mangos

[DaemonCantor]

Now that's a great Idea, Kinda like running 32bit at the same time as 64bit thru multiple cores and improving overall efficiency and stability.

[awkwarddev]

So, basically this is multi-threading engine for MaNGOS, such as Trinity's one?

[darkstalker]

don't know if its the same topic, but is this related to chat messages lost when you're on loading screen?

[Ambal]

don't know if its the same topic, but is this related to chat messages lost when you're on loading screen?

this patch is not meant to fix issues with existing packet handlers. So if chat messages are lost - they are still lost...

[Ambal]

So, basically this is multi-threading engine for MaNGOS, such as Trinity's one?

no, this is not a multithreaded engine - its purpose is to improve core scalability when you have mtmaps patch installed. Mangos core is not officially multithreaded.

[XdK]

Nice one. You are doing great work on making MaNGOS faster, keep it up

[Patman128]

Is there any specific MTMaps branch/patch you recommend, or just the latest one in the thread?

[awkwarddev]

But multithreading should make the core more stable and less lagful, and of course increase the player limits and uptime?

[darkstalker]

But multithreading should make the core more stable and less lagful, and of course increase the player limits and uptime?

Multithreading would make core less stable (most of core isn't thread safe) and more scalable (less "lagful" on high player count).

[cyberium]

Seem's to work well but, ...

For my little server i don't see any impact.

good :

- Probably reduce packet processing time. (need to be verified with before/after patched on large server)

- Like said if overal moving will be improved this patch is essential.

not good :

- Hard to detect bug caused by this so it must be verified alot before applied for some opcodes.

Thanks for your good works.

[Ambal]

Is there any specific MTMaps branch/patch you recommend, or just the latest one in the thread?

No specific mtmaps patch is needed - just pick up that which works for you

[Ambal]

good :

- Probably reduce packet processing time. (need to be verified with before/after patched on large server)

- Like said if overal moving will be improved this patch is essential.

not good :

At current stage you should verify if this patch does not degrade packet processing time because of internal scheduling. You should check server uptime too since if you start getting weird crashes - it means some packets are not threadsafe OR teleport logic started to become broken. So grab all your mtmaps/vmaps//mmaps patches you have and write your findings here

Also, I hardly understand what "if overal moving will be improved" means since packet scheduling covers ALL sorts of opcodes and not only movement packets - in future we want to parallelize as many packets as mangos core will allow us. Currently we cannot afford to parallelize ALL opcodes by moving ALL packet processing into Map::Update() - this is very unsafe and it will definitely either make your server crash often OR will lag it to the death

Player/object movement is totally different field of logic so it has to be improved in specific patches.

P.S. We should also verify possibility to parallelize processing of all "messaging" packets ( WorldSession::HandleMessagechatOpcode() ) - they seems to be heavy too.

Cheers to everyone

[qsa]

First of all, I have to admit, that I'm not fully understand the threading mechanism used by different thread patches. Never had the opportunity to check it.

If I'm asking something silly, please do explain. This will sure help me (and others) understand the model.

This said, I have a very basic question about this, and similar patches:

How can it be thread safe without actually locking shared objects?

For example, movement packets (per thread per map like in this patch) manipulate player objects. What assures us that same object cannot be manipulated from other thread simultaneously? For example, by chat handler.

I really fail to see how being in world ("belonging" to some map) makes it thread safe.

I know this is bad example, due to granularity, but lets assume our granularity is player object and not its fields. Since, if you count on fact, that different threads change different parts, you have to be 100% sure you checked ALL the fields involved. This is, far, far from trivial. If they don't, we don't need this patch at all.

So, the way I see it, we can get undefined values due to R/W, W/R, W/W, basically everything but R/R.

Assuming store/loads are atomic don't help either (they are not).

Thanks in advance.

[Ambal]

How can it be thread safe without actually locking shared objects?

For example, movement packets (per thread per map like in this patch) manipulate player objects. So, the way I see it, we can get undefined values due to R/W, W/R, W/W, basically everything but R/R.

In theory you are right, qsa, since w/o reader_writer locks or atomics it is not possible to guarantee thread safety of methods and consistency of data. But this actually happens ONLY if you read and/or write from ANY thread at the same time.

What if you manipulate object's private data in only one thread while other ones can't access your object? For example, MovementInfo is a private data for Unit object and it cannot be manipulated from other threads - why would we need locks in this case when ONLY ONE thread can modify it, e.g. that one which is calling Map::Update() function? Do we need locks if we want to send wispers to players on any map/group<= this is read operations as soon as you can confirm your group is loaded and player is loaded in the world?

So to make things simple:

Threading model for mangos:

thread_1:

World::UpdateSessions()

...

join();

...

thread_1 thread_2 ............. thread_N

map_1->Update() map_2->Update() map_X->Update()

...

join()

...

World::UpdateSessions() <- single-threaded execution, ALL sorts of read-writes allowed

Map::Update() - this is where we process 'thread-safe' packet handlers. writes/reads allowed on data which is either not accessible from other threads OR it is 'read-only' OR protected by locks.

So it is your responsibility to check if data accessed by handler stays const OR not accessed from other threads while you are changing it when you want to execute packet handlers in Map::Update() by setting their PacketProcessing type to PROCESS_THREADSAFE!!

Is it explains enough for you, qsa?

[qsa]

This is how I imagined this is done.

Thanks for the explanation.

You assume there's no R/W shared data between the threads, therefore it is safe.

My concern, as explained in previous post, is that there are.

What I'm really saying here, is that determining not safe data relations between the threads is 99% of things that have to be done to make it multi-threaded. Things may appear working, but, once in a while you get data corruption - those things are next to impossible to debug.

oh, well, if it works, it works.

Thanks once again.

[Ambal]

You assume there's no R/W shared data between the threads, therefore it is safe. What I'm really saying here, is that determining not safe data relations between the threads is 99% of things that have to be done to make it multi-threaded.

Qsa, you know what it takes before you can claim that your methods are thread-safe, aren't you You have to protect your data with mutexes/atomics or make sure your data is 'read-only' if you don't use locks... and in the end don't forget to CHECK if you haven't missed something

Mangos core is designing in such way that multiple Map::Updates() could run in parallel, so more and more functions/managers/data are made thread-safe and put inside this function call... In the end in World::Update() we should end up with just 2 functions:

1) World::UpdateSessions()

2) Map::Update()

If you'll have more questions, I'll try to answer them. Cheers

[SilverIce]

As i can see, you have recognized movement packets handlers as thread safe -

almost all _QUERY_* handlers are threadsafe too.. most of them are constant queries to ObjectManager's storage

[Ambal]

almost all _QUERY_* handlers are threadsafe too.. most of them are constant queries to ObjectManager's storage

Yes, it seems so. There are alot of packet handlers which might be already thread-safe, but lets finish with movement handling first. My biggest concern is taxi packet handler which is using TeleportTo() function call - I'm not sure if we have taxies teleporting to other maps (or this packet is for transports too?).

We can review the rest of packet handlers one-by-one and submit smaller patches once we are sure packet handlers are safe to call from Map::Update()

[SilverIce]

I'm not sure if we have taxies teleporting to other maps (or this packet is for transports too?)

No, transport teleports players in Transopt::Update (currently its safe, because it called from one thread)

There are a lot of taxi paths that teleports players to another map

[lukaasm]

full day test with 2.5k online. No crashes, no weird behaviour, lower update time diff by 40-50 ? mtmaps based on ace ;]

[divisorxx]

Testing with 700 users... not crash at all, diff time seems to be a little lower but not too much, maybe on more populate servers will be better. Thanks Ambal!

Edit: Still not crashing (8 hours uptime and counting) and I forgot to say I'm using ACE mtmaps.

[Ambal]

Patch updated: now all taxi packet handlers are processed in Map::Update(). Check newest changes as if everything will be OK - this should be the last patch version before submit.

Thanks to everyone making tests and/or posting their feedback

UPDATE: I think we should try and collect some statistics about amount of each opcode received to see what packet handlers might need to be parallelized in order to improve performance... I'll submit a small patch which will write output file with all info we need. Stay tuned

[divisorxx]

Testing with new changes, 700 users online with ACE mtmaps.... same than before, not a single crash related (12 hs uptime and counting) and diff time seems to be a little lower but, again, not too much, sure on more populate servers will be better. Thanks Ambal for your great work!!!!!