From f4b4365464cce72b69d929d318e49025753f5a53 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Fri, 23 Oct 2026 11:23:04 +0200 Subject: [PATCH] [Search] Stop allocating per card in card-name completion (#7604) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Search] Stop allocating per card in card-name completion CardSearchModel::updateSearchResults() runs on every textEdited() signal, walks the whole card database and runs Levenshtein against every matching name. Three costs dominated that path: 1. Every candidate built a QStringList of its searchable names, then lowercased each name into another QString, only to break out of the loop after the first hit. 2. The sort comparator called sortableName() for each comparison, which re-fetched the name (QMap lookup plus QString construction) O(n log n) times. 3. levenshteinDistance() allocated a full len1 x len2 vector of vectors. The search model now lowercases the one name it tests directly, stores the tie-breaking name length once per result, and the comparator only compares integers. levenshteinDistance() uses the standard two-row formulation: same result, O(len2) memory instead of O(len1*len2), with early exits for empty and equal strings. FilterString query closures get the same treatment: - Set queries iterate the set map directly instead of materializing keys()/values() lists for every card. - Rarity queries iterate printings in place instead of copying them into a QList first. - Power/toughness split the string with indexOf and compare QStringViews instead of allocating a QStringList. - Field queries do one hash lookup instead of contains() followed by value(). - Color queries bind the colors string by reference instead of copying. Verified with filter_string_test (42 tests) and a full client build. * [Search] Address review comments - Use std::any_of/std::none_of over the set map's key/value iterators instead of hand-rolled loops. - Extract the name matching into matchingLowercaseName() returning std::optional. - Reuse the matched name's length for the distance tie-break instead of looking the name up again, removing one getLocalizedName() call per card. - Take the color strings by value; the getters already return by value. --------- Co-authored-by: Lukas Brübach --- .../libcockatrice/filters/filter_string.cpp | 64 ++++++++++-------- .../database/card/card_search_model.cpp | 66 ++++++++++--------- .../models/database/card/card_search_model.h | 18 +++-- .../libcockatrice/utility/levenshtein.cpp | 41 ++++++++---- 4 files changed, 113 insertions(+), 76 deletions(-) diff --git a/libcockatrice_filters/libcockatrice/filters/filter_string.cpp b/libcockatrice_filters/libcockatrice/filters/filter_string.cpp index 0acc0eca0..081ada824 100644 --- a/libcockatrice_filters/libcockatrice/filters/filter_string.cpp +++ b/libcockatrice_filters/libcockatrice/filters/filter_string.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -161,30 +162,28 @@ static void setupParserRules() if (sv.choice() == 0) { auto matcher = std::any_cast(sv[0]); return [=](const CardData &x) -> bool { - QList sets = x->getSets().keys(); - - auto matchesSet = [&matcher](const QString &set) { return matcher(set); }; - return std::any_of(sets.begin(), sets.end(), matchesSet); + const auto &sets = x->getSets(); + return std::any_of(sets.keyValueBegin(), sets.keyValueEnd(), + [&matcher](const auto &set) { return matcher(set.first); }); }; } if (sv.choice() == 1) { auto matcher = std::any_cast(sv[0]); return [=](const CardData &x) -> bool { - QList sets = x->getSets().keys(); - - auto matchesSet = [&matcher](const QString &set) { return matcher(set); }; - return std::none_of(sets.begin(), sets.end(), matchesSet); + const auto &sets = x->getSets(); + return std::none_of(sets.keyValueBegin(), sets.keyValueEnd(), + [&matcher](const auto &set) { return matcher(set.first); }); }; } auto matcher = std::any_cast(sv[0]); return [=](const CardData &x) -> bool { - const auto &sets = x->getSets().values(); - auto matchesSet = [&](const PrintingInfo &printing) { - return printing.getSet()->getEnabled() && matcher(printing.getSet()->getReleaseDate().toJulianDay()); - }; - return std::any_of(sets.begin(), sets.end(), [&](const auto &printings) { - return std::any_of(printings.begin(), printings.end(), matchesSet); + const auto &sets = x->getSets(); + return std::any_of(sets.constBegin(), sets.constEnd(), [&matcher](const auto &printings) { + return std::any_of(printings.begin(), printings.end(), [&matcher](const PrintingInfo &printing) { + const CardSetPtr set = printing.getSet(); + return set && set->getEnabled() && matcher(set->getReleaseDate().toJulianDay()); + }); }); }; }; @@ -207,15 +206,14 @@ static void setupParserRules() search["RarityQuery"] = [](const peg::SemanticValues &sv) -> Filter { const auto rarity = std::any_cast(sv[0]); return [=](const CardData &x) -> bool { - QList infos; for (const auto &printings : x->getSets()) { for (const auto &printing : printings) { - infos.append(printing); + if (rarity == printing.getProperty("rarity")) { + return true; + } } } - - auto matchesRarity = [&rarity](const PrintingInfo &info) { return rarity == info.getProperty("rarity"); }; - return std::any_of(infos.begin(), infos.end(), matchesRarity); + return false; }; }; @@ -411,7 +409,7 @@ static void setupParserRules() const bool identity = sv.tokens[0].empty() || sv.tokens[0][0] != 'i'; if (sv.tokens[1][0] == ':') { return [=](const CardData &x) { - QString match = identity ? x->getColors() : x->getProperty("coloridentity"); + const QString match = identity ? x->getColors() : x->getProperty("coloridentity"); if (parts.contains("m") && match.length() < 2) { return false; } @@ -429,7 +427,7 @@ static void setupParserRules() } return [=](const CardData &x) { - QString match = identity ? x->getColors() : x->getProperty("colorIdentity"); + const QString match = identity ? x->getColors() : x->getProperty("colorIdentity"); if (parts.contains("m") && match.length() < 2) { return false; } @@ -455,25 +453,37 @@ static void setupParserRules() }; search["PowerQuery"] = [](const peg::SemanticValues &sv) -> Filter { const auto matcher = std::any_cast(sv[0]); - return [=](const CardData &x) -> bool { return matcher(x->getPowTough().split("/")[0].toInt()); }; + return [=](const CardData &x) -> bool { + const QString &powTough = x->getPowTough(); + const qsizetype slash = powTough.indexOf('/'); + return matcher(slash < 0 ? powTough.toInt() : QStringView(powTough).left(slash).toInt()); + }; }; search["ToughnessQuery"] = [](const peg::SemanticValues &sv) -> Filter { const auto matcher = std::any_cast(sv[0]); return [=](const CardData &x) -> bool { - auto parts = x->getPowTough().split("/"); - return matcher(parts.length() == 2 ? parts[1].toInt() : 0); + const QString &powTough = x->getPowTough(); + const qsizetype slash = powTough.indexOf('/'); + return matcher(slash < 0 ? 0 : QStringView(powTough).mid(slash + 1).toInt()); }; }; search["FieldQuery"] = [](const peg::SemanticValues &sv) -> Filter { const auto field = std::any_cast(sv[0]); if (sv.choice() == 0) { const auto matcher = std::any_cast(sv[1]); - return [=](const CardData &x) -> bool { return x->hasProperty(field) && matcher(x->getProperty(field)); }; + return [=](const CardData &x) -> bool { + const auto &properties = x->getPropertiesHash(); + const auto it = properties.constFind(field); + return it != properties.constEnd() && matcher(it.value()); + }; } const auto matcher = std::any_cast(sv[1]); - return - [=](const CardData &x) -> bool { return x->hasProperty(field) && matcher(x->getProperty(field).toInt()); }; + return [=](const CardData &x) -> bool { + const auto &properties = x->getPropertiesHash(); + const auto it = properties.constFind(field); + return it != properties.constEnd() && matcher(it.value().toInt()); + }; }; search["GenericQuery"] = [](const peg::SemanticValues &sv) -> Filter { const auto matcher = std::any_cast(sv[0]); diff --git a/libcockatrice_models/libcockatrice/models/database/card/card_search_model.cpp b/libcockatrice_models/libcockatrice/models/database/card/card_search_model.cpp index 5bde6e3fc..8a267ed7f 100644 --- a/libcockatrice_models/libcockatrice/models/database/card/card_search_model.cpp +++ b/libcockatrice_models/libcockatrice/models/database/card/card_search_model.cpp @@ -9,6 +9,7 @@ #include #include #include +#include CardSearchModel::CardSearchModel(CardDatabaseDisplayModel *sourceModel, QObject *parent) : QAbstractListModel(parent), sourceModel(sourceModel) @@ -69,31 +70,28 @@ void CardSearchModel::updateSearchResults(const QString &query) continue; } - // The completer suggestions match against the same languages the card - // search uses, so typing a localized name finds the card. In Both mode - // either language can match. - for (const QString &matchName : searchableNames(card)) { - const QString lowerName = matchName.toLower(); - if (!lowerName.contains(lowerQuery)) { - continue; - } + const std::optional lowerName = matchingLowercaseName(card, lowerQuery); + if (!lowerName) { + continue; + } - const int distance = levenshteinDistance(lowerQuery, lowerName); + const int distance = levenshteinDistance(lowerQuery, *lowerName); + // Length of the matched name, used to break distance ties. Stored here + // so the comparator does not look the name up again for every comparison. + const int nameLength = static_cast(lowerName->size()); - if (lowerName.startsWith(lowerQuery)) { - prefixMatches.append({card, distance}); - } else { - containsMatches.append({card, distance}); - } - break; + if (lowerName->startsWith(lowerQuery)) { + prefixMatches.append({card, distance, nameLength}); + } else { + containsMatches.append({card, distance, nameLength}); } } - auto sortByDistanceThenLength = [this](const SearchResult &a, const SearchResult &b) { + auto sortByDistanceThenLength = [](const SearchResult &a, const SearchResult &b) { if (a.distance != b.distance) { return a.distance < b.distance; } - return sortableName(a.card).size() < sortableName(b.card).size(); + return a.nameLength < b.nameLength; }; std::sort(prefixMatches.begin(), prefixMatches.end(), sortByDistanceThenLength); @@ -112,24 +110,28 @@ void CardSearchModel::updateSearchResults(const QString &query) endResetModel(); } -QStringList CardSearchModel::searchableNames(const CardInfoPtr &card) const +std::optional CardSearchModel::matchingLowercaseName(const CardInfoPtr &card, const QString &lowerQuery) const { + if (!searchLanguage.isEnglishOnly() && searchLanguage.mode == SearchLanguageMode::Selected) { + QString localizedName = card->getLocalizedName(searchLanguage.language).toLower(); + if (localizedName.contains(lowerQuery)) { + return std::optional(std::move(localizedName)); + } + return std::nullopt; + } + + QString englishName = card->getName().toLower(); + if (englishName.contains(lowerQuery)) { + return std::optional(std::move(englishName)); + } + if (searchLanguage.isEnglishOnly()) { - return {card->getName()}; + return std::nullopt; } - const QString localizedName = card->getLocalizedName(searchLanguage.language); - if (searchLanguage.mode == SearchLanguageMode::Selected) { - return {localizedName}; + QString localizedName = card->getLocalizedName(searchLanguage.language).toLower(); + if (localizedName.contains(lowerQuery)) { + return std::optional(std::move(localizedName)); } - - return {card->getName(), localizedName}; -} - -QString CardSearchModel::sortableName(const CardInfoPtr &card) const -{ - if (searchLanguage.isEnglishOnly()) { - return card->getName(); - } - return card->getLocalizedName(searchLanguage.language); + return std::nullopt; } diff --git a/libcockatrice_models/libcockatrice/models/database/card/card_search_model.h b/libcockatrice_models/libcockatrice/models/database/card/card_search_model.h index 08d0e8636..ec7c1195e 100644 --- a/libcockatrice_models/libcockatrice/models/database/card/card_search_model.h +++ b/libcockatrice_models/libcockatrice/models/database/card/card_search_model.h @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -51,13 +52,20 @@ private: { CardInfoPtr card; int distance; + int nameLength; }; - /** @brief The names a card is searched by with the current search language. */ - [[nodiscard]] QStringList searchableNames(const CardInfoPtr &card) const; - - /** @brief The name used to break distance ties when sorting suggestions. */ - [[nodiscard]] QString sortableName(const CardInfoPtr &card) const; + /** + * @brief Finds the lowercased card name that matches the query. + * + * The completer suggestions match against the same languages the card + * search uses, so typing a localized name finds the card. In Both mode + * either language can match. + * + * @return The matched lowercased name, or std::nullopt when the card does not match. + */ + [[nodiscard]] std::optional matchingLowercaseName(const CardInfoPtr &card, + const QString &lowerQuery) const; CardDatabaseDisplayModel *sourceModel; QList searchResults; diff --git a/libcockatrice_utility/libcockatrice/utility/levenshtein.cpp b/libcockatrice_utility/libcockatrice/utility/levenshtein.cpp index f7d8459cb..11eeef19b 100644 --- a/libcockatrice_utility/libcockatrice/utility/levenshtein.cpp +++ b/libcockatrice_utility/libcockatrice/utility/levenshtein.cpp @@ -7,23 +7,40 @@ int levenshteinDistance(const QString &s1, const QString &s2) { - int len1 = s1.size(); - int len2 = s2.size(); - std::vector> dp(len1 + 1, std::vector(len2 + 1)); + const qsizetype len1 = s1.size(); + const qsizetype len2 = s2.size(); - for (int i = 0; i <= len1; i++) { - dp[i][0] = i; + if (len1 == 0) { + return static_cast(len2); } - for (int j = 0; j <= len2; j++) { - dp[0][j] = j; + if (len2 == 0) { + return static_cast(len1); + } + if (s1 == s2) { + return 0; } - for (int i = 1; i <= len1; i++) { - for (int j = 1; j <= len2; j++) { - int cost = (s1[i - 1] == s2[j - 1]) ? 0 : 1; - dp[i][j] = std::min({dp[i - 1][j] + 1, dp[i][j - 1] + 1, dp[i - 1][j - 1] + cost}); + // Only the previous row of the distance matrix is needed to compute the + // next one, so two rows replace the full len1 x len2 matrix. The card + // search runs this for every matching card on each keystroke. + std::vector previous(len2 + 1); + std::vector current(len2 + 1); + + for (qsizetype j = 0; j <= len2; j++) { + previous[j] = static_cast(j); + } + + for (qsizetype i = 1; i <= len1; i++) { + current[0] = static_cast(i); + const QChar c1 = s1.at(i - 1); + + for (qsizetype j = 1; j <= len2; j++) { + const int cost = (c1 == s2.at(j - 1)) ? 0 : 1; + current[j] = std::min(previous[j] + 1, std::min(current[j - 1] + 1, previous[j - 1] + cost)); } + + std::swap(previous, current); } - return dp[len1][len2]; + return previous[len2]; }