[Patch] Multi-Threaded AuctionHouseMgr

[Naicisum]

Heres a patch I kind of whipped togeather. I havent fully tested it out to see if it catches all the places, but for the most part, it seems to be working in small simple cases. I figure this will be the beginning part of it. What do you guys thnk?

diff --git a/src/game/AuctionHouseMgr.cpp b/src/game/AuctionHouseMgr.cpp
index ed8a059..95a94b6 100644
--- a/src/game/AuctionHouseMgr.cpp
+++ b/src/game/AuctionHouseMgr.cpp
@@ -35,6 +35,11 @@

#include "Policies/SingletonImp.h"

+#ifdef WIN32
+#include "ServiceWin32.h"
+extern int m_ServiceStatus;
+#endif
+
INSTANTIATE_SINGLETON_1( AuctionHouseMgr );

AuctionHouseMgr::AuctionHouseMgr()
@@ -47,6 +52,40 @@ AuctionHouseMgr::~AuctionHouseMgr()
        delete itr->second;
}

+void AuctionHouseMgr::run()
+{
+    ACE_DEBUG
+        ((LM_DEBUG, ACE_TEXT ("(%t) Handler Thread running\\n")));
+
+    runUpdate = false;
+    initLoading = false;
+
+    LoadAuctionItems();
+    LoadAuctions();
+
+    initLoading = true;
+
+    while (!World::IsStopped())
+    {
+        if (runUpdate)
+        {
+            Update();
+            runUpdate = false;
+        }
+
+        #ifdef WIN32
+        ACE_Based::Thread::Sleep(50);
+        #else
+        ACE_Based::Thread::Sleep(100);
+        #endif
+
+        #ifdef WIN32
+            if (m_ServiceStatus == 0) World::StopNow(SHUTDOWN_EXIT_CODE);
+            while (m_ServiceStatus == 2) Sleep(1000);
+        #endif
+    }
+}
+
AuctionHouseObject * AuctionHouseMgr::GetAuctionsMap( uint32 factionTemplateId )
{
    if(sWorld.getConfig(CONFIG_ALLOW_TWO_SIDE_INTERACTION_AUCTION))
@@ -278,6 +317,8 @@ void AuctionHouseMgr::SendAuctionExpiredMail( AuctionEntry * auction )

void AuctionHouseMgr::LoadAuctionItems()
{
+    ACE_DEBUG
+        ((LM_DEBUG, ACE_TEXT ("(%t) LoadAuctionItems Thread running\\n")));
    // data needs to be at first place for Item::LoadFromDB
    QueryResult *result = CharacterDatabase.Query( "SELECT data,itemguid,item_template FROM auctionhouse JOIN item_instance ON itemguid = guid" );

@@ -331,6 +372,8 @@ void AuctionHouseMgr::LoadAuctionItems()

void AuctionHouseMgr::LoadAuctions()
{
+    ACE_DEBUG
+        ((LM_DEBUG, ACE_TEXT ("(%t) LoadAuctions Thread running\\n")));
    QueryResult *result = CharacterDatabase.Query("SELECT COUNT(*) FROM auctionhouse");
    if( !result )
    {
diff --git a/src/game/AuctionHouseMgr.h b/src/game/AuctionHouseMgr.h
index 2775838..164f637 100644
--- a/src/game/AuctionHouseMgr.h
+++ b/src/game/AuctionHouseMgr.h
@@ -114,7 +114,7 @@ class AuctionHouseObject
        AuctionEntryMap AuctionsMap;
};

-class AuctionHouseMgr
+class AuctionHouseMgr : public ACE_Based::Runnable
{
    public:
        AuctionHouseMgr();
@@ -152,7 +152,12 @@ class AuctionHouseMgr

        void Update();

+        bool runUpdate;
+        bool initLoading;
+
    private:
+        void run();
+
        AuctionHouseObject  mHordeAuctions;
        AuctionHouseObject  mAllianceAuctions;
        AuctionHouseObject  mNeutralAuctions;
diff --git a/src/game/World.cpp b/src/game/World.cpp
index bf20ccf..7bf2501 100644
--- a/src/game/World.cpp
+++ b/src/game/World.cpp
@@ -1309,8 +1309,13 @@ void World::SetInitialWorldSettings()
    ///- Load dynamic data tables from the database
    sLog.outString( "Loading Auctions..." );
    sLog.outString();
-    auctionmgr.LoadAuctionItems();
-    auctionmgr.LoadAuctions();
+    ACE_DEBUG
+        ((LM_DEBUG, ACE_TEXT ("(%t) Main Thread running\\n")));
+    ACE_Based::Thread t(*new AuctionHouseMgr);
+    while(auctionmgr.initLoading == false)
+        ACE_Based::Thread::Sleep(100);
+    //auctionmgr.LoadAuctionItems();
+    //auctionmgr.LoadAuctions();
    sLog.outString( ">>> Auctions loaded" );
    sLog.outString();

@@ -1529,7 +1534,8 @@ void World::Update(uint32 diff)
        }

        ///- Handle expired auctions
-        auctionmgr.Update();
+        auctionmgr.runUpdate = true;
+        //auctionmgr.Update();
    }

    /// [*] Handle session updates when the timer has passed

[mfour]

Sorry for obviously asking a stupid question, but what is this patch supposed to do ? I am not quite following your meaning.

[pelle]

Sorry for obviously asking a stupid question, but what is this patch supposed to do ? I am not quite following your meaning.

this should improve performances of the ahbot patch... right?

[painejake]

this should improve performances of the ahbot patch... right?

Nothing to do with ahbot. By the look of it and the title, its multi-threading the auction house manager.

[pelle]

Nothing to do with ahbot. By the look of it and the title, its multi-threading the auction house manager.

right sorry .

[Naicisum]

Yeah, it will basically split out the cpu cycles and work to another thread to work on. But just for the AuctionHouseMgr class. So for instance if you have like 20000 auctions that expired, instead of holding the entire server up while all those process, it queues the update which processes in another thread and the server continues to function normally. The ACE_DEBUG lines are for outputting info messages which show the thread number that its running under to verify its working. Those are not needed for the final version.

It was late when i posted, wanted to get it out there before i fell asleep. It could use a little revision and cleanup, but if anyone was interested in testing this (keep in mind this is completely experimental and i give no guarentees of stability) then give it a shot.

I am going to stress test it with the AuctionHouse Bot later on and watch its performance, but since this is such a change to core, i felt it needed to be submitted as a seperate modification all togeather.

@davemm/moderator

Could you rename this thread to: [Patch] Threading AuctionHouseMgr

its not necessarily multi-threading the AuctionHouseMgr but rather having that process in a seperate single thread.

[mfour]

Thanks, now I get it, sounds pretty good.

First I need to get rid of client 132 errors before testing any new stuff

[Milk13]

Thx

I will try with other stuff.

[Derex]

Nice idea, but unfortunatelly its not that simple to implement such thing, you open some thread synchronization issues.

[xeross155]

Once this gets passed as working correctly I'll use it.

[Naicisum]

@Derex

I havent begin heavy testing to see it break yet, but sync errors such as? Do you see any glaring issues?

Since the class is implemented as a Singleton and Instanced only once as long as its referenced with auctionmgr key word, it will all use the same class object. Or so i thought was my understanding.

It's all a learning experience to me, but i would love to develop this idea to one day get fully implemented.

[Derex]

The problem is that AuctionHouseMgr::Update which you call from the new thread calls code which is accessing shared memory ( Players, Mails etc ). If 2 threads access/modify the same piece of memory without synchornization, it can cause race condition.

You can read wikipedia what is race condition, what can cause it and how to avoid it.

[Naicisum]

So the solution would be to implement a Mutex or Guard around those objects yes? Im looking into it.

[freghar]

Some kind of a lock (mutex-based, spinlock, semaphores, ..) or atomic operation(s).

[Derex]

Unfortunatelly, the solution will be to make almost entire mangos thread safe. Since all the code called in your new thread needs to be safe. And in your new thread you call ::Update which accesses players, sends mails etc.

[xzbbzx]

Implementing a mutex would be quite simple. You wouldn't even need to touch the rest of the core. Just put the mutex in the AuctionHouseMgr where the code which makes the calls to other functions is, and that would be it.

[XTZGZoReX]

@xzbbzx: Yes, but that beats the purpose of this entire patch.

[xzbbzx]

Not exactly. From what I've read in the thread (haven't looked at core closely), if there are many auctions that have expired, the whole server will be blocked until it finishes. Doing it as a separate thread will let the let of the server continue with its stuff while a separate thread will be working with the auction stuff.

[XTZGZoReX]

Yes... I'm aware, but that was not my point. My point is that a mutex is blocking. Thus, adding a mutex to AuctionMgr::run would just make it block the rest of the server - in other words, make the whole patch pointless.

[xzbbzx]

No, it would block the actual thread until there was no other thread in that part of the code. The "primary thread", the one running the actual server wouldn't be blocked.

[XTZGZoReX]

Then I have no clue what you're talking about...

There's only ONE thread that's going to access AuctionMgr::run, and that's its actual thread. So why put a mutex there to begin with? I don't think you get what a mutex does, or, you aren't explaining yourself properly.

[Naicisum]

I beleive a combination would be needed, a mutex that locks and unlocks after one iteraition or update (not talking specific code sections, just in general) and then once it unlocks, a Yield statement which will allow other threads to run. Since these operations would happen so fast, it would accomplish both needs. The server continues processing and no sync issues happen with the mutex in place.

[Derex]

Hello,

I suggest you run your patch with valgrind --tool=helgrind, I think it will show you the possible race conditions.

[xeross155]

I don't see how this would have race conditions as only 1 thread will be the ah thread, so threads can't conflict.

[maikash]

The problem is that AuctionHouseMgr::Update which you call from the new thread calls code which is accessing shared memory ( Players, Mails etc ). If 2 threads access/modify the same piece of memory without synchornization, it can cause race condition.

filler.. .. . . ..