From 3bea3fbb12896884cbf9335577a629ef00ba1ebf Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 27 Feb 2017 07:59:40 -0800
Subject: [PATCH] When computing revision ownership, cache some intermediate
 results for performance

Summary:
Ref T12319. With large datasets, the computation of which packages own paths in a revision is needlessly slow.

Improve performance through caching:

  - Cache which paths belong to each repository.
  - Cache the split fragments of each path.
  - Cache the path fragment counts.
  - Micro-optimize accessing `$this->path`.

Test Plan:
  - Used `bin/lipsum` to generate 4,000 packages with 150,000 paths.
  - Created a revision affecting 100 paths in `phabricator/` (these paths mostly overlap with `bin/lipsum` path rules, since Lipsum uses Phabricator-like rules to generate paths).
  - Before optimizations, this revision spent about 5.5 seconds computing paths.
  - After optimizations, it spends about 275ms.

{F3423414}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12319

Differential Revision: https://secure.phabricator.com/D17424
---
 .../query/PhabricatorOwnersPackageQuery.php   | 15 +++++++------
 .../storage/PhabricatorOwnersPackage.php      | 22 +++++++++++++++++++
 .../owners/storage/PhabricatorOwnersPath.php  | 17 +++++++++-----
 3 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
index 4fbd717a2a..d99768acbb 100644
--- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
+++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
@@ -353,6 +353,9 @@ final class PhabricatorOwnersPackageQuery
     $packages = $this->controlResults;
     $weak_dominion = PhabricatorOwnersPackage::DOMINION_WEAK;
 
+    $path_fragments = PhabricatorOwnersPackage::splitPath($path);
+    $fragment_count = count($path_fragments);
+
     $matches = array();
     foreach ($packages as $package_id => $package) {
       $best_match = null;
@@ -365,13 +368,11 @@ final class PhabricatorOwnersPackageQuery
         continue;
       }
 
-      foreach ($package->getPaths() as $package_path) {
-        if ($package_path->getRepositoryPHID() != $repository_phid) {
-          // If this path is for some other repository, skip it.
-          continue;
-        }
-
-        $strength = $package_path->getPathMatchStrength($path);
+      $repository_paths = $package->getPathsForRepository($repository_phid);
+      foreach ($repository_paths as $package_path) {
+        $strength = $package_path->getPathMatchStrength(
+          $path_fragments,
+          $fragment_count);
         if ($strength > $best_match) {
           $best_match = $strength;
           $include = !$package_path->getExcluded();
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index f2290d2866..1b12365e4f 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -26,6 +26,7 @@ final class PhabricatorOwnersPackage
   private $paths = self::ATTACHABLE;
   private $owners = self::ATTACHABLE;
   private $customFields = self::ATTACHABLE;
+  private $pathRepositoryMap = array();
 
   const STATUS_ACTIVE = 'active';
   const STATUS_ARCHIVED = 'archived';
@@ -369,6 +370,10 @@ final class PhabricatorOwnersPackage
   public function attachPaths(array $paths) {
     assert_instances_of($paths, 'PhabricatorOwnersPath');
     $this->paths = $paths;
+
+    // Drop this cache if we're attaching new paths.
+    $this->pathRepositoryMap = array();
+
     return $this;
   }
 
@@ -376,6 +381,23 @@ final class PhabricatorOwnersPackage
     return $this->assertAttached($this->paths);
   }
 
+  public function getPathsForRepository($repository_phid) {
+    if (isset($this->pathRepositoryMap[$repository_phid])) {
+      return $this->pathRepositoryMap[$repository_phid];
+    }
+
+    $map = array();
+    foreach ($this->getPaths() as $path) {
+      if ($path->getRepositoryPHID() == $repository_phid) {
+        $map[] = $path;
+      }
+    }
+
+    $this->pathRepositoryMap[$repository_phid] = $map;
+
+    return $this->pathRepositoryMap[$repository_phid];
+  }
+
   public function attachOwners(array $owners) {
     assert_instances_of($owners, 'PhabricatorOwnersOwner');
     $this->owners = $owners;
diff --git a/src/applications/owners/storage/PhabricatorOwnersPath.php b/src/applications/owners/storage/PhabricatorOwnersPath.php
index b552a7fb0d..f240db2851 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPath.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPath.php
@@ -7,6 +7,9 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
   protected $path;
   protected $excluded;
 
+  private $fragments;
+  private $fragmentCount;
+
   protected function getConfiguration() {
     return array(
       self::CONFIG_TIMESTAMPS => false,
@@ -74,19 +77,21 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
    * Get the number of directory matches between this path specification and
    * some real path.
    */
-  public function getPathMatchStrength($path) {
-    $this_path = $this->getPath();
+  public function getPathMatchStrength($path_fragments, $path_count) {
+    $this_path = $this->path;
 
     if ($this_path === '/') {
       // The root path "/" just matches everything with strength 1.
       return 1;
     }
 
-    $self_fragments = PhabricatorOwnersPackage::splitPath($this_path);
-    $path_fragments = PhabricatorOwnersPackage::splitPath($path);
+    if ($this->fragments === null) {
+      $this->fragments = PhabricatorOwnersPackage::splitPath($this_path);
+      $this->fragmentCount = count($this->fragments);
+    }
 
-    $self_count = count($self_fragments);
-    $path_count = count($path_fragments);
+    $self_fragments = $this->fragments;
+    $self_count = $this->fragmentCount;
     if ($self_count > $path_count) {
       // If this path is longer (and therefore more specific) than the target
       // path, we don't match it at all.