Batch execution of LastModified query

Summary:
Ref T2683. Further reduces query count of last modified loads; we're now at 11 instead of 200+.

(This works in SVN but could be further optimized.)

Test Plan:
Loaded SVN, Mercurial, Git:

{F34864}
{F34865}
{F34866}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, vrana, aran

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D5256

src/applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php

@@ -30,7 +30,8 @@ final class ConduitAPI_diffusion_diffquery_Method
 
     return array(
       'changes' => mpull($result, 'toDictionary'),
-      'effectiveCommit' => $this->getEffectiveCommit($request));
+      'effectiveCommit' => $this->getEffectiveCommit($request),
+    );
   }
 
   protected function getGitResult(ConduitAPIRequest $request) {
@@ -160,26 +161,19 @@ final class ConduitAPI_diffusion_diffquery_Method
   private function getEffectiveCommit(ConduitAPIRequest $request) {
     if ($this->effectiveCommit === null) {
       $drequest = $this->getDiffusionRequest();
-      $user = $request->getUser();
-      $commit = null;
 
-      $conduit_result = DiffusionQuery::callConduitWithDiffusionRequest(
-        $user,
+      $path = $drequest->getPath();
+      $result = DiffusionQuery::callConduitWithDiffusionRequest(
+        $request->getUser(),
         $drequest,
         'diffusion.lastmodifiedquery',
         array(
-          'commit' => $drequest->getCommit(),
-          'path' => $drequest->getPath()));
-      $c_dict = $conduit_result['commit'];
-      if ($c_dict) {
-        $commit = PhabricatorRepositoryCommit::newFromDictionary($c_dict);
-      }
-      if (!$commit) {
-        // TODO: Improve error messages here.
-        return null;
-      }
-      $this->effectiveCommit = $commit->getCommitIdentifier();
+          'paths' => array($path => $drequest->getCommit()),
+        ));
+
+      $this->effectiveCommit = idx($result, $path);
     }
+
     return $this->effectiveCommit;
   }
 

src/applications/diffusion/conduit/ConduitAPI_diffusion_lastmodifiedquery_Method.php

@@ -1,115 +1,84 @@
 <?php
 
-/**
- * @group conduit
- */
 final class ConduitAPI_diffusion_lastmodifiedquery_Method
   extends ConduitAPI_diffusion_abstractquery_Method {
 
   public function getMethodDescription() {
-    return
-      'Get last modified information from a repository for a specific commit '.
-      'at a specific path.';
+    return pht('Get the commits at which paths were last modified.');
   }
 
   public function defineReturnType() {
-    return 'array';
+    return 'map<string, string>';
   }
 
   protected function defineCustomParamTypes() {
     return array(
-      'commit' => 'required string',
-      'path' => 'required string',
+      'paths' => 'required map<string, string>',
     );
   }
 
-  protected function getResult(ConduitAPIRequest $request) {
-    list($commit, $commit_data) = parent::getResult($request);
-    if ($commit) {
-      $commit = $commit->toDictionary();
-    }
-    if ($commit_data) {
-      $commit_data = $commit_data->toDictionary();
-    }
-    return array(
-      'commit' => $commit,
-      'commitData' => $commit_data);
-  }
-
   protected function getGitResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
 
-    list($hash) = $repository->execxLocalCommand(
-      'log -n1 --format=%%H %s -- %s',
-      $drequest->getCommit(),
-      $drequest->getPath());
-    $hash = trim($hash);
+    $result = array();
+    foreach ($request->getValue('paths') as $path => $commit) {
+      list($hash) = $repository->execxLocalCommand(
+        'log -n1 --format=%%H %s -- %s',
+        $commit,
+        $path);
+      $result[$path] = trim($hash);
+    }
 
-    return $this->loadDataFromHash($hash);
+    return $result;
   }
 
   protected function getSVNResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
 
-    $path = $drequest->getPath();
+    $result = array();
+    foreach ($request->getValue('paths') as $path => $commit) {
+      $history_result = DiffusionQuery::callConduitWithDiffusionRequest(
+        $request->getUser(),
+        $drequest,
+        'diffusion.historyquery',
+        array(
+          'commit' => $commit,
+          'path' => $path,
+          'limit' => 1,
+          'offset' => 0,
+          'needDirectChanges' => true,
+          'needChildChanges' => true,
+        ));
 
-    $history_result = DiffusionQuery::callConduitWithDiffusionRequest(
-      $request->getUser(),
-      $drequest,
-      'diffusion.historyquery',
-      array(
-        'commit' => $drequest->getCommit(),
-        'path' => $path,
-        'limit' => 1,
-        'offset' => 0,
-        'needDirectChanges' => true,
-        'needChildChanges' => true));
-    $history_array = DiffusionPathChange::newFromConduit(
-      $history_result['pathChanges']);
-
-    if (!$history_array) {
-      return array(array(), array());
+      $history_array = DiffusionPathChange::newFromConduit(
+        $history_result['pathChanges']);
+      if ($history_array) {
+        $result[$path] = head($history_array)
+          ->getCommit()
+          ->getCommitIdentifier();
+      }
     }
 
-    $history = reset($history_array);
-
-    return array($history->getCommit(), $history->getCommitData());
+    return $result;
   }
 
   protected function getMercurialResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
 
-    $path = $drequest->getPath();
-
-    list($hash) = $repository->execxLocalCommand(
-      'log --template %s --limit 1 --removed --rev %s -- %s',
-      '{node}',
-      hgsprintf('reverse(ancestors(%s))',  $drequest->getCommit()),
-      nonempty(ltrim($path, '/'), '.'));
-
-    return $this->loadDataFromHash($hash);
-  }
-
-  private function loadDataFromHash($hash) {
-    $drequest = $this->getDiffusionRequest();
-    $repository = $drequest->getRepository();
-
-    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-      'repositoryID = %d AND commitIdentifier = %s',
-      $repository->getID(),
-      $hash);
-
-    if ($commit) {
-      $commit_data = $commit->loadCommitData();
-    } else {
-      $commit = array();
-      $commit_data = array();
+    $result = array();
+    foreach ($request->getValue('paths') as $path => $commit) {
+      list($hash) = $repository->execxLocalCommand(
+        'log --template %s --limit 1 --removed --rev %s -- %s',
+        '{node}',
+        hgsprintf('reverse(ancestors(%s))',  $commit),
+        nonempty(ltrim($path, '/'), '.'));
+      $result[$path] = trim($hash);
     }
 
-    return array($commit, $commit_data);
+    return $result;
   }
 
 }

src/applications/diffusion/controller/DiffusionLastModifiedController.php

@@ -9,6 +9,7 @@ final class DiffusionLastModifiedController extends DiffusionController {
   public function processRequest() {
     $drequest = $this->getDiffusionRequest();
     $request = $this->getRequest();
+    $viewer = $request->getUser();
 
     $paths = $request->getStr('paths');
     $paths = json_decode($paths, true);
@@ -16,44 +17,46 @@ final class DiffusionLastModifiedController extends DiffusionController {
       return new Aphront400Response();
     }
 
-    $commits = array();
-    foreach ($paths as $path) {
-      $prequest = clone $drequest;
-      $prequest->setPath($path);
+    $modified_map = $this->callConduitWithDiffusionRequest(
+      'diffusion.lastmodifiedquery',
+      array(
+        'paths' => array_fill_keys($paths, $drequest->getCommit()),
+      ));
 
-      $conduit_result = $this->callConduitWithDiffusionRequest(
-        'diffusion.lastmodifiedquery',
-        array(
-          'commit' => $prequest->getCommit(),
-          'path' => $prequest->getPath(),
-        ));
-
-      $commit = PhabricatorRepositoryCommit::newFromDictionary(
-        $conduit_result['commit']);
-
-      $commit_data = PhabricatorRepositoryCommitData::newFromDictionary(
-        $conduit_result['commitData']);
-
-      $commit->attachCommitData($commit_data);
-
-      $phids = array();
-      if ($commit_data) {
-        if ($commit_data->getCommitDetail('authorPHID')) {
-          $phids[$commit_data->getCommitDetail('authorPHID')] = true;
-        }
-        if ($commit_data->getCommitDetail('committerPHID')) {
-          $phids[$commit_data->getCommitDetail('committerPHID')] = true;
-        }
-      }
-
-      $commits[$path] = $commit;
+    if ($modified_map) {
+      $commit_map = id(new DiffusionCommitQuery())
+        ->setViewer($viewer)
+        ->withRepository($drequest->getRepository())
+        ->withIdentifiers(array_values($modified_map))
+        ->needCommitData(true)
+        ->execute();
+      $commit_map = mpull($commit_map, null, 'getCommitIdentifier');
+    } else {
+      $commit_map = array();
     }
 
-    $phids = array_keys($phids);
+    $commits = array();
+    foreach ($paths as $path) {
+      $modified_at = idx($modified_map, $path);
+      if ($modified_at) {
+        $commit = idx($commit_map, $modified_at);
+        if ($commit) {
+          $commits[$path] = $commit;
+        }
+      }
+    }
+
+    $phids = array();
+    foreach ($commits as $commit) {
+      $data = $commit->getCommitData();
+      $phids[] = $data->getCommitDetail('authorPHID');
+      $phids[] = $data->getCommitDetail('committerPHID');
+    }
+    $phids = array_filter($phids);
     $handles = $this->loadViewerHandles($phids);
 
     $branch = $drequest->loadBranch();
-    if ($branch) {
+    if ($branch && $commits) {
       $lint_query = id(new DiffusionLintCountQuery())
         ->withBranchIDs(array($branch->getID()))
         ->withPaths(array_keys($commits));

src/applications/diffusion/query/DiffusionCommitQuery.php

@@ -250,6 +250,9 @@ final class DiffusionCommitQuery
       $data = mpull($data, null, 'getCommitID');
       foreach ($commits as $commit) {
         $commit_data = idx($data, $commit->getID());
+        if (!$commit_data) {
+          $commit_data = new PhabricatorRepositoryCommitData();
+        }
         $commit->attachCommitData($commit_data);
       }
     }