Improve performance of differential.query

Summary:
  - Instead of running (1 + N * 4) queries, run 5 queries.
  - Add 'dateModified' field.

Test Plan:
  - Ran various queries against various revisions, data seemed unchanged.
  - Checked query log.

Reviewers: nh, jonathanhester, jungejason, btrahan

Reviewed By: jonathanhester

CC: aran, epriestley, jonathanhester, jungejason

Differential Revision: 1252

src/applications/conduit/method/differential/query/ConduitAPI_differential_query_Method.php

@@ -124,36 +124,40 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
       $query->withSubscribers($subscribers);
     }
 
+    $query->needRelationships(true);
+    $query->needCommitPHIDs(true);
+    $query->needDiffIDs(true);
+    $query->needActiveDiffs(true);
+
     $revisions = $query->execute();
 
     $results = array();
     foreach ($revisions as $revision) {
-      $diff = $revision->loadActiveDiff();
+      $diff = $revision->getActiveDiff();
       if (!$diff) {
         continue;
       }
 
-      $revision->loadRelationships();
-
       $id = $revision->getID();
       $results[] = array(
-        'id'          => $id,
-        'phid'        => $revision->getPHID(),
-        'title'       => $revision->getTitle(),
-        'uri'         => PhabricatorEnv::getProductionURI('/D'.$id),
-        'dateCreated' => $revision->getDateCreated(),
-        'authorPHID'  => $revision->getAuthorPHID(),
-        'status'      => $revision->getStatus(),
-        'statusName'  => DifferentialRevisionStatus::getNameForRevisionStatus(
+        'id'            => $id,
+        'phid'          => $revision->getPHID(),
+        'title'         => $revision->getTitle(),
+        'uri'           => PhabricatorEnv::getProductionURI('/D'.$id),
+        'dateCreated'   => $revision->getDateCreated(),
+        'dateModified'  => $revision->getDateModified(),
+        'authorPHID'    => $revision->getAuthorPHID(),
+        'status'        => $revision->getStatus(),
+        'statusName'    => DifferentialRevisionStatus::getNameForRevisionStatus(
           $revision->getStatus()),
-        'sourcePath'  => $diff->getSourcePath(),
-        'summary'     => $revision->getSummary(),
-        'testPlan'    => $revision->getTestPlan(),
-        'lineCount'   => $revision->getLineCount(),
-        'diffs'       => array_keys($revision->loadDiffs()),
-        'commits'     => $revision->loadCommitPHIDs(),
-        'reviewers'   => array_values($revision->getReviewers()),
-        'ccs'         => array_values($revision->getCCPHIDs()),
+        'sourcePath'    => $diff->getSourcePath(),
+        'summary'       => $revision->getSummary(),
+        'testPlan'      => $revision->getTestPlan(),
+        'lineCount'     => $revision->getLineCount(),
+        'diffs'         => $revision->getDiffIDs(),
+        'commits'       => $revision->getCommitPHIDs(),
+        'reviewers'     => array_values($revision->getReviewers()),
+        'ccs'           => array_values($revision->getCCPHIDs()),
       );
     }
 

src/applications/differential/query/revision/DifferentialRevisionQuery.php

@@ -61,7 +61,10 @@ final class DifferentialRevisionQuery {
   private $limit  = 1000;
   private $offset = 0;
 
-  private $needRelationships = false;
+  private $needRelationships  = false;
+  private $needActiveDiffs    = false;
+  private $needDiffIDs        = false;
+  private $needCommitPHIDs    = false;
 
 
 /* -(  Query Configuration  )------------------------------------------------ */
@@ -245,6 +248,48 @@ final class DifferentialRevisionQuery {
   }
 
 
+  /**
+   * Set whether or not the query should load the active diff for each
+   * revision.
+   *
+   * @param bool True to load and attach diffs.
+   * @return this
+   * @task config
+   */
+  public function needActiveDiffs($need_active_diffs) {
+    $this->needActiveDiffs = $need_active_diffs;
+    return $this;
+  }
+
+
+  /**
+   * Set whether or not the query should load the associated commit PHIDs for
+   * each revision.
+   *
+   * @param bool True to load and attach diffs.
+   * @return this
+   * @task config
+   */
+  public function needCommitPHIDs($need_commit_phids) {
+    $this->needCommitPHIDs = $need_commit_phids;
+    return $this;
+  }
+
+
+  /**
+   * Set whether or not the query should load associated diff IDs for each
+   * revision.
+   *
+   * @param bool True to load and attach diff IDs.
+   * @return this
+   * @task config
+   */
+  public function needDiffIDs($need_diff_ids) {
+    $this->needDiffIDs = $need_diff_ids;
+    return $this;
+  }
+
+
 /* -(  Query Execution  )---------------------------------------------------- */
 
 
@@ -267,19 +312,21 @@ final class DifferentialRevisionQuery {
 
     $revisions = $table->loadAllFromArray($data);
 
-    if ($revisions && $this->needRelationships) {
-      $relationships = queryfx_all(
-        $conn_r,
-        'SELECT * FROM %T WHERE revisionID in (%Ld) ORDER BY sequence',
-        DifferentialRevision::RELATIONSHIP_TABLE,
-        mpull($revisions, 'getID'));
-      $relationships = igroup($relationships, 'revisionID');
-      foreach ($revisions as $revision) {
-        $revision->attachRelationships(
-          idx(
-            $relationships,
-            $revision->getID(),
-            array()));
+    if ($revisions) {
+      if ($this->needRelationships) {
+        $this->loadRelationships($conn_r, $revisions);
+      }
+
+      if ($this->needCommitPHIDs) {
+        $this->loadCommitPHIDs($conn_r, $revisions);
+      }
+
+      if ($this->needActiveDiffs || $this->needDiffIDs) {
+        $this->loadDiffIDs($conn_r, $revisions);
+      }
+
+      if ($this->needActiveDiffs) {
+        $this->loadActiveDiffs($conn_r, $revisions);
       }
     }
 
@@ -563,5 +610,77 @@ final class DifferentialRevisionQuery {
     }
   }
 
+  private function loadRelationships($conn_r, array $revisions) {
+    $relationships = queryfx_all(
+      $conn_r,
+      'SELECT * FROM %T WHERE revisionID in (%Ld) ORDER BY sequence',
+      DifferentialRevision::RELATIONSHIP_TABLE,
+      mpull($revisions, 'getID'));
+    $relationships = igroup($relationships, 'revisionID');
+    foreach ($revisions as $revision) {
+      $revision->attachRelationships(
+        idx(
+          $relationships,
+          $revision->getID(),
+          array()));
+    }
+  }
+
+  private function loadCommitPHIDs($conn_r, array $revisions) {
+    $commit_phids = queryfx_all(
+      $conn_r,
+      'SELECT * FROM %T WHERE revisionID IN (%Ld)',
+      DifferentialRevision::TABLE_COMMIT,
+      mpull($revisions, 'getID'));
+    $commit_phids = igroup($commit_phids, 'revisionID');
+    foreach ($revisions as $revision) {
+      $phids = idx($commit_phids, $revision->getID(), array());
+      $phids = ipull($phids, 'commitPHID');
+      $revision->attachCommitPHIDs($phids);
+    }
+  }
+
+  private function loadDiffIDs($conn_r, array $revisions) {
+    $diff_table = new DifferentialDiff();
+
+    $diff_ids = queryfx_all(
+      $conn_r,
+      'SELECT revisionID, id FROM %T WHERE revisionID IN (%Ld)
+        ORDER BY id DESC',
+      $diff_table->getTableName(),
+      mpull($revisions, 'getID'));
+    $diff_ids = igroup($diff_ids, 'revisionID');
+
+    foreach ($revisions as $revision) {
+      $ids = idx($diff_ids, $revision->getID(), array());
+      $ids = ipull($ids, 'id');
+      $revision->attachDiffIDs($ids);
+    }
+  }
+
+  private function loadActiveDiffs($conn_r, array $revisions) {
+    $diff_table = new DifferentialDiff();
+
+    $load_ids = array();
+    foreach ($revisions as $revision) {
+      $diffs = $revision->getDiffIDs();
+      if ($diffs) {
+        $load_ids[] = max($diffs);
+      }
+    }
+
+    $active_diffs = array();
+    if ($load_ids) {
+      $active_diffs = $diff_table->loadAllWhere(
+        'id IN (%Ld)',
+        $load_ids);
+    }
+
+    $active_diffs = mpull($active_diffs, null, 'getRevisionID');
+    foreach ($revisions as $revision) {
+      $revision->attachActiveDiff(idx($active_diffs, $revision->getID()));
+    }
+  }
+
 
 }

src/applications/differential/query/revision/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus');
 phutil_require_module('phabricator', 'applications/differential/storage/affectedpath');
+phutil_require_module('phabricator', 'applications/differential/storage/diff');
 phutil_require_module('phabricator', 'applications/differential/storage/revision');
 phutil_require_module('phabricator', 'storage/qsprintf');
 phutil_require_module('phabricator', 'storage/queryfx');

src/applications/differential/storage/revision/DifferentialRevision.php

@@ -37,6 +37,8 @@ class DifferentialRevision extends DifferentialDAO {
 
   private $relationships;
   private $commits;
+  private $activeDiff = false;
+  private $diffIDs;
 
   const RELATIONSHIP_TABLE    = 'differential_relationship';
   const TABLE_COMMIT          = 'differential_commit';
@@ -71,11 +73,46 @@ class DifferentialRevision extends DifferentialDAO {
 
   public function getCommitPHIDs() {
     if ($this->commits === null) {
-      throw new Exception("Must load commits!");
+      throw new Exception("Must attach commits first!");
     }
     return $this->commits;
   }
 
+  public function getActiveDiff() {
+    // TODO: Because it's currently technically possible to create a revision
+    // without an associated diff, we allow an attached-but-null active diff.
+    // It would be good to get rid of this once we make diff-attaching
+    // transactional.
+
+    if ($this->activeDiff === false) {
+      throw new Exception("Must attach active diff first!");
+    }
+    return $this->activeDiff;
+  }
+
+  public function attachActiveDiff($diff) {
+    $this->activeDiff = $diff;
+    return $this;
+  }
+
+  public function getDiffIDs() {
+    if ($this->diffIDs === null) {
+      throw new Exception("Must attach diff IDs first!");
+    }
+    return $this->diffIDs;
+  }
+
+  public function attachDiffIDs(array $ids) {
+    rsort($ids);
+    $this->diffIDs = array_values($ids);
+    return $this;
+  }
+
+  public function attachCommitPHIDs(array $phids) {
+    $this->commits = array_values($phids);
+    return $this;
+  }
+
   public function getAttachedPHIDs($type) {
     return array_keys(idx($this->attached, $type, array()));
   }

src/applications/phid/handle/data/PhabricatorObjectHandleData.php

@@ -234,10 +234,14 @@ class PhabricatorObjectHandleData {
           $commits = $object->loadAllWhere('phid in (%Ls)', $phids);
           $commits = mpull($commits, null, 'getPHID');
 
-          $repository_ids = mpull($commits, 'getRepositoryID');
-          $repositories = id(new PhabricatorRepository())->loadAllWhere(
-            'id in (%Ld)', array_unique($repository_ids));
-          $callsigns = mpull($repositories, 'getCallsign');
+          $repository_ids = array();
+          $callsigns = array();
+          if ($commits) {
+            $repository_ids = mpull($commits, 'getRepositoryID');
+            $repositories = id(new PhabricatorRepository())->loadAllWhere(
+              'id in (%Ld)', array_unique($repository_ids));
+            $callsigns = mpull($repositories, 'getCallsign');
+          }
 
           foreach ($phids as $phid) {
             $handle = new PhabricatorObjectHandle();