From a8104699291c87a12f5edd941db3a2e8ae7422e5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 20 Dec 2011 17:05:52 -0800 Subject: [PATCH] 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 --- .../ConduitAPI_differential_query_Method.php | 42 ++--- .../revision/DifferentialRevisionQuery.php | 147 ++++++++++++++++-- .../differential/query/revision/__init__.php | 1 + .../storage/revision/DifferentialRevision.php | 39 ++++- .../data/PhabricatorObjectHandleData.php | 12 +- 5 files changed, 203 insertions(+), 38 deletions(-) diff --git a/src/applications/conduit/method/differential/query/ConduitAPI_differential_query_Method.php b/src/applications/conduit/method/differential/query/ConduitAPI_differential_query_Method.php index fb9202340f..73b30f007a 100644 --- a/src/applications/conduit/method/differential/query/ConduitAPI_differential_query_Method.php +++ b/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()), ); } diff --git a/src/applications/differential/query/revision/DifferentialRevisionQuery.php b/src/applications/differential/query/revision/DifferentialRevisionQuery.php index c6c58d00dc..2438a69fc3 100644 --- a/src/applications/differential/query/revision/DifferentialRevisionQuery.php +++ b/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())); + } + } + } diff --git a/src/applications/differential/query/revision/__init__.php b/src/applications/differential/query/revision/__init__.php index 52e0f233b8..b15dbd2138 100644 --- a/src/applications/differential/query/revision/__init__.php +++ b/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'); diff --git a/src/applications/differential/storage/revision/DifferentialRevision.php b/src/applications/differential/storage/revision/DifferentialRevision.php index d16452ae65..edd1168112 100644 --- a/src/applications/differential/storage/revision/DifferentialRevision.php +++ b/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())); } diff --git a/src/applications/phid/handle/data/PhabricatorObjectHandleData.php b/src/applications/phid/handle/data/PhabricatorObjectHandleData.php index 74d9a433da..c8c9e75d1a 100644 --- a/src/applications/phid/handle/data/PhabricatorObjectHandleData.php +++ b/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();