revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00
Code Issues Releases Wiki Activity

Add an abstraction for preventing the 1+N queries problem

Browse source 
Summary:
Our code is quite complex in areas where we prevents the 1+N queries problem explained in [[ http://www.phabricator.com/docs/phabricator/article/Performance_N+1_Query_Problem.html | a performance chapter ]].

This diff adds an abstraction for preventing this code.

Test Plan:
Run all examples mentioned in the doc-comments with logging the queries.
Generate and read docs.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2557
This commit is contained in:
vrana 2012-05-23 14:05:33 -07:00
parent 1377d349e1
commit 0446e636ea
4 changed files with 219 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/docs/developer/n_plus_one.diviner
View file

@ -63,6 +63,9 @@ are no longer paying the overhead of issuing hundreds of extra queries. This
will perform much better (although, as with all performance changes, you should will perform much better (although, as with all performance changes, you should
verify this claim by measuring it). verify this claim by measuring it).
See also @{method:LiskDAO::loadRelatives} method which provides an abstraction
to prevent this problem.
= Detecting the Problem = = Detecting the Problem =
Beyond reasoning about it while figuring out how to load the data you need, the Beyond reasoning about it while figuring out how to load the data you need, the

130
src/storage/lisk/dao/LiskDAO.php
View file

@ -138,6 +138,9 @@
* @{method:loadAllWhere} returns a list of objects, while * @{method:loadAllWhere} returns a list of objects, while
* @{method:loadOneWhere} returns a single object (or `null`). * @{method:loadOneWhere} returns a single object (or `null`).
* *
* There's also a @{method:loadRelatives} method which helps to prevent the 1+N
* queries problem.
*
* = Managing Transactions = * = Managing Transactions =
* *
* Lisk uses a transaction stack, so code does not generally need to be aware * Lisk uses a transaction stack, so code does not generally need to be aware
@ -204,6 +207,8 @@ abstract class LiskDAO {
private static $connections = array(); private static $connections = array();
private $inSet = null;
/** /**
* Build an empty object. * Build an empty object.
* *
@ -692,11 +697,136 @@ abstract class LiskDAO {
} else { } else {
$result[] = $obj->loadFromArray($row); $result[] = $obj->loadFromArray($row);
} }
if ($this->inSet) {
$this->inSet->addToSet($obj);
}
} }
return $result; return $result;
} }
/**
* This method helps to prevent the 1+N queries problem. It happens when you
* execute a query for each row in a result set. Like in this code:
*
* COUNTEREXAMPLE, name=Easy to write but expensive to execute
* $diffs = id(new DifferentialDiff())->loadAllWhere(
* 'revisionID = %d',
* $revision->getID());
* foreach ($diffs as $diff) {
* $changesets = id(new DifferentialChangeset())->loadAllWhere(
* 'diffID = %d',
* $diff->getID());
* // Do something with $changesets.
* }
*
* One can solve this problem by reading all the dependent objects at once and
* assigning them later:
*
* COUNTEREXAMPLE, name=Cheaper to execute but harder to write and maintain
* $diffs = id(new DifferentialDiff())->loadAllWhere(
* 'revisionID = %d',
* $revision->getID());
* $all_changesets = id(new DifferentialChangeset())->loadAllWhere(
* 'diffID IN (%Ld)',
* mpull($diffs, 'getID'));
* $all_changesets = mgroup($all_changesets, 'getDiffID');
* foreach ($diffs as $diff) {
* $changesets = idx($all_changesets, $diff->getID(), array());
* // Do something with $changesets.
* }
*
* The method @{method:loadRelatives} abstracts this approach which allows
* writing a code which is simple and efficient at the same time:
*
* name=Easy to write and cheap to execute
* $diffs = $revision->loadRelatives(new DifferentialDiff(), 'revisionID');
* foreach ($diffs as $diff) {
* $changesets = $diff->loadRelatives(
* new DifferentialChangeset(),
* 'diffID');
* // Do something with $changesets.
* }
*
* This will load dependent objects for all diffs in the first call of
* @{method:loadRelatives} and use this result for all following calls.
*
* The method supports working with set of sets, like in this code:
*
* $diffs = $revision->loadRelatives(new DifferentialDiff(), 'revisionID');
* foreach ($diffs as $diff) {
* $changesets = $diff->loadRelatives(
* new DifferentialChangeset(),
* 'diffID');
* foreach ($changesets as $changeset) {
* $hunks = $changeset->loadRelatives(
* new DifferentialHunk(),
* 'changesetID');
* // Do something with hunks.
* }
* }
*
* This code will execute just three queries - one to load all diffs, one to
* load all their related changesets and one to load all their related hunks.
* You can try to write an equivalent code without using this method as
* a homework.
*
* The method also supports retrieving referenced objects, for example authors
* of all diffs:
*
* foreach ($diffs as $diff) {
* $author = head($diff->loadRelatives(
* new PhabricatorUser(),
* 'phid',
* 'getAuthorPHID'));
* // Do something with author.
* }
*
* It is also possible to specify additional conditions for the `WHERE`
* clause. Similarly to @{method:loadAllWhere}, you can specify everything
* after `WHERE` (except `LIMIT`). Contrary to @{method:loadAllWhere}, it is
* allowed to pass only a constant string (`%` doesn't have a special
* meaning). This is intentional to avoid mistakes with using data from one
* row in retrieving other rows. Example of a correct usage:
*
* $status = head($author->loadRelatives(
* new PhabricatorUserStatus(),
* 'userPHID',
* 'getPHID',
* '(UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo)'));
*
* @param LiskDAO Type of objects to load.
* @param string Name of the column in target table.
* @param string Method name in this table.
* @param string Additional constraints on returned rows. It supports no
* placeholders and requires putting the WHERE part into
* parentheses. It's not possible to use LIMIT.
* @return list Objects of type $object.
*
* @task load
*/
public function loadRelatives(
LiskDAO $object,
$foreign_column,
$key_method = 'getID',
$where = '') {
if (!$this->inSet) {
id(new LiskDAOSet())->addToSet($this);
}
$relatives = $this->inSet->loadRelatives(
$object,
$foreign_column,
$key_method,
$where);
return idx($relatives, $this->$key_method(), array());
}
final public function putInSet(LiskDAOSet $set) {
$this->inSet = $set;
return $this;
}
/* -( Examining Objects )-------------------------------------------------- */ /* -( Examining Objects )-------------------------------------------------- */

85
src/storage/lisk/dao/LiskDAOSet.php Normal file
View file

@ -0,0 +1,85 @@
<?php
/*
* Copyright 2012 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/**
* You usually don't need to use this class directly as it is controlled by
* @{class:LiskDAO}. You can create it if you want to work with objects of same
* type from different sources as with one set. Let's say you want to get
* e-mails of all users involved in a revision:
*
* $users = new LiskDAOSet();
* $users->addToSet($author);
* foreach ($reviewers as $reviewer) {
* $users->addToSet($reviewer);
* }
* foreach ($ccs as $cc) {
* $users->addToSet($cc);
* }
* // Preload e-mails of all involved users and return e-mails of author.
* $author_emails = $author->loadRelatives(
* new PhabricatorUserEmail(),
* 'userPHID',
* 'getPHID');
*/
final class LiskDAOSet {
private $daos = array();
private $relatives = array();
public function addToSet(LiskDAO $dao) {
$this->daos[] = $dao;
$dao->putInSet($this);
return $this;
}
/**
* See @{method:LiskDAO::loadRelatives}.
*/
public function loadRelatives(
LiskDAO $object,
$foreign_column,
$key_method = 'getID',
$where = '') {
$relatives = &$this->relatives[
get_class($object)."-{$foreign_column}-{$key_method}-{$where}"];
if ($relatives === null) {
$ids = array();
foreach ($this->daos as $dao) {
$id = $dao->$key_method();
if ($id !== null) {
$ids[$id] = $id;
}
}
if (!$ids) {
$relatives = array();
} else {
$set = new LiskDAOSet();
$relatives = $object->putInSet($set)->loadAllWhere(
'%C IN (%Ls) %Q',
$foreign_column,
$ids,
($where != '' ? 'AND '.$where : ''));
$relatives = mgroup($relatives, 'get'.$foreign_column);
}
}
return $relatives;
}
}

1
src/storage/lisk/dao/__init__.php
View file

@ -16,4 +16,5 @@ phutil_require_module('phutil', 'utils');
phutil_require_source('LiskDAO.php'); phutil_require_source('LiskDAO.php');
phutil_require_source('LiskDAOSet.php');
phutil_require_source('LiskEphemeralObjectException.php'); phutil_require_source('LiskEphemeralObjectException.php');