From 224185757db5013019ad6bec936745b0e998bac6 Mon Sep 17 00:00:00 2001
From: Marco Castelluccio <mcastelluccio@mozilla.com>
Date: Fri, 11 Sep 2026 16:09:08 +0200
Subject: [PATCH] Parallelize issue hashes computation

Fixes #3638
---
 bot/code_review_bot/__init__.py              | 59 ++++++++-----------
 bot/code_review_bot/revisions/base.py        | 48 +++++++++++++++
 bot/code_review_bot/revisions/github.py      |  8 +++
 bot/code_review_bot/revisions/phabricator.py |  5 ++
 bot/code_review_bot/workflow.py              | 16 +++--
 bot/tests/test_backend.py                    |  9 ++-
 bot/tests/test_clang.py                      | 10 ++--
 bot/tests/test_coverage.py                   |  1 +
 bot/tests/test_hash.py                       | 61 ++++++++++++++++++++
 bot/tests/test_lint.py                       |  3 +
 bot/tests/test_remote.py                     | 42 ++++++++++++--
 bot/tests/test_workflow.py                   |  1 +
 12 files changed, 205 insertions(+), 58 deletions(-)

diff --git a/bot/code_review_bot/__init__.py b/bot/code_review_bot/__init__.py
index 7a99afde1..67d020023 100644
--- a/bot/code_review_bot/__init__.py
+++ b/bot/code_review_bot/__init__.py
@@ -186,47 +186,40 @@ def in_patch(self):
     def in_touched_files(self):
         return self.revision.in_touched_files(self)
 
-    @cached_property
+    @property
+    def is_autogenerated(self):
+        """
+        An autogenerated file resides in the build directory that has the
+        format `obj-x86_64-pc-linux-gnu`
+        """
+        return "obj-" in self.path
+
+    @property
     def hash(self):
         """
-        Build a unique hash identifying that issue and cache the resulting value
+        Unique hash identifying that issue, built by `Revision.build_issues_hashes`
         The text concerned by the issue is used and not its position in the file
         Message content is hashed as a single linter may return multiple issues on a single line
         We make the assumption that the message does not contain the line number
         If an error occurs reading the file content (locally or remotely), None is returned
         """
-        from code_review_bot.revisions import GithubRevision, PhabricatorRevision
+        assert hasattr(
+            self, "_hash"
+        ), "Issue hash is not built yet, use Revision.build_issues_hashes first"
+        return self._hash
 
-        assert self.revision is not None, "Missing revision"
-
-        local_repository = None
-        if isinstance(self.revision, PhabricatorRevision):
-            if settings.mercurial_cache_checkout:
-                local_repository = settings.mercurial_cache_checkout
-        elif isinstance(self.revision, GithubRevision):
-            assert (
-                settings.git_cache
-            ), "Github cache repository is mandatory to analyse a github revision"
-            local_repository = settings.git_cache / self.revision.repository_slug
-        else:
-            raise NotImplementedError(self.revision.__class__)
-
-        # Build the hash only if the file is not autogenerated.
-        # An autogenerated file resides in the build directory that it has the
-        #  format `obj-x86_64-pc-linux-gnu`
-        file_content = None
-        if "obj-" not in self.path:
-            file_content = self.revision.get_file_content(self.path, local_repository)
-
-        if file_content is None:
-            self._hash = None
-            return self._hash
+    @hash.setter
+    def hash(self, value):
+        self._hash = value
 
+    def build_hash(self, file_lines):
+        """
+        Build the hash from the lines of the file affected by the issue
+        Only meant to be called by `Revision.build_issues_hashes`
+        """
         # Build raw content:
         # 1. lines affected by patch
         # 2. without any spaces around each line
-        file_lines = file_content.splitlines()
-
         if self.line is None or self.nb_lines is None:
             # Use full file when line is not specified
             lines = file_lines
@@ -308,12 +301,6 @@ def as_dict(self):
         Build the serializable dict representation of the issue
         Used by debugging tools
         """
-        issue_hash = None
-        try:
-            issue_hash = self.hash
-        except Exception as e:
-            logger.warn("Failed to build issue hash", error=str(e), issue=str(self))
-
         return {
             "analyzer": self.analyzer.name,
             "path": self.path,
@@ -326,7 +313,7 @@ def as_dict(self):
             "in_patch": self.in_patch,
             "validates": self.validates(),
             "publishable": self.is_publishable(),
-            "hash": issue_hash,
+            "hash": self.hash,
             "fix": self.fix,
         }
 
diff --git a/bot/code_review_bot/revisions/base.py b/bot/code_review_bot/revisions/base.py
index 688112949..a07db3719 100644
--- a/bot/code_review_bot/revisions/base.py
+++ b/bot/code_review_bot/revisions/base.py
@@ -5,6 +5,8 @@
 import os
 import random
 from abc import ABC
+from collections import defaultdict
+from concurrent.futures import ThreadPoolExecutor
 from datetime import timedelta
 from pathlib import Path
 
@@ -87,6 +89,14 @@ def __init__(
     def namespaces(self):
         raise NotImplementedError
 
+    @property
+    def local_repository(self):
+        """
+        Path to the local clone of the repository at the revision's changeset
+        (None when the files must be retrieved remotely)
+        """
+        raise NotImplementedError
+
     @property
     def before_after_feature(self):
         """
@@ -192,6 +202,44 @@ def get_file_content(
                 file_content = None
         return file_content
 
+    def build_issues_hashes(self, issues, max_workers=8):
+        """
+        Build the hashes of all the issues, in parallel
+        Issues are grouped by file so that each file is loaded only once,
+        then released once the hashes of its issues are built
+        """
+        local_repository = self.local_repository
+
+        groups = defaultdict(list)
+        for issue in issues:
+            if issue.is_autogenerated:
+                issue.hash = None
+            else:
+                groups[issue.path].append(issue)
+        if not groups:
+            return
+
+        def _build_hashes(path_issues):
+            content = self.get_file_content(path_issues[0].path, local_repository)
+            if content is None:
+                # The file could not be read (locally or remotely), no hash
+                for issue in path_issues:
+                    issue.hash = None
+                return
+
+            file_lines = content.splitlines()
+            for issue in path_issues:
+                issue.hash = issue.build_hash(file_lines)
+
+        logger.info(
+            "Building issues hashes",
+            nb_issues=sum(len(g) for g in groups.values()),
+            nb_files=len(groups),
+        )
+        with ThreadPoolExecutor(max_workers=max_workers) as executor:
+            # Consume the iterator to raise any exception from the workers
+            list(executor.map(_build_hashes, groups.values()))
+
     @property
     def has_clang_files(self):
         """
diff --git a/bot/code_review_bot/revisions/github.py b/bot/code_review_bot/revisions/github.py
index 249b34758..602381434 100644
--- a/bot/code_review_bot/revisions/github.py
+++ b/bot/code_review_bot/revisions/github.py
@@ -9,6 +9,7 @@
 import structlog
 
 from code_review_bot import taskcluster
+from code_review_bot.config import settings
 from code_review_bot.git import build_repo_slug
 from code_review_bot.revisions import Revision
 
@@ -69,6 +70,13 @@ def namespaces(self):
             f"github.head.{_head_repository_slug}.rev.{self.head_changeset}",
         ]
 
+    @property
+    def local_repository(self):
+        assert (
+            settings.git_cache
+        ), "Github cache repository is mandatory to analyse a github revision"
+        return settings.git_cache / self.repository_slug
+
     def load_patch(self):
         """
         Load the patch content for the current pull request HEAD
diff --git a/bot/code_review_bot/revisions/phabricator.py b/bot/code_review_bot/revisions/phabricator.py
index 5ff0b1959..e21edf835 100644
--- a/bot/code_review_bot/revisions/phabricator.py
+++ b/bot/code_review_bot/revisions/phabricator.py
@@ -79,6 +79,11 @@ def __init__(
         # Patch analysis
         self.patch = patch
 
+    @property
+    def local_repository(self):
+        # Files are loaded from HGMO when no local mercurial checkout is configured
+        return settings.mercurial_cache_checkout
+
     @property
     def namespaces(self):
         # Simplify repository names
diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py
index a4f873c0d..126d908fb 100644
--- a/bot/code_review_bot/workflow.py
+++ b/bot/code_review_bot/workflow.py
@@ -128,6 +128,11 @@ def run(self, revision):
             revision, settings.try_group_id
         )
 
+        # Clone local repo when required, then build the issues hashes
+        # as they are needed to find previous issues and to publish
+        self.clone_repository(revision)
+        revision.build_issues_hashes(issues)
+
         # Analyze issues in case the before/after feature is enabled
         if revision.before_after_feature:
             logger.info("Running the before/after feature")
@@ -143,10 +148,6 @@ def run(self, revision):
                     task=settings.try_group_id,
                 )
 
-            # Clone local repo when required
-            # as find_previous_issues will build the hashes
-            self.clone_repository(revision)
-
             # Mark know issues to avoid publishing them on this patch
             self.find_previous_issues(revision, issues, base_rev_changeset)
             new_issues_count = sum(issue.new_issue for issue in issues)
@@ -154,10 +155,6 @@ def run(self, revision):
                 f"Found {new_issues_count} new issues (over {len(issues)} total detected issues)",
                 task=settings.try_group_id,
             )
-        else:
-            # Clone local repo when required
-            # as publication need the hashes
-            self.clone_repository(revision)
 
         if (
             all(issue.new_issue is False for issue in issues)
@@ -256,8 +253,9 @@ def _build_tasks(tasks):
             logger.info("No issues for that revision")
             return
 
-        # Clone local repo when required
+        # Clone local repo when required, then build the issues hashes
         self.clone_repository(revision)
+        revision.build_issues_hashes(issues)
 
         # Publish issues in the backend
         self.backend_api.publish_issues(issues, revision)
diff --git a/bot/tests/test_backend.py b/bot/tests/test_backend.py
index 8d3d922cc..d11f6458d 100644
--- a/bot/tests/test_backend.py
+++ b/bot/tests/test_backend.py
@@ -64,6 +64,7 @@ def test_publication(mock_clang_tidy_issues, mock_revision, mock_backend, mock_h
     assert len(issues) == 0
 
     # Let's publish them
+    mock_revision.build_issues_hashes(mock_clang_tidy_issues)
     published = r.publish_issues(mock_clang_tidy_issues, mock_revision)
     assert published == len(mock_clang_tidy_issues) == 2
 
@@ -217,6 +218,7 @@ def test_publication_failures(
     # Issues URL must be set when publishing issues on the backend
     mock_revision.issues_url = "http://code-review-backend.test/v1/revision/51/issues/"
 
+    mock_revision.build_issues_hashes(mock_clang_tidy_issues)
     published = r.publish_issues(mock_clang_tidy_issues, mock_revision)
     assert published == 1
 
@@ -272,6 +274,7 @@ def test_publish_issues(
         *mock_clang_tidy_issues,
     ]
 
+    mock_revision.build_issues_hashes(issues)
     published = r.publish_issues(issues, mock_revision)
     assert published == 2
 
@@ -360,9 +363,9 @@ def test_publication_skips_rustfmt_dot_path(
     # Issue URL is set when publishing the issue on the backend
     mock_revision.issues_url = "http://code-review-backend.test/v1/revision/51/issues/"
 
-    published = r.publish_issues(
-        [*mock_clang_tidy_issues, ignored_issue], mock_revision
-    )
+    all_issues = [*mock_clang_tidy_issues, ignored_issue]
+    mock_revision.build_issues_hashes(all_issues)
+    published = r.publish_issues(all_issues, mock_revision)
     assert published == 1
 
     assert list(issues.keys()) == ["b29184e6-4d35-5bbd-8a53-e00686e08407"]
diff --git a/bot/tests/test_clang.py b/bot/tests/test_clang.py
index f7fda2a92..ed7419513 100644
--- a/bot/tests/test_clang.py
+++ b/bot/tests/test_clang.py
@@ -92,6 +92,7 @@ def test_as_dict(mock_revision, mock_hgmo, mock_task):
         Reliability.Low,
     )
 
+    mock_revision.build_issues_hashes([issue])
     assert issue.as_dict() == {
         "analyzer": "clang-tidy",
         "path": "test.cpp",
@@ -199,7 +200,7 @@ def test_missing_clang_format_diff(mock_task, mock_revision, capsys):
     )
 
 
-def test_real_patch(mock_revision, mock_task):
+def test_real_patch(mock_revision, mock_hgmo, mock_task):
     """
     Test clang format patch parsing with a real patch
     """
@@ -214,6 +215,7 @@ def test_real_patch(mock_revision, mock_task):
 
     assert len(issues) == 3
 
+    mock_revision.build_issues_hashes(issues)
     assert [i.as_dict() for i in issues] == [
         {
             "analyzer": "mock-clang-format",
@@ -227,7 +229,7 @@ def test_real_patch(mock_revision, mock_task):
                                       aUseFontSmoothing, aApplySyntheticBold);
 }
 #endif\n""",
-            "hash": None,
+            "hash": "115bec679bf3ef70239394c71389ac82",
             "in_patch": False,
             "level": "warning",
             "line": 616,
@@ -247,7 +249,7 @@ def test_real_patch(mock_revision, mock_task):
   }
   return true;
 }\n""",
-            "hash": None,
+            "hash": "3c077d8a49dca767f36e224ae6f97511",
             "in_patch": False,
             "level": "warning",
             "line": 118,
@@ -264,7 +266,7 @@ def test_real_patch(mock_revision, mock_task):
             "fix": """  if (false) return true;
   return eNameOK;
 }\n""",
-            "hash": None,
+            "hash": "13c464427ce2a73f26538b99b59f2e87",
             "in_patch": False,
             "level": "warning",
             "line": 36,
diff --git a/bot/tests/test_coverage.py b/bot/tests/test_coverage.py
index efb77291b..6139460ee 100644
--- a/bot/tests/test_coverage.py
+++ b/bot/tests/test_coverage.py
@@ -25,6 +25,7 @@ def test_coverage(
 
     # The list must have three elements
     assert len(issues) == 3
+    mock_revision.build_issues_hashes(issues)
 
     # Verify that each element has a sane value
     issue = issues[0]
diff --git a/bot/tests/test_hash.py b/bot/tests/test_hash.py
index 13b63c231..94eeb1c27 100644
--- a/bot/tests/test_hash.py
+++ b/bot/tests/test_hash.py
@@ -5,6 +5,7 @@
 import hashlib
 
 import pytest
+import responses
 
 from code_review_bot.tasks.lint import MozLintIssue, MozLintTask
 
@@ -50,6 +51,11 @@ def test_get_hash(mock_revision, mock_hgmo, mock_task):
         "A random & fake linting issue"
     )
     hash_check = hashlib.md5(payload.encode("utf-8")).hexdigest()
+
+    # The hash is not available until it is built through the revision
+    with pytest.raises(AssertionError, match="Issue hash is not built yet"):
+        issue.hash
+    mock_revision.build_issues_hashes([issue])
     assert hash_check == "b06e5b92a609496d1473ca90fec1749c" == issue.hash
 
 
@@ -92,6 +98,7 @@ def test_indentation_effect(mock_revision, mock_hgmo, mock_task):
     assert lines[4] == 'print("Hello !")'
 
     # Check the hashes are equal
+    mock_revision.build_issues_hashes([issue_indent, issue_no_indent])
     assert (
         issue_indent.hash == issue_no_indent.hash == "a8c5c52b21c12b483617adc60cdd5dc2"
     )
@@ -123,6 +130,7 @@ def test_full_file(mock_revision, mock_hgmo, mock_task):
     assert issue.line is None
 
     # Build hash should use the full file
+    mock_revision.build_issues_hashes([issue])
     assert issue.hash == "65fe9040e64b3617e4cbf40ef478f62d"
 
     # Check positive integers or None are used in report
@@ -143,6 +151,59 @@ def test_full_file(mock_revision, mock_hgmo, mock_task):
     }
 
 
+def test_build_issues_hashes(mock_revision, mock_hgmo, mock_task):
+    """
+    Test the hashes of many issues are built in parallel,
+    loading each affected file only once
+    """
+    # Hardcode revision & repo
+    mock_revision.head_repository = "test-try"
+    mock_revision.head_changeset = "deadbeef1234"
+
+    def build_issue(path, line):
+        return MozLintIssue(
+            mock_task(MozLintTask, "mock-analyzer-eslint"),
+            path,
+            42,
+            "error",
+            line,
+            "eslint",
+            "A random & fake linting issue",
+            "EXXX",
+            mock_revision,
+        )
+
+    issues = [
+        build_issue("preload/file.cpp", 1),
+        build_issue("preload/file.cpp", 2),
+        build_issue("preload/file.cpp", 3),
+        build_issue("preload/another.cpp", 1),
+        # Autogenerated files are never loaded
+        build_issue("build/obj-x86_64-pc-linux-gnu/generated.cpp", 1),
+    ]
+
+    def hgmo_urls():
+        return sorted(
+            call.request.url
+            for call in responses.calls
+            if "raw-file" in call.request.url
+        )
+
+    # Only the two real files are downloaded from HGMO, once each
+    mock_revision.build_issues_hashes(issues)
+    assert hgmo_urls() == [
+        "https://hg.mozilla.org/test-try/raw-file/deadbeef1234/preload/another.cpp",
+        "https://hg.mozilla.org/test-try/raw-file/deadbeef1234/preload/file.cpp",
+    ]
+
+    # All hashes are built, reading them does not trigger any new download
+    hashes = [issue.hash for issue in issues]
+    assert all(h is not None for h in hashes[:4])
+    assert hashes[4] is None
+    assert len(set(hashes[:4])) == 4
+    assert len(hgmo_urls()) == 2
+
+
 @pytest.mark.parametrize("path", [".", "..", "a/../../b"])
 def test_incorrect_file_path_no_raise(mock_revision, path):
     """
diff --git a/bot/tests/test_lint.py b/bot/tests/test_lint.py
index db6862076..8fc031c15 100644
--- a/bot/tests/test_lint.py
+++ b/bot/tests/test_lint.py
@@ -45,6 +45,7 @@ def test_flake8_checks(mock_config, mock_revision, mock_hgmo, mock_task):
     assert issue.is_disabled_check()
     assert not issue.validates()
 
+    mock_revision.build_issues_hashes([issue])
     assert issue.as_dict() == {
         "analyzer": "mock-lint-flake8",
         "check": "Q000",
@@ -93,6 +94,7 @@ def test_as_text(mock_config, mock_revision, mock_hgmo, mock_task):
         "severity": "error",
     }
 
+    mock_revision.build_issues_hashes([issue])
     assert issue.as_dict() == {
         "analyzer": "mock-lint-flake8",
         "check": "dummy rule",
@@ -139,4 +141,5 @@ def test_licence_payload(mock_revision, mock_hgmo):
         == "source-test-mozlint-license issue source-test-mozlint-license@error intl/locale/rust/unic-langid-ffi/src/lib.rs full file"
     )
     assert issue.check == issue.analyzer.name == "source-test-mozlint-license"
+    mock_revision.build_issues_hashes([issue])
     assert issue.hash == "7142c536e10b31925b018c37b0e6f9f8"
diff --git a/bot/tests/test_remote.py b/bot/tests/test_remote.py
index 8af91b1a0..7e5935278 100644
--- a/bot/tests/test_remote.py
+++ b/bot/tests/test_remote.py
@@ -208,7 +208,12 @@ def test_no_issues(
 
 
 def test_build_status_fail_on_error(
-    mock_config, mock_revision, mock_workflow, mock_backend, bypass_publication_check
+    mock_config,
+    mock_revision,
+    mock_workflow,
+    mock_backend,
+    mock_hgmo,
+    bypass_publication_check,
 ):
     """
     Test a remote workflow with an error causes the build to be reported as failing
@@ -254,7 +259,12 @@ def test_build_status_fail_on_error(
 
 
 def test_build_status_pass_on_warning(
-    mock_config, mock_revision, mock_workflow, mock_backend, bypass_publication_check
+    mock_config,
+    mock_revision,
+    mock_workflow,
+    mock_backend,
+    mock_hgmo,
+    bypass_publication_check,
 ):
     """
     Test a remote workflow with no errors causes the build to be reported as passing
@@ -326,7 +336,12 @@ def test_unsupported_analyzer(
 
 
 def test_mozlint_task(
-    mock_config, mock_revision, mock_workflow, mock_backend, bypass_publication_check
+    mock_config,
+    mock_revision,
+    mock_workflow,
+    mock_backend,
+    mock_hgmo,
+    bypass_publication_check,
 ):
     """
     Test a remote workflow with a mozlint analyzer
@@ -383,7 +398,12 @@ def test_mozlint_task(
 
 
 def test_clang_tidy_task(
-    mock_config, mock_revision, mock_workflow, mock_backend, bypass_publication_check
+    mock_config,
+    mock_revision,
+    mock_workflow,
+    mock_backend,
+    mock_hgmo,
+    bypass_publication_check,
 ):
     """
     Test a remote workflow with a clang-tidy analyzer
@@ -582,7 +602,12 @@ def test_no_tasks(
 
 
 def test_zero_coverage_option(
-    mock_config, mock_revision, mock_workflow, mock_backend, bypass_publication_check
+    mock_config,
+    mock_revision,
+    mock_workflow,
+    mock_backend,
+    mock_hgmo,
+    bypass_publication_check,
 ):
     """
     Test the zero coverage trigger on the workflow
@@ -624,7 +649,12 @@ def test_zero_coverage_option(
 
 
 def test_external_tidy_task(
-    mock_config, mock_revision, mock_workflow, mock_backend, bypass_publication_check
+    mock_config,
+    mock_revision,
+    mock_workflow,
+    mock_backend,
+    mock_hgmo,
+    bypass_publication_check,
 ):
     """
     Test a remote workflow with a clang-tidy-externak analyzer
diff --git a/bot/tests/test_workflow.py b/bot/tests/test_workflow.py
index 29eb99f21..b4ff5a538 100644
--- a/bot/tests/test_workflow.py
+++ b/bot/tests/test_workflow.py
@@ -219,6 +219,7 @@ def test_before_after(mock_taskcluster_config, mock_workflow, mock_task, mock_re
     mock_workflow.backend_api.password = "hunter2"
     for index, hash_val in enumerate(("aaaa", "bbbb")):
         issues[index].hash = hash_val
+    mock_revision.build_issues_hashes = mock.Mock()
 
     current_date = datetime.now().strftime("%Y-%m-%d")
     responses.add(