Fix #15129: Improve matching performance for hash based suppressions (#8970)

Makes it so that hash based suppressions are indexed by hash.
Matching hash based suppressions is now O(1) (ignoring collisions)
instead of O(n).
For large code bases using baseline suppressions (which may be in the
number of tens of thousands) this leads to a substantial speed up. We
could probably speed up matching of other suppressions in the future as
well, but I assume these are in general manually added and won't even
get close to the same number as baseline suppressions. Also fixed a bug
in ProcessExecutor that would cause new suppressions to be added
unintentionally.

Author: ludviggunne

cli/processexecutor.cpp

@@ -96,7 +96,7 @@ namespace {
             {
                 if (suppr.isInline)
                     writeToPipe(REPORT_SUPPR_INLINE, suppressionToString(suppr));
-                else if (suppr.checked)
+                else if (suppr.checked && suppr.hash == 0)
                     writeToPipe(REPORT_SUPPR, suppressionToString(suppr));
             }
         }

lib/suppressions.cpp

@@ -32,7 +32,6 @@
 #include <cctype>   // std::isdigit, std::isalnum, etc
 #include <cstring>
 #include <exception>
-#include <functional> // std::bind, std::placeholders
 #include <sstream>
 #include <stdexcept>
 #include <utility>
@@ -325,14 +324,27 @@ std::string SuppressionList::addSuppressionLine(const std::string &line)
     }
 }
 
+SuppressionList::Suppression *SuppressionList::findSameParameters(const Suppression &suppression)
+{
+    const std::vector<Suppression*> *candidates = &mNonHashSuppressions;
+    if (suppression.hash > 0) {
+        const auto it = mHashSuppressions.find(suppression.hash);
+        if (it == mHashSuppressions.end())
+            return nullptr;
+        candidates = &it->second;
+    }
+    const auto found = std::find_if(candidates->cbegin(), candidates->cend(), [&](const Suppression *s) {
+        return suppression.isSameParameters(*s);
+    });
+    return found != candidates->cend() ? *found : nullptr;
+}
+
 std::string SuppressionList::addSuppression(SuppressionList::Suppression suppression)
 {
     std::lock_guard<std::mutex> lg(mSuppressionsSync);
 
     // Check if suppression is already in list
-    auto foundSuppression = std::find_if(mSuppressions.begin(), mSuppressions.end(),
-                                         std::bind(&Suppression::isSameParameters, &suppression, std::placeholders::_1));
-    if (foundSuppression != mSuppressions.end()) {
+    if (findSameParameters(suppression)) {
         return "suppression '" + suppression.toString() + "' already exists";
     }
 
@@ -356,6 +368,12 @@ std::string SuppressionList::addSuppression(SuppressionList::Suppression suppres
 
     mSuppressions.push_back(std::move(suppression));
 
+    Suppression *const added = &mSuppressions.back();
+    if (added->hash > 0)
+        mHashSuppressions[added->hash].push_back(added);
+    else
+        mNonHashSuppressions.push_back(added);
+
     return "";
 }
 
@@ -374,9 +392,8 @@ bool SuppressionList::updateSuppressionState(const SuppressionList::Suppression&
     std::lock_guard<std::mutex> lg(mSuppressionsSync);
 
     // Check if suppression is already in list
-    auto foundSuppression = std::find_if(mSuppressions.begin(), mSuppressions.end(),
-                                         std::bind(&Suppression::isSameParameters, &suppression, std::placeholders::_1));
-    if (foundSuppression != mSuppressions.end()) {
+    Suppression *const foundSuppression = findSameParameters(suppression);
+    if (foundSuppression) {
         if (suppression.checked)
             foundSuppression->checked = true;
         if (suppression.matched)
@@ -525,13 +542,21 @@ bool SuppressionList::isSuppressed(const SuppressionList::ErrorMessage &errmsg,
     // TODO: handle unmatchedPolyspaceSuppression?
     const bool unmatchedSuppression(errmsg.errorId == "unmatchedSuppression");
     bool returnValue = false;
-    for (Suppression &s : mSuppressions) {
-        if (!global && !s.isLocal())
-            continue;
-        if (unmatchedSuppression && s.errorId != errmsg.errorId)
-            continue;
-        if (s.isMatch(errmsg))
-            returnValue = true;
+    const auto match = [&](const std::vector<Suppression*> &suppressions) {
+        for (Suppression *s : suppressions) {
+            if (!global && !s->isLocal())
+                continue;
+            if (unmatchedSuppression && s->errorId != errmsg.errorId)
+                continue;
+            if (s->isMatch(errmsg))
+                returnValue = true;
+        }
+    };
+    match(mNonHashSuppressions);
+    if (errmsg.hash != 0) {
+        const auto it = mHashSuppressions.find(errmsg.hash);
+        if (it != mHashSuppressions.end())
+            match(it->second);
     }
     return returnValue;
 }
@@ -540,13 +565,21 @@ bool SuppressionList::isSuppressedExplicitly(const SuppressionList::ErrorMessage
 {
     std::lock_guard<std::mutex> lg(mSuppressionsSync);
 
-    for (Suppression &s : mSuppressions) {
-        if (!global && !s.isLocal())
-            continue;
-        if (s.errorId != errmsg.errorId) // Error id must match exactly
-            continue;
-        if (s.isMatch(errmsg))
-            return true;
+    const auto match = [&](const std::vector<Suppression*> &suppressions) {
+        return std::any_of(suppressions.cbegin(), suppressions.cend(), [&](Suppression *s) {
+            if (!global && !s->isLocal())
+                return false;
+            if (s->errorId != errmsg.errorId) // Error id must match exactly
+                return false;
+            return s->isMatch(errmsg);
+        });
+    };
+    if (match(mNonHashSuppressions))
+        return true;
+    if (errmsg.hash != 0) {
+        const auto it = mHashSuppressions.find(errmsg.hash);
+        if (it != mHashSuppressions.end())
+            return match(it->second);
     }
     return false;
 }
@@ -687,7 +720,9 @@ void SuppressionList::markUnmatchedInlineSuppressionsAsChecked(const TokenList &
         if (currFileIdx != tok->fileIndex() || currLineNr != tok->linenr()) {
             currLineNr = tok->linenr();
             currFileIdx = tok->fileIndex();
-            for (auto &suppression : mSuppressions) {
+            // checked is not used for hash suppressions
+            for (Suppression *s : mNonHashSuppressions) {
+                Suppression &suppression = *s;
                 if (suppression.type == SuppressionList::Type::unique) {
                     if (!suppression.checked && (suppression.lineNumber == currLineNr) && (suppression.fileName == tokenlist.file(tok))) {
                         suppression.checked = true;

lib/suppressions.h

@@ -29,6 +29,7 @@
 #include <mutex>
 #include <set>
 #include <string>
+#include <unordered_map>
 #include <utility>
 #include <vector>
 #include <map>
@@ -51,14 +52,14 @@ class CPPCHECKLIB SuppressionList {
     };
 
     struct CPPCHECKLIB ErrorMessage {
-        std::size_t hash;
+        std::size_t hash{};
         std::string errorId;
         void setFileName(std::string s);
         const std::string &getFileName() const {
             return mFileName;
         }
-        int lineNumber; // TODO: need to be unsigned
-        Certainty certainty;
+        int lineNumber{}; // TODO: need to be unsigned
+        Certainty certainty{};
         std::string symbolNames;
         std::set<std::string> macroNames;
 
@@ -294,9 +295,20 @@ class CPPCHECKLIB SuppressionList {
     void markUnmatchedInlineSuppressionsAsChecked(const TokenList &tokenlist);
 
 private:
+    /**
+     * @brief Find a suppression with the same parameters. mSuppressionsSync must be locked.
+     * @param suppression the suppression to look for
+     * @return the found suppression, or nullptr
+     */
+    Suppression *findSameParameters(const Suppression &suppression);
+
     mutable std::mutex mSuppressionsSync;
     /** @brief List of error which the user doesn't want to see. */
     std::list<Suppression> mSuppressions;
+    /** @brief Suppressions in mSuppressions with hash > 0, indexed by hash */
+    std::unordered_map<std::size_t, std::vector<Suppression*>> mHashSuppressions;
+    /** @brief Suppressions in mSuppressions with hash == 0 */
+    std::vector<Suppression*> mNonHashSuppressions;
 };
 
 struct Suppressions

test/cli/other_test.py

@@ -4925,6 +4925,40 @@ def test_ipc_inline_suppressions(tmp_path):
     assert stdout_lines == stdout_exp
     assert stderr.splitlines() == []
 
+
+@pytest.mark.skipif(sys.platform == 'win32', reason="requires ProcessExecutor")
+def test_ipc_hash_suppressions(tmp_path):
+    test_file = tmp_path / 'test.c'
+    with open(test_file, "w") as f:
+        f.write('void f() { int x = 1; }')
+
+    _, _, stderr = cppcheck(['--enable=style', '--xml-version=3', str(test_file)])
+    error_hash = ElementTree.fromstring(stderr).find('.//error').get('hash')
+
+    suppr_file = tmp_path / 'suppr.xml'
+    with open(suppr_file, "w") as f:
+        f.write(f'<suppressions><suppress><id>unreadVariable</id><fileName>{test_file}</fileName><hash>{error_hash}</hash></suppress></suppressions>')
+
+    args = [
+        '-q',
+        '--debug-ipc',
+        '-j2',
+        '--executor=process',
+        '--no-cppcheck-build-dir',
+        '--enable=style',
+        f'--suppress-xml={suppr_file}',
+        str(test_file)
+    ]
+
+    exitcode, stdout, stderr = cppcheck(args)
+    assert exitcode == 0
+    # the hash suppression is not sent to the parent
+    assert stdout.splitlines() == [
+        'writeToPipe - 5 - 0',
+        'handleRead - 5 - 0'
+    ]
+    assert stderr.splitlines() == []
+
 test_redundant_file_reads_params = [
     ([],                       3),
     (['--suppress=zerodiv'],   1),

test/testsuppressions.cpp

@@ -117,6 +117,7 @@ class TestSuppressions : public TestFixture {
         TEST_CASE(addSuppressionDuplicate);
         TEST_CASE(updateSuppressionState);
         TEST_CASE(addSuppressionLineMultiple);
+        TEST_CASE(hashSuppression);
 
         TEST_CASE(suppressionsParseXmlFile);
         TEST_CASE(xmlMacroSuppressions);
@@ -1959,6 +1960,36 @@ class TestSuppressions : public TestFixture {
         }
     }
 
+    void hashSuppression() const {
+        SuppressionList supprs;
+        SuppressionList::Suppression s;
+        s.errorId = "id";
+        s.hash = 1;
+        ASSERT_EQUALS("", supprs.addSuppression(s));
+        ASSERT_EQUALS("suppression 'id' already exists", supprs.addSuppression(s));
+        ASSERT_EQUALS(true, supprs.updateSuppressionState(s));
+        s.hash = 2;
+        ASSERT_EQUALS(false, supprs.updateSuppressionState(s));
+        // same hash, different id
+        s.errorId = "id2";
+        s.hash = 1;
+        ASSERT_EQUALS("", supprs.addSuppression(s));
+        ASSERT_EQUALS(true, supprs.updateSuppressionState(s));
+
+        SuppressionList::ErrorMessage error = errorMessage("id");
+        error.hash = 1;
+        ASSERT_EQUALS(true, supprs.isSuppressed(error));
+        error.hash = 2;
+        ASSERT_EQUALS(false, supprs.isSuppressed(error));
+        error.hash = 0;
+        ASSERT_EQUALS(false, supprs.isSuppressed(error));
+        error.hash = 1;
+        error.errorId = "id2";
+        ASSERT_EQUALS(true, supprs.isSuppressed(error));
+        error.errorId = "id3";
+        ASSERT_EQUALS(false, supprs.isSuppressed(error));
+    }
+
     void addSuppressionLineMultiple() const {
         SuppressionList supprlist;