From e34ee684e1387a83715800d1edfc42d469613c12 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 10 May 2014 16:46:32 -0700 Subject: [PATCH] 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 --- .../ConduitAPI_diffusion_diffquery_Method.php | 26 ++-- ...API_diffusion_lastmodifiedquery_Method.php | 117 +++++++----------- .../DiffusionLastModifiedController.php | 67 +++++----- .../diffusion/query/DiffusionCommitQuery.php | 3 + 4 files changed, 91 insertions(+), 122 deletions(-) diff --git a/src/applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php b/src/applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php index 35cb59854e..6becde1908 100644 --- a/src/applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php +++ b/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; } diff --git a/src/applications/diffusion/conduit/ConduitAPI_diffusion_lastmodifiedquery_Method.php b/src/applications/diffusion/conduit/ConduitAPI_diffusion_lastmodifiedquery_Method.php index 8cfa1f6f55..d0101c4b15 100644 --- a/src/applications/diffusion/conduit/ConduitAPI_diffusion_lastmodifiedquery_Method.php +++ b/src/applications/diffusion/conduit/ConduitAPI_diffusion_lastmodifiedquery_Method.php @@ -1,115 +1,84 @@ '; } protected function defineCustomParamTypes() { return array( - 'commit' => 'required string', - 'path' => 'required string', + 'paths' => 'required map', ); } - 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; } } diff --git a/src/applications/diffusion/controller/DiffusionLastModifiedController.php b/src/applications/diffusion/controller/DiffusionLastModifiedController.php index fd10082af6..7b96f6ab7f 100644 --- a/src/applications/diffusion/controller/DiffusionLastModifiedController.php +++ b/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)); diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 0b8d9cc523..1b7b90d706 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/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); } }