feat(Core/AuctionHouse): Rework auctionhouse search threading #20830

Takenbacon · 2024-12-03T18:34:47Z

Complete rework of the auctionhouse search threading as the old system has been proven to be obviously very unsafe (#20538).

Simple model, spawns a configurable number of worker threads that each contain a pointer to a shared copy of AuctionEntry with an almost completely decoupled data structure (except for a pointer to ItemTemplate, which will be fine as long as we don't actively invalidate ItemTemplate data during run time. This can be "fixed" if needed by copying relevant item template data at the cost of increased memory usage and inability to modify existing data).

Scales well and is safe. I have used a similar model on servers with xx,xxx+ players and 150k+ auctions, however the implementation is quite different. I have done some initial local testing but I would highly recommend more testing before any merging. No danger to auctionhouse item data integrity during as we do not modify any auction entry data. Compile tested using MSVC only currently, GCC/Clang untested.

The auctionhouse system itself could use more changes but I kept the scope of this PR limited to just the searches.

Changes Proposed:

This PR proposes changes to:

Core (units, players, creatures, game systems).
Scripts (bosses, spell scripts, creature scripts).
Database (SAI, creatures, etc).

Issues Addressed:

Closes Feature: Rewrite Auction House search #20538

SOURCE:

The changes have been validated through:

Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
Sniffs (remember to share them with the open source community!)
Video evidence, knowledge databases or other public sources (e.g forums, Wowhead, etc.)
The changes promoted by this pull request come partially or entirely from another project (cherry-pick). Cherry-picks must be committed using the proper --author tag in order to be accepted, thus crediting the original authors, unless otherwise unable to be found

Tests Performed:

This PR has been:

Tested in-game by the author.
Tested in-game by other community members/someone else other than the author/has been live on production servers.
This pull request requires further testing and may have edge cases to be tested.

How to Test the Changes:

This pull request can be tested by following the reproduction steps provided in the linked issue
This pull request requires further testing. Provide steps to test your changes. If it requires any specific setup e.g multiple players please specify it as well.

Simply use the auctionhouse (specifically anything related to searches, owned and bidded auction lists)

Known Issues and TODO List:

None currently.

[ ]
[ ]

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

Always update worker threads to prevent possible resource leaks on empty/idle servers Remove redundant SearchableAuctionEntry list from AuctionHouseSearcher

These changes, while good, aren't currently necessary. Restore back to original to limit scope of PR. Will reimplement later with some changes.

and a little bit of misc refactoring to standardize some notation

heyitsbench · 2024-12-03T19:07:13Z

Looks like this also closes #5068.

src/server/game/AuctionHouse/AuctionHouseMgr.h

…t happy

Takenbacon · 2024-12-04T00:25:37Z

Looks like I finally got most of the build check issues resolved, only problem remaining appears to be a couple small compatibility issues with ahbot, in which it'd probably be best just to update the ahbot module.

sudlud · 2024-12-04T08:30:18Z

Thanks for the update, could you please also prepare a PR for ahbot? Then we could get both merged when everything's ready :)

walkline

I think you already know that I love this implementation 😁 I just have a few small nitpicks (some of them related to the old code).

walkline · 2024-12-03T22:56:07Z

src/server/game/World/World.cpp

+    METRIC_TIMER("world_update_time", METRIC_TAG("type", "Update expired auctions"));
+    sAuctionMgr->Update(diff);


I think we still need to put this code in an anonymous scope to measure the metric correctly.

Done. 6cb4d9c

walkline · 2024-12-04T08:22:01Z

src/server/game/AuctionHouse/AuctionHouseSearcher.cpp

+
+    AuctionSearcherResponse* searchResponse = new AuctionSearcherResponse();
+    searchResponse->playerGuid = searchBidderListRequest.ownerGuid;
+    searchResponse->packet.Initialize(SMSG_AUCTION_BIDDER_LIST_RESULT, (4 + 4 + 4) + 30000); // pussywizard: ensure there is enough memory


I know that this change (upfront allocation of 30 012 bytes) doesn't come from your side, but does it make sense to you? I thought the average bidder list shouldn't be that long.
On the other hand, I think upfront allocation can be useful for getAll requests.

I did see that but opted to not change it at the time, but yes, it's a bit excessive and ultimately useless. At 148 bytes per auction entry it's effectively preallocating the buffer size for ~202 bidder auction entries which is unlikely to be the case 99% of the time. It's a bit insane to pre-optimize for ultra edge cases especially when the optimization is overall extremely insignificant. Also, we don't do the same for either SMSG_AUCTION_OWNER_LIST_RESULT or SMSG_AUCTION_LIST_RESULT. As such, I have now removed it ed83ecd

walkline · 2024-12-04T08:28:33Z

src/server/game/AuctionHouse/AuctionHouseSearcher.cpp

+    {
+        for (auto const& pair : auctionMap)
+            auctionEntries.push_back(pair.second.get());
+    }


Small nitpick: if we add return here, we can get rid of the else statement below.

Done. f2e9608

walkline · 2024-12-04T08:41:22Z

src/server/game/Handlers/AuctionHouseHandler.cpp

        else
        {
            CreatureData const* auctioneerData = sObjectMgr->GetCreatureData(creature->GetSpawnId());
            if (!auctioneerData)
            {
                LOG_ERROR("network.opcode", "Data for auctioneer not found ({})", auctioneer.ToString());
+                delete AH;


Good catch!

walkline · 2024-12-04T08:52:19Z

src/server/game/Handlers/AuctionHouseHandler.cpp

-    const Milliseconds delay = 2s;
-    const Milliseconds now = GameTime::GetGameTimeMS();
-    Milliseconds diff = GetMSTimeDiff(_lastAuctionListItemsMSTime, now);
-    if (diff > delay)


I was thinking maybe we could add rate limiter for some auction house opcodes somewhere here. What do you think? Maybe we can address it in a separate PR if needed.

The auction list opcodes definitely should be throttled, in fact there's even a blizzlike client side delay that we add to the list packets (now defined as AUCTION_SEARCH_DELAY) but that's obviously only on the client side. Personally, I've seen the antidos system and I can't say I'm a huge fan of how it works for a variety of reasons that would take too long to elaborate on, but I suppose it's the best we have for now. It could still use some refinement if we had a better system but as a temporary measure I've added them 2a51d56

CMSG_AUCTION_LIST_ITEMS is still lacking the blizzlike throttle for "getAll" requests (something around ~15 minutes if I recall correctly?). I don't believe it to be too important at the moment but it should also be implemented at some point. (Note: If someone other than me eventually implements it, there's a "gotcha" that CMSG_AUCTION_LIST_ITEMS is currently handled as PROCESS_THREADSAFE, any global containers used in the handler must be locked or the opcode moved to PROCESS_THREADUNSAFE)

Yeah, agree. Thanks for addressing the comments!

TheSCREWEDSoftware · 2024-12-05T15:20:00Z

@Takenbacon just so I know if i need to update the labels. Is this ready to be tested or still being changed/developed?

Takenbacon · 2024-12-05T16:28:31Z

@Takenbacon just so I know if i need to update the labels. Is this ready to be tested or still being changed/developed?

Ready to be tested, nothing left on my end except for any requested changes during reviews/testing that may come up.

walkline · 2024-12-08T13:47:33Z

I ran a small smoke test - no issues were found.

Grimdhex · 2024-12-10T11:04:57Z

Why use std::wstring instead string ? Most of the core manipulate char and not wchar_t. Even with OS differences, that shouldn't be an issue ? Does I'm wrong ?

Takenbacon · 2024-12-10T12:24:29Z

Why use std::wstring instead string ? Most of the core manipulate char and not wchar_t. Even with OS differences, that shouldn't be an issue ? Does I'm wrong ?

Mostly because that is what was there in the original implementation. Use of a wide string is not new from this PR.

azerothcore-wotlk/src/server/game/Misc/AsyncAuctionListing.cpp

Line 56 in 69c85ca

if (!Utf8toWStr(_searchedname, wsearchedname))

Now, why we use a wide string in general? It's going to be for supporting the various languages/character encoding inputs received from the client. But, I couldn't really give you the specifics and I can't say I've bothered to really look into it. Character encoding is not a particular strong suit of mine as it is a confusing mess and varies significantly. You'll notice Utf8toWStr has quite a few uses in the codebase, mostly converting client string inputs (who list, channels, chat, etc)

Nyeriah · 2024-12-12T14:09:43Z

Please check the merge conflicts
Also we need someone to please take a look into the AH bot module so we can merge it (the CI is failing)

Takenbacon · 2024-12-12T14:40:22Z

Please check the merge conflicts
Also we need someone to please take a look into the AH bot module so we can merge it (the CI is failing)

Conflicts resolved.

Kitzunu · 2024-12-18T17:07:54Z

Awesome sauce

Nyeriah · 2024-12-19T03:06:33Z

https://github.com/azerothcore/mod-eluna needs an update

…hcore#20830)

…azerothcore#20830)" This reverts commit 4d349e8

…hreading (azerothcore#20830)"" This reverts commit e2b2b13.

Takenbacon added 9 commits November 19, 2024 05:15

Initial work on AH threading rewrite - WIP

02c586e

Couple changes based on feedback

e8ff193

Always update worker threads to prevent possible resource leaks on empty/idle servers Remove redundant SearchableAuctionEntry list from AuctionHouseSearcher

Restore original auctionEntry item handling

e23e31a

These changes, while good, aren't currently necessary. Restore back to original to limit scope of PR. Will reimplement later with some changes.

Handle SMSG_AUCTION_OWNER_LIST_RESULT, SMSG_AUCTION_BIDDER_LIST_RESULT

d3e230c

and a little bit of misc refactoring to standardize some notation

Add worker thread config

77cd878

Move timer updating of AuctionHouseMgr to fix delay in thread responses

61b83b8

Fix mistake with usable flag optional

e6aaca9

Fix restoring bidder info on realm restart

e644c70

Merge branch 'azerothcore:master' into ah_threading

d2fac12

github-actions bot added CORE Related to the core file-cpp Used to trigger the matrix build labels Dec 3, 2024

Nyeriah added Ready to be Reviewed Waiting to be Tested labels Dec 3, 2024

heyitsbench reviewed Dec 3, 2024

View reviewed changes

src/server/game/AuctionHouse/AuctionHouseMgr.h Show resolved Hide resolved

Takenbacon added 6 commits December 3, 2024 11:14

Fix GCC compile warning

04cb0de

Missing timer include

1c52e6b

Still working on making the build tools happy...

e1cd9f7

Fix gcc enumeration warning

06a7acc

Add back GetAuctionOutBid helper to fix ahbot compatibility

ac5a73e

Modify how we stop the worker threads to make the dry run build scrip…

c7f2b8c

…t happy

walkline reviewed Dec 4, 2024

View reviewed changes

Takenbacon added 4 commits December 4, 2024 01:53

Scope AH update call for proper metrics

6cb4d9c

Remove useless preexisting micro-optimization

ed83ecd

Small codestyle change

f2e9608

Add auction list opcodes to antidos system

2a51d56

Takenbacon mentioned this pull request Dec 4, 2024

Add compatibility with AH threading PR request azerothcore/mod-ah-bot#124

Merged

jerryshell mentioned this pull request Dec 8, 2024

feat(Core/AuctionHouse): Implement Auction House getAll scan #20511

Closed

12 tasks

Nyeriah approved these changes Dec 9, 2024

View reviewed changes

Grimdhex approved these changes Dec 10, 2024

View reviewed changes

Merge branch 'master' into ah_threading

dc8b75f

oops lol

c921b61

Grimdhex mentioned this pull request Dec 18, 2024

Item storage rework #20961

Closed

8 tasks

Takenbacon added 2 commits December 18, 2024 05:38

Merge branch 'master' into ah_threading

07746fc

Add comment for MAX_GETALL_RETURN

85e0903

sudlud approved these changes Dec 18, 2024

View reviewed changes

sudlud merged commit 4d349e8 into azerothcore:master Dec 18, 2024
12 of 13 checks passed

Grimdhex mentioned this pull request Dec 19, 2024

Feature: Auction House getAll scan #5068

Closed

xpdemon pushed a commit to xpdemon/azerothcore-wotlk that referenced this pull request Dec 25, 2024

feat(Core/AuctionHouse): Rework auctionhouse search threading (azerot…

650be45

…hcore#20830)

skelUA added a commit to skelUA/wotlk that referenced this pull request Jan 19, 2025

Revert "feat(Core/AuctionHouse): Rework auctionhouse search threading (…

e2b2b13

…azerothcore#20830)" This reverts commit 4d349e8

skelUA added a commit to skelUA/wotlk that referenced this pull request Jan 24, 2025

Revert "Revert "feat(Core/AuctionHouse): Rework auctionhouse search t…

ac25aca

…hreading (azerothcore#20830)"" This reverts commit e2b2b13.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Core/AuctionHouse): Rework auctionhouse search threading #20830

feat(Core/AuctionHouse): Rework auctionhouse search threading #20830

Takenbacon commented Dec 3, 2024 •

edited

Loading

heyitsbench commented Dec 3, 2024

Takenbacon commented Dec 4, 2024

sudlud commented Dec 4, 2024

walkline left a comment

walkline Dec 3, 2024

Takenbacon Dec 4, 2024

walkline Dec 4, 2024

Takenbacon Dec 4, 2024

walkline Dec 4, 2024

Takenbacon Dec 4, 2024

walkline Dec 4, 2024

walkline Dec 4, 2024

Takenbacon Dec 4, 2024

walkline Dec 4, 2024

TheSCREWEDSoftware commented Dec 5, 2024

Takenbacon commented Dec 5, 2024

walkline commented Dec 8, 2024

Grimdhex commented Dec 10, 2024

Takenbacon commented Dec 10, 2024

Nyeriah commented Dec 12, 2024 •

edited

Loading

Takenbacon commented Dec 12, 2024 •

edited

Loading

Kitzunu commented Dec 18, 2024

Nyeriah commented Dec 19, 2024

		METRIC_TIMER("world_update_time", METRIC_TAG("type", "Update expired auctions"));
		sAuctionMgr->Update(diff);

feat(Core/AuctionHouse): Rework auctionhouse search threading #20830

feat(Core/AuctionHouse): Rework auctionhouse search threading #20830

Conversation

Takenbacon commented Dec 3, 2024 • edited Loading

Changes Proposed:

Issues Addressed:

SOURCE:

Tests Performed:

How to Test the Changes:

Known Issues and TODO List:

How to Test AzerothCore PRs

heyitsbench commented Dec 3, 2024

Takenbacon commented Dec 4, 2024

sudlud commented Dec 4, 2024

walkline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSCREWEDSoftware commented Dec 5, 2024

Takenbacon commented Dec 5, 2024

walkline commented Dec 8, 2024

Grimdhex commented Dec 10, 2024

Takenbacon commented Dec 10, 2024

Nyeriah commented Dec 12, 2024 • edited Loading

Takenbacon commented Dec 12, 2024 • edited Loading

Kitzunu commented Dec 18, 2024

Nyeriah commented Dec 19, 2024

Takenbacon commented Dec 3, 2024 •

edited

Loading

Nyeriah commented Dec 12, 2024 •

edited

Loading

Takenbacon commented Dec 12, 2024 •

edited

Loading