Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: optimize getInbox usage and shared pointer handling in loops #3150

Merged
merged 1 commit into from
Nov 27, 2024
+60 −28
Merged

perf: optimize getInbox usage and shared pointer handling in loops #3150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/creatures/players/player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5039,10 +5039,14 @@ ItemsTierCountList Player::getDepotInboxItemsId() const {
ItemsTierCountList itemMap;

const auto &inboxPtr = getInbox();
const auto &container = inboxPtr->getContainer();
const auto &container = inboxPtr ? inboxPtr->getContainer() : nullptr;
if (container) {
for (ContainerIterator it = container->iterator(); it.hasNext(); it.advance()) {
const auto &item = *it;
if (!item) {
continue;
}

(itemMap[item->getID()])[item->getTier()] += Item::countByType(item, -1);
}
}
Expand Down Expand Up @@ -9073,7 +9077,7 @@ void Player::forgeFuseItems(ForgeAction_t actionType, uint16_t firstItemId, uint

returnValue = g_game().internalAddItem(static_self_cast<Player>(), exaltationContainer, INDEX_WHEREEVER);
if (returnValue != RETURNVALUE_NOERROR) {
g_logger().error("Failed to add exaltation chest to player with name {}", fmt::underlying(ITEM_EXALTATION_CHEST), getName());
g_logger().error("Failed to add exaltation chest to player with name {}", getName());
sendCancelMessage(getReturnMessage(returnValue));
sendForgeError(RETURNVALUE_CONTACTADMINISTRATOR);
return;
Expand Down
34 changes: 21 additions & 13 deletions src/game/game.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9025,7 +9025,7 @@ void Game::playerCreateMarketOffer(uint32_t playerId, uint8_t type, uint16_t ite
return;
}

std::shared_ptr<DepotLocker> depotLocker = player->getDepotLocker(player->getLastDepotId());
const std::shared_ptr<DepotLocker> &depotLocker = player->getDepotLocker(player->getLastDepotId());
if (depotLocker == nullptr) {
offerStatus << "Depot locker is nullptr for player " << player->getName();
return;
Expand Down Expand Up @@ -9108,6 +9108,7 @@ void Game::playerCancelMarketOffer(uint32_t playerId, uint32_t timestamp, uint16
return;
}

const auto &playerInbox = player->getInbox();
if (offer.type == MARKETACTION_BUY) {
player->setBankBalance(player->getBankBalance() + offer.price * offer.amount);
g_metrics().addCounter("balance_decrease", offer.price * offer.amount, { { "player", player->getName() }, { "context", "market_purchase" } });
Expand All @@ -9124,10 +9125,11 @@ void Game::playerCancelMarketOffer(uint32_t playerId, uint32_t timestamp, uint16
player->getAccount()->addCoins(CoinType::Transferable, offer.amount, "");
} else if (it.stackable) {
uint16_t tmpAmount = offer.amount;

while (tmpAmount > 0) {
int32_t stackCount = std::min<int32_t>(it.stackSize, tmpAmount);
const auto &item = Item::CreateItem(it.id, stackCount);
if (internalAddItem(player->getInbox(), item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
if (internalAddItem(playerInbox, item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
break;
}

Expand All @@ -9147,7 +9149,7 @@ void Game::playerCancelMarketOffer(uint32_t playerId, uint32_t timestamp, uint16

for (uint16_t i = 0; i < offer.amount; ++i) {
const auto &item = Item::CreateItem(it.id, subType);
if (internalAddItem(player->getInbox(), item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
if (internalAddItem(playerInbox, item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
break;
}

Expand Down Expand Up @@ -9205,35 +9207,41 @@ void Game::playerAcceptMarketOffer(uint32_t playerId, uint32_t timestamp, uint16
return;
}

const auto &playerInbox = player->getInbox();

uint64_t totalPrice = offer.price * amount;

// The player has an offer to by something and someone is going to sell to item type
// so the market action is 'buy' as who created the offer is buying.
if (offer.type == MARKETACTION_BUY) {
std::shared_ptr<DepotLocker> depotLocker = player->getDepotLocker(player->getLastDepotId());
const std::shared_ptr<DepotLocker> &depotLocker = player->getDepotLocker(player->getLastDepotId());
if (depotLocker == nullptr) {
offerStatus << "Depot locker is nullptr";
return;
}

std::shared_ptr<Player> buyerPlayer = getPlayerByGUID(offer.playerId, true);
const std::shared_ptr<Player> &buyerPlayer = getPlayerByGUID(offer.playerId, true);
if (!buyerPlayer) {
offerStatus << "Failed to load buyer player " << player->getName();
return;
}

if (!buyerPlayer->getAccount()) {
const auto &buyerPlayerAccount = buyerPlayer->getAccount();
if (!buyerPlayerAccount) {
player->sendTextMessage(MESSAGE_MARKET, "Cannot accept offer.");
return;
}

if (player == buyerPlayer || player->getAccount() == buyerPlayer->getAccount()) {
const auto &playerAccount = player->getAccount();
if (player == buyerPlayer || playerAccount == buyerPlayerAccount) {
player->sendTextMessage(MESSAGE_MARKET, "You cannot accept your own offer.");
return;
}

const auto &buyerPlayerInbox = buyerPlayer->getInbox();

if (it.id == ITEM_STORE_COIN) {
auto [transferableCoins, error] = player->getAccount()->getCoins(CoinType::Transferable);
auto [transferableCoins, error] = playerAccount->getCoins(CoinType::Transferable);

if (error != AccountErrors_t::Ok) {
offerStatus << "Failed to load transferable coins for player " << player->getName();
Expand All @@ -9245,7 +9253,7 @@ void Game::playerAcceptMarketOffer(uint32_t playerId, uint32_t timestamp, uint16
return;
}

player->getAccount()->removeCoins(
playerAccount->removeCoins(
CoinType::Transferable,
amount,
"Sold on Market"
Expand Down Expand Up @@ -9280,7 +9288,7 @@ void Game::playerAcceptMarketOffer(uint32_t playerId, uint32_t timestamp, uint16
while (tmpAmount > 0) {
uint16_t stackCount = std::min<uint16_t>(it.stackSize, tmpAmount);
const auto &item = Item::CreateItem(it.id, stackCount);
if (internalAddItem(buyerPlayer->getInbox(), item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
if (internalAddItem(buyerPlayerInbox, item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
offerStatus << "Failed to add player inbox stackable item for buy offer for player " << player->getName();

break;
Expand All @@ -9302,7 +9310,7 @@ void Game::playerAcceptMarketOffer(uint32_t playerId, uint32_t timestamp, uint16

for (uint16_t i = 0; i < amount; ++i) {
const auto &item = Item::CreateItem(it.id, subType);
if (internalAddItem(buyerPlayer->getInbox(), item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
if (internalAddItem(buyerPlayerInbox, item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
offerStatus << "Failed to add player inbox item for buy offer for player " << player->getName();

break;
Expand Down Expand Up @@ -9353,7 +9361,7 @@ void Game::playerAcceptMarketOffer(uint32_t playerId, uint32_t timestamp, uint16
const auto &item = Item::CreateItem(it.id, stackCount);
if (
// Init-statement
auto ret = internalAddItem(player->getInbox(), item, INDEX_WHEREEVER, FLAG_NOLIMIT);
auto ret = internalAddItem(playerInbox, item, INDEX_WHEREEVER, FLAG_NOLIMIT);
// Condition
ret != RETURNVALUE_NOERROR
) {
Expand Down Expand Up @@ -9381,7 +9389,7 @@ void Game::playerAcceptMarketOffer(uint32_t playerId, uint32_t timestamp, uint16
const auto &item = Item::CreateItem(it.id, subType);
if (
// Init-statement
auto ret = internalAddItem(player->getInbox(), item, INDEX_WHEREEVER, FLAG_NOLIMIT);
auto ret = internalAddItem(playerInbox, item, INDEX_WHEREEVER, FLAG_NOLIMIT);
// Condition
ret != RETURNVALUE_NOERROR
) {
Expand Down
12 changes: 9 additions & 3 deletions src/io/functions/iologindata_load_player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,16 +679,22 @@ void IOLoginDataLoad::loadPlayerInboxItems(const std::shared_ptr<Player> &player
ItemsMap inboxItems;
loadItems(inboxItems, result, player);

for (auto it = inboxItems.rbegin(), end = inboxItems.rend(); it != end; ++it) {
const std::pair<std::shared_ptr<Item>, int32_t> &pair = it->second;
const auto &playerInbox = player->getInbox();
if (!playerInbox) {
g_logger().warn("[{}] - Player inbox nullptr", __FUNCTION__);
return;
}

for (const auto &it : std::ranges::reverse_view(inboxItems)) {
const std::pair<std::shared_ptr<Item>, int32_t> &pair = it.second;
const auto &item = pair.first;
if (!item) {
continue;
}

int32_t pid = pair.second;
if (pid >= 0 && pid < 100) {
player->getInbox()->internalAddThing(item);
playerInbox->internalAddThing(item);
item->startDecaying();
} else {
auto inboxIt = inboxItems.find(pid);
Expand Down
6 changes: 4 additions & 2 deletions src/io/iomarket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,14 @@ void IOMarket::processExpiredOffers(const DBResult_ptr &result, bool) {
continue;
}

const auto &playerInbox = player->getInbox();

if (itemType.stackable) {
uint16_t tmpAmount = amount;
while (tmpAmount > 0) {
uint16_t stackCount = std::min<uint16_t>(100, tmpAmount);
const auto &item = Item::CreateItem(itemType.id, stackCount);
if (g_game().internalAddItem(player->getInbox(), item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
if (g_game().internalAddItem(playerInbox, item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
g_logger().error("[{}] Ocurred an error to add item with id {} to player {}", __FUNCTION__, itemType.id, player->getName());

break;
Expand All @@ -202,7 +204,7 @@ void IOMarket::processExpiredOffers(const DBResult_ptr &result, bool) {

for (uint16_t i = 0; i < amount; ++i) {
const auto &item = Item::CreateItem(itemType.id, subType);
if (g_game().internalAddItem(player->getInbox(), item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
if (g_game().internalAddItem(playerInbox, item, INDEX_WHEREEVER, FLAG_NOLIMIT) != RETURNVALUE_NOERROR) {
break;
}

Expand Down
19 changes: 14 additions & 5 deletions src/items/containers/container.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ std::shared_ptr<Container> Container::createBrowseField(const std::shared_ptr<Ti

Container::~Container() {
if (getID() == ITEM_BROWSEFIELD) {
auto parent = getParent();
const auto &parent = getParent();
if (parent) {
auto tile = parent->getTile();
const auto &tile = parent->getTile();
if (tile) {
auto browseField = g_game().browseFields.find(tile);
if (browseField != g_game().browseFields.end()) {
Expand Down Expand Up @@ -385,7 +385,12 @@ uint32_t Container::getContainerHoldingCount() {

bool Container::isHoldingItem(const std::shared_ptr<Item> &item) {
for (ContainerIterator it = iterator(); it.hasNext(); it.advance()) {
if (*it == item) {
const auto &compareItem = *it;
if (!compareItem || !item) {
continue;
}

if (compareItem == item) {
return true;
}
}
Expand All @@ -395,6 +400,10 @@ bool Container::isHoldingItem(const std::shared_ptr<Item> &item) {
bool Container::isHoldingItemWithId(const uint16_t id) {
for (ContainerIterator it = iterator(); it.hasNext(); it.advance()) {
const auto &item = *it;
if (!item) {
continue;
}

if (item && item->getID() == id) {
return true;
}
Expand Down Expand Up @@ -1024,9 +1033,9 @@ void ContainerIterator::advance() {
return;
}

auto currentItem = container->itemlist[top.index];
const auto &currentItem = container->itemlist[top.index];
if (currentItem) {
auto subContainer = currentItem->getContainer();
const auto &subContainer = currentItem->getContainer();
if (subContainer && !subContainer->itemlist.empty()) {
size_t newDepth = top.depth + 1;
if (newDepth <= maxTraversalDepth) {
Expand Down
4 changes: 3 additions & 1 deletion src/items/containers/mailbox/mailbox.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ bool Mailbox::sendItem(const std::shared_ptr<Item> &item) const {
text = item->getAttribute<std::string>(ItemAttribute_t::TEXT);
}
if (player && item) {
if (g_game().internalMoveItem(item->getParent(), player->getInbox(), INDEX_WHEREEVER, item, item->getItemCount(), nullptr, FLAG_NOLIMIT) == RETURNVALUE_NOERROR) {
const auto &playerInbox = player->getInbox();
const auto &itemParent = item->getParent();
if (g_game().internalMoveItem(itemParent, playerInbox, INDEX_WHEREEVER, item, item->getItemCount(), nullptr, FLAG_NOLIMIT) == RETURNVALUE_NOERROR) {
const auto &newItem = g_game().transformItem(item, item->getID() + 1);
if (newItem && newItem->getID() == ITEM_LETTER_STAMPED && !writer.empty()) {
newItem->setAttribute(ItemAttribute_t::WRITER, writer);
Expand Down
5 changes: 3 additions & 2 deletions src/map/house/house.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,7 @@ void Houses::payHouses(RentPeriod_t rentPeriod) const {
if (house->getPayRentWarnings() < 7) {
const int32_t daysLeft = 7 - house->getPayRentWarnings();

std::shared_ptr<Item> letter = Item::CreateItem(ITEM_LETTER_STAMPED);
const std::shared_ptr<Item> &letter = Item::CreateItem(ITEM_LETTER_STAMPED);
std::string period;

switch (rentPeriod) {
Expand All @@ -876,7 +876,8 @@ void Houses::payHouses(RentPeriod_t rentPeriod) const {
std::ostringstream ss;
ss << "Warning! \nThe " << period << " rent of " << house->getRent() << " gold for your house \"" << house->getName() << "\" is payable. Have it within " << daysLeft << " days or you will lose this house.";
letter->setAttribute(ItemAttribute_t::TEXT, ss.str());
g_game().internalAddItem(player->getInbox(), letter, INDEX_WHEREEVER, FLAG_NOLIMIT);
const auto &playerInbox = player->getInbox();
g_game().internalAddItem(playerInbox, letter, INDEX_WHEREEVER, FLAG_NOLIMIT);
house->setPayRentWarnings(house->getPayRentWarnings() + 1);
} else {
house->setOwner(0, true, player);
Expand Down
Loading