From 58884b94dc0b6e2193612542a2672fa5bb594aef Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 3 Jul 2013 05:39:09 -0700 Subject: [PATCH] Simplify construction and execution of Differential queries for "responsible" users Summary: Currently, when querying for responsible users (revisions where a given user is either the author or a reviewer), we do this: - If the query passes a bunch of hard-coded special cases, use a special hard-coded UNION. - Otherwise, use a very complicated JOIN/WHERE clause. This is bad for several reasons: - Tons and tons of hard-coding and special casing. - The JOIN/WHERE clause performs very poorly for large datasets. - (As a material consequence, the homepage issues a responsible query which barely misses the hard-coded special cases and goes down the slow path.) Instead, //always// use the UNION strategy to execute a "responsible" query. Specifically, if we have responsible PHIDs, temporarily add them to the author list and build a normal query, then repeat for reviewers, then UNION any clauses we built. Fixes T3377. Ref T603. Ref T2625. Depends on D6342. There's various folklore about UNION ALL / UNION DISTINCT performance. UNION DISTINCT is simpler here and the number of rows is small, although we could use UNION ALL in the form: SELECT * FROM ((SELECT ...) UNION ALL (SELECT ...) ORDER) GROUP LIMIT ...if we find that there's some performance benefit at some point. Test Plan: Used DarkConsole to examine queries. Viewed home page and Differential dashboard/authors/subscribers. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603, T2625, T3377 Differential Revision: https://secure.phabricator.com/D6343 --- .../query/DifferentialRevisionQuery.php | 163 ++++++++---------- 1 file changed, 70 insertions(+), 93 deletions(-) diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 2dce0eba79..5015486166 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -376,11 +376,7 @@ final class DifferentialRevisionQuery { $table = new DifferentialRevision(); $conn_r = $table->establishConnection('r'); - if ($this->shouldUseResponsibleFastPath()) { - $data = $this->loadDataUsingResponsibleFastPath(); - } else { - $data = $this->loadData(); - } + $data = $this->loadData(); $revisions = $table->loadAllFromArray($data); @@ -417,65 +413,6 @@ final class DifferentialRevisionQuery { return head($this->execute()); } - - /** - * Determine if we should execute an optimized, fast-path query to fetch - * open revisions for one responsible user. This is used by the Differential - * dashboard and much faster when executed as a UNION ALL than with JOIN - * and WHERE, which is why we special case it. - */ - private function shouldUseResponsibleFastPath() { - if ((count($this->responsibles) == 1) && - ($this->status == self::STATUS_OPEN) && - ($this->order == self::ORDER_MODIFIED) && - !$this->offset && - !$this->limit && - !$this->subscribers && - !$this->reviewers && - !$this->ccs && - !$this->authors && - !$this->revIDs && - !$this->commitHashes && - !$this->phids && - !$this->branches && - !$this->arcanistProjectPHIDs) { - return true; - } - return false; - } - - - private function loadDataUsingResponsibleFastPath() { - $table = new DifferentialRevision(); - $conn_r = $table->establishConnection('r'); - - $responsible_phid = reset($this->responsibles); - $open_statuses = array( - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW, - ArcanistDifferentialRevisionStatus::NEEDS_REVISION, - ArcanistDifferentialRevisionStatus::ACCEPTED, - ); - - return queryfx_all( - $conn_r, - 'SELECT * FROM %T WHERE authorPHID = %s AND status IN (%Ld) - UNION ALL - SELECT r.* FROM %T r JOIN %T rel - ON rel.revisionID = r.id - AND rel.relation = %s - AND rel.objectPHID = %s - WHERE r.status IN (%Ld) ORDER BY dateModified DESC', - $table->getTableName(), - $responsible_phid, - $open_statuses, - - $table->getTableName(), - DifferentialRevision::RELATIONSHIP_TABLE, - DifferentialRevision::RELATION_REVIEWER, - $responsible_phid, - $open_statuses); - } - private function loadData() { $table = new DifferentialRevision(); $conn_r = $table->establishConnection('r'); @@ -506,6 +443,54 @@ final class DifferentialRevisionQuery { } } + $selects = array(); + + // NOTE: If the query includes "responsiblePHIDs", we execute it as a + // UNION of revisions they own and revisions they're reviewing. This has + // much better performance than doing it with JOIN/WHERE. + if ($this->responsibles) { + $basic_authors = $this->authors; + $basic_reviewers = $this->reviewers; + try { + // Build the query where the responsible users are authors. + $this->authors = array_merge($basic_authors, $this->responsibles); + $this->reviewers = $basic_reviewers; + $selects[] = $this->buildSelectStatement($conn_r); + + // Build the query where the responsible users are reviewers. + $this->authors = $basic_authors; + $this->reviewers = array_merge($basic_reviewers, $this->responsibles); + $selects[] = $this->buildSelectStatement($conn_r); + + // Put everything back like it was. + $this->authors = $basic_authors; + $this->reviewers = $basic_reviewers; + } catch (Exception $ex) { + $this->authors = $basic_authors; + $this->reviewers = $basic_reviewers; + throw $ex; + } + } else { + $selects[] = $this->buildSelectStatement($conn_r); + } + + if (count($selects) > 1) { + $query = qsprintf( + $conn_r, + '%Q %Q %Q', + implode(' UNION DISTINCT ', $selects), + $this->buildOrderByClause($conn_r, $is_global = true), + $this->buildLimitClause($conn_r)); + } else { + $query = head($selects); + } + + return queryfx_all($conn_r, '%Q', $query); + } + + private function buildSelectStatement(AphrontDatabaseConnection $conn_r) { + $table = new DifferentialRevision(); + $select = qsprintf( $conn_r, 'SELECT r.* FROM %T r', @@ -515,7 +500,20 @@ final class DifferentialRevisionQuery { $where = $this->buildWhereClause($conn_r); $group_by = $this->buildGroupByClause($conn_r); $order_by = $this->buildOrderByClause($conn_r); + $limit = $this->buildLimitClause($conn_r); + return qsprintf( + $conn_r, + '(%Q %Q %Q %Q %Q %Q)', + $select, + $joins, + $where, + $group_by, + $order_by, + $limit); + } + + private function buildLimitClause(AphrontDatabaseConnection $conn_r) { $limit = ''; if ($this->offset || $this->limit) { $limit = qsprintf( @@ -525,18 +523,9 @@ final class DifferentialRevisionQuery { $this->limit); } - return queryfx_all( - $conn_r, - '%Q %Q %Q %Q %Q %Q', - $select, - $joins, - $where, - $group_by, - $order_by, - $limit); + return $limit; } - /* -( Internals )---------------------------------------------------------- */ @@ -596,17 +585,6 @@ final class DifferentialRevisionQuery { $this->subscribers); } - if ($this->responsibles) { - $joins[] = qsprintf( - $conn_r, - 'LEFT JOIN %T responsibles_rel ON responsibles_rel.revisionID = r.id '. - 'AND responsibles_rel.relation = %s '. - 'AND responsibles_rel.objectPHID in (%Ls)', - DifferentialRevision::RELATIONSHIP_TABLE, - DifferentialRevision::RELATION_REVIEWER, - $this->responsibles); - } - $joins = implode(' ', $joins); return $joins; @@ -675,13 +653,6 @@ final class DifferentialRevisionQuery { $this->phids); } - if ($this->responsibles) { - $where[] = qsprintf( - $conn_r, - '(responsibles_rel.objectPHID IS NOT NULL OR r.authorPHID IN (%Ls))', - $this->responsibles); - } - if ($this->branches) { $where[] = qsprintf( $conn_r, @@ -786,11 +757,17 @@ final class DifferentialRevisionQuery { /** * @task internal */ - private function buildOrderByClause($conn_r) { + private function buildOrderByClause($conn_r, $is_global = false) { switch ($this->order) { case self::ORDER_MODIFIED: + if ($is_global) { + return 'ORDER BY dateModified DESC'; + } return 'ORDER BY r.dateModified DESC'; case self::ORDER_CREATED: + if ($is_global) { + return 'ORDER BY dateCreated DESC'; + } return 'ORDER BY r.dateCreated DESC'; case self::ORDER_PATH_MODIFIED: if (!$this->pathIDs) {