From 5f26dd9b6660bd89b7e3dbe3ba56cf8db0703c9e Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 8 Dec 2016 06:39:33 -0800 Subject: [PATCH] Use futures to improve clustered repository main page performance Summary: Ref T11954. In cluster configurations, we get repository information by making HTTP calls over Conduit. These are slower than local calls, so clustering imposes a performance penalty. However, we can use futures and parallelize them so that clustering actually improves overall performance. When not running in clustered mode, this just makes us run stuff inline. Test Plan: - Browsed Git, Mercurial and Subversion repositories. - Locally, saw a 700ms wall time page drop to 200ms. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11954 Differential Revision: https://secure.phabricator.com/D17009 --- .../storage/PhabricatorConduitToken.php | 25 ++- .../controller/DiffusionController.php | 12 ++ .../DiffusionRepositoryController.php | 145 +++++++++++++----- .../diffusion/query/DiffusionQuery.php | 14 +- .../people/storage/PhabricatorUser.php | 14 ++ 5 files changed, 158 insertions(+), 52 deletions(-) diff --git a/src/applications/conduit/storage/PhabricatorConduitToken.php b/src/applications/conduit/storage/PhabricatorConduitToken.php index f5673fdab3..aa836cf623 100644 --- a/src/applications/conduit/storage/PhabricatorConduitToken.php +++ b/src/applications/conduit/storage/PhabricatorConduitToken.php @@ -42,6 +42,10 @@ final class PhabricatorConduitToken return null; } + if ($user->hasConduitClusterToken()) { + return $user->getConduitClusterToken(); + } + $tokens = id(new PhabricatorConduitTokenQuery()) ->setViewer($user) ->withObjectPHIDs(array($user->getPHID())) @@ -55,23 +59,28 @@ final class PhabricatorConduitToken $now = PhabricatorTime::getNow(); $must_expire_after = $now + phutil_units('5 minutes in seconds'); + $valid_token = null; foreach ($tokens as $token) { if ($token->getExpires() > $must_expire_after) { - return $token; + $valid_token = $token; + break; } } // We didn't find any existing tokens (or the existing tokens are all about // to expire) so generate a new token. + if (!$valid_token) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $valid_token = self::initializeNewToken( + $user->getPHID(), + self::TYPE_CLUSTER); + $valid_token->save(); + unset($unguarded); + } - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $token = self::initializeNewToken( - $user->getPHID(), - self::TYPE_CLUSTER); - $token->save(); - unset($unguarded); + $user->attachConduitClusterToken($valid_token); - return $token; + return $valid_token; } public static function initializeNewToken($object_phid, $token_type) { diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index 14d4f05789..61c17e0877 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -239,6 +239,18 @@ abstract class DiffusionController extends PhabricatorController { $params); } + protected function callConduitMethod($method, array $params = array()) { + $user = $this->getViewer(); + $drequest = $this->getDiffusionRequest(); + + return DiffusionQuery::callConduitWithDiffusionRequest( + $user, + $drequest, + $method, + $params, + true); + } + protected function getRepositoryControllerURI( PhabricatorRepository $repository, $path) { diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php index d38ad77787..ae6a656bd2 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php @@ -2,6 +2,11 @@ final class DiffusionRepositoryController extends DiffusionController { + private $historyFuture; + private $browseFuture; + private $tagFuture; + private $branchFuture; + public function shouldAllowPublic() { return true; } @@ -106,18 +111,67 @@ final class DiffusionRepositoryController extends DiffusionController { $request = $this->getRequest(); $repository = $drequest->getRepository(); + $commit = $drequest->getCommit(); + $path = $drequest->getPath(); + + $this->historyFuture = $this->callConduitMethod( + 'diffusion.historyquery', + array( + 'commit' => $commit, + 'path' => $path, + 'offset' => 0, + 'limit' => 15, + )); + + $browse_pager = id(new PHUIPagerView()) + ->readFromRequest($request); + + $this->browseFuture = $this->callConduitMethod( + 'diffusion.browsequery', + array( + 'commit' => $commit, + 'path' => $path, + 'limit' => $browse_pager->getPageSize() + 1, + )); + + if ($this->needTagFuture()) { + $tag_limit = $this->getTagLimit(); + $this->tagFuture = $this->callConduitMethod( + 'diffusion.tagsquery', + array( + // On the home page, we want to find tags on any branch. + 'commit' => null, + 'limit' => $tag_limit + 1, + )); + } + + if ($this->needBranchFuture()) { + $branch_limit = $this->getBranchLimit(); + $this->branchFuture = $this->callConduitMethod( + 'diffusion.branchquery', + array( + 'closed' => false, + 'limit' => $branch_limit + 1, + )); + } + + $futures = array( + $this->historyFuture, + $this->browseFuture, + $this->tagFuture, + $this->branchFuture, + ); + $futures = array_filter($futures); + $futures = new FutureIterator($futures); + foreach ($futures as $future) { + // Just resolve all the futures before continuing. + } + $phids = array(); $content = array(); try { - $history_results = $this->callConduitWithDiffusionRequest( - 'diffusion.historyquery', - array( - 'commit' => $drequest->getCommit(), - 'path' => $drequest->getPath(), - 'offset' => 0, - 'limit' => 15, - )); + $history_results = $this->historyFuture->resolve(); $history = DiffusionPathChange::newFromConduit( $history_results['pathChanges']); @@ -139,18 +193,11 @@ final class DiffusionRepositoryController extends DiffusionController { $history_exception = $ex; } - $browse_pager = id(new PHUIPagerView()) - ->readFromRequest($request); - try { + $browse_results = $this->browseFuture->resolve(); $browse_results = DiffusionBrowseResultSet::newFromConduit( - $this->callConduitWithDiffusionRequest( - 'diffusion.browsequery', - array( - 'path' => $drequest->getPath(), - 'commit' => $drequest->getCommit(), - 'limit' => $browse_pager->getPageSize() + 1, - ))); + $browse_results); + $browse_paths = $browse_results->getPaths(); $browse_paths = $browse_pager->sliceResults($browse_paths); @@ -366,22 +413,16 @@ final class DiffusionRepositoryController extends DiffusionController { private function buildBranchListTable(DiffusionRequest $drequest) { $viewer = $this->getViewer(); - if ($drequest->getBranch() === null) { + if (!$this->needBranchFuture()) { return null; } - $limit = 15; - - $branches = $this->callConduitWithDiffusionRequest( - 'diffusion.branchquery', - array( - 'closed' => false, - 'limit' => $limit + 1, - )); + $branches = $this->branchFuture->resolve(); if (!$branches) { return null; } + $limit = $this->getBranchLimit(); $more_branches = (count($branches) > $limit); $branches = array_slice($branches, 0, $limit); @@ -428,26 +469,17 @@ final class DiffusionRepositoryController extends DiffusionController { $viewer = $this->getViewer(); $repository = $drequest->getRepository(); - switch ($repository->getVersionControlSystem()) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - // no tags in SVN - return null; + if (!$this->needTagFuture()) { + return null; } - $tag_limit = 15; - $tags = array(); - $tags = DiffusionRepositoryTag::newFromConduit( - $this->callConduitWithDiffusionRequest( - 'diffusion.tagsquery', - array( - // On the home page, we want to find tags on any branch. - 'commit' => null, - 'limit' => $tag_limit + 1, - ))); + $tags = $this->tagFuture->resolve(); + $tags = DiffusionRepositoryTag::newFromConduit($tags); if (!$tags) { return null; } + $tag_limit = $this->getTagLimit(); $more_tags = (count($tags) > $tag_limit); $tags = array_slice($tags, 0, $tag_limit); @@ -688,4 +720,35 @@ final class DiffusionRepositoryController extends DiffusionController { ->setDisplayURI($display); } + private function needTagFuture() { + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + switch ($repository->getVersionControlSystem()) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + // No tags in SVN. + return false; + } + + return true; + } + + private function getTagLimit() { + return 15; + } + + private function needBranchFuture() { + $drequest = $this->getDiffusionRequest(); + + if ($drequest->getBranch() === null) { + return false; + } + + return true; + } + + private function getBranchLimit() { + return 15; + } + } diff --git a/src/applications/diffusion/query/DiffusionQuery.php b/src/applications/diffusion/query/DiffusionQuery.php index 409bf68980..6675197932 100644 --- a/src/applications/diffusion/query/DiffusionQuery.php +++ b/src/applications/diffusion/query/DiffusionQuery.php @@ -48,7 +48,8 @@ abstract class DiffusionQuery extends PhabricatorQuery { PhabricatorUser $user, DiffusionRequest $drequest, $method, - array $params = array()) { + array $params = array(), + $return_future = false) { $repository = $drequest->getRepository(); @@ -76,12 +77,19 @@ abstract class DiffusionQuery extends PhabricatorQuery { $user, $drequest->getIsClusterRequest()); if (!$client) { - return id(new ConduitCall($method, $params)) + $result = id(new ConduitCall($method, $params)) ->setUser($user) ->execute(); + $future = new ImmediateFuture($result); } else { - return $client->callMethodSynchronous($method, $params); + $future = $client->callMethod($method, $params); } + + if (!$return_future) { + return $future->resolve(); + } + + return $future; } public function execute() { diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index b30f3cac13..61b2209c24 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -64,6 +64,7 @@ final class PhabricatorUser private $settingCacheKeys = array(); private $settingCache = array(); private $allowInlineCacheGeneration; + private $conduitClusterToken = self::ATTACHABLE; protected function readField($field) { switch ($field) { @@ -937,6 +938,19 @@ final class PhabricatorUser return $this->authorities; } + public function hasConduitClusterToken() { + return ($this->conduitClusterToken !== self::ATTACHABLE); + } + + public function attachConduitClusterToken(PhabricatorConduitToken $token) { + $this->conduitClusterToken = $token; + return $this; + } + + public function getConduitClusterToken() { + return $this->assertAttached($this->conduitClusterToken); + } + /* -( Availability )------------------------------------------------------- */ -- 2.51.2