Skip to content

Commit

Permalink
perf: remove "inbox" from inside extensive loops
Browse files Browse the repository at this point in the history 
Removed getInbox from extensive loops and optimized pointer usage by changing certain shared pointers to const&. These pointers were frequently accessed in recursive calls or iteration-heavy functions, such as the ContainerIterator, reducing unnecessary reference count increments and improving CPU performance.
  • Loading branch information
@dudantas
dudantas committed Nov 24, 2024
1 parent 34b6fa8 commit d22a5df
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 d22a5df

Please sign in to comment.