Skip to content

Commit

Permalink
perf: optimize getInbox usage and shared pointer handling in loops (o…
Browse files Browse the repository at this point in the history 
…pentibiabr#3150)

This addresses performance issues by optimizing the use of shared
pointers and removing redundant calls. Specifically, the `getInbox` call
was removed from extensive loops to prevent unnecessary reference count
increments, which were causing CPU overhead. Additionally, shared
pointers used in recursive or iteration-heavy functions, such as the
`ContainerIterator`, were changed to `const&` where applicable, reducing
the impact of reference counting on performance.
  • Loading branch information
@dudantas
dudantas authored Nov 27, 2024
1 parent a77f3c3 commit 6a6a688
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 28 deletions.
8 changes: 6 additions & 2 deletions src/creatures/players/player.cpp
View file
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
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
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
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
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
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
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

0 comments on commit 6a6a688

Please sign in to comment.