Parallelize issue hashes computation

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,
         }
 

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):
         """

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

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

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,21 +148,13 @@ 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)
             logger.info(
                 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)

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"]

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,

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]