mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 05:50:55 +01:00
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
This commit is contained in:
parent
e5569bdd3a
commit
58884b94dc
1 changed files with 70 additions and 93 deletions
|
@ -376,11 +376,7 @@ final class DifferentialRevisionQuery {
|
||||||
$table = new DifferentialRevision();
|
$table = new DifferentialRevision();
|
||||||
$conn_r = $table->establishConnection('r');
|
$conn_r = $table->establishConnection('r');
|
||||||
|
|
||||||
if ($this->shouldUseResponsibleFastPath()) {
|
$data = $this->loadData();
|
||||||
$data = $this->loadDataUsingResponsibleFastPath();
|
|
||||||
} else {
|
|
||||||
$data = $this->loadData();
|
|
||||||
}
|
|
||||||
|
|
||||||
$revisions = $table->loadAllFromArray($data);
|
$revisions = $table->loadAllFromArray($data);
|
||||||
|
|
||||||
|
@ -417,65 +413,6 @@ final class DifferentialRevisionQuery {
|
||||||
return head($this->execute());
|
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() {
|
private function loadData() {
|
||||||
$table = new DifferentialRevision();
|
$table = new DifferentialRevision();
|
||||||
$conn_r = $table->establishConnection('r');
|
$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(
|
$select = qsprintf(
|
||||||
$conn_r,
|
$conn_r,
|
||||||
'SELECT r.* FROM %T r',
|
'SELECT r.* FROM %T r',
|
||||||
|
@ -515,7 +500,20 @@ final class DifferentialRevisionQuery {
|
||||||
$where = $this->buildWhereClause($conn_r);
|
$where = $this->buildWhereClause($conn_r);
|
||||||
$group_by = $this->buildGroupByClause($conn_r);
|
$group_by = $this->buildGroupByClause($conn_r);
|
||||||
$order_by = $this->buildOrderByClause($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 = '';
|
$limit = '';
|
||||||
if ($this->offset || $this->limit) {
|
if ($this->offset || $this->limit) {
|
||||||
$limit = qsprintf(
|
$limit = qsprintf(
|
||||||
|
@ -525,18 +523,9 @@ final class DifferentialRevisionQuery {
|
||||||
$this->limit);
|
$this->limit);
|
||||||
}
|
}
|
||||||
|
|
||||||
return queryfx_all(
|
return $limit;
|
||||||
$conn_r,
|
|
||||||
'%Q %Q %Q %Q %Q %Q',
|
|
||||||
$select,
|
|
||||||
$joins,
|
|
||||||
$where,
|
|
||||||
$group_by,
|
|
||||||
$order_by,
|
|
||||||
$limit);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
/* -( Internals )---------------------------------------------------------- */
|
/* -( Internals )---------------------------------------------------------- */
|
||||||
|
|
||||||
|
|
||||||
|
@ -596,17 +585,6 @@ final class DifferentialRevisionQuery {
|
||||||
$this->subscribers);
|
$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);
|
$joins = implode(' ', $joins);
|
||||||
|
|
||||||
return $joins;
|
return $joins;
|
||||||
|
@ -675,13 +653,6 @@ final class DifferentialRevisionQuery {
|
||||||
$this->phids);
|
$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) {
|
if ($this->branches) {
|
||||||
$where[] = qsprintf(
|
$where[] = qsprintf(
|
||||||
$conn_r,
|
$conn_r,
|
||||||
|
@ -786,11 +757,17 @@ final class DifferentialRevisionQuery {
|
||||||
/**
|
/**
|
||||||
* @task internal
|
* @task internal
|
||||||
*/
|
*/
|
||||||
private function buildOrderByClause($conn_r) {
|
private function buildOrderByClause($conn_r, $is_global = false) {
|
||||||
switch ($this->order) {
|
switch ($this->order) {
|
||||||
case self::ORDER_MODIFIED:
|
case self::ORDER_MODIFIED:
|
||||||
|
if ($is_global) {
|
||||||
|
return 'ORDER BY dateModified DESC';
|
||||||
|
}
|
||||||
return 'ORDER BY r.dateModified DESC';
|
return 'ORDER BY r.dateModified DESC';
|
||||||
case self::ORDER_CREATED:
|
case self::ORDER_CREATED:
|
||||||
|
if ($is_global) {
|
||||||
|
return 'ORDER BY dateCreated DESC';
|
||||||
|
}
|
||||||
return 'ORDER BY r.dateCreated DESC';
|
return 'ORDER BY r.dateCreated DESC';
|
||||||
case self::ORDER_PATH_MODIFIED:
|
case self::ORDER_PATH_MODIFIED:
|
||||||
if (!$this->pathIDs) {
|
if (!$this->pathIDs) {
|
||||||
|
|
Loading…
Reference in a new issue