From 431c57688e0d730e2f74a5574f4811db374466a2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 20 Jun 2011 16:32:44 -0700 Subject: [PATCH] Improve performance of project list view Summary: D477 added functionality to the project list view but had a couple of performance issues that I missed in review, because it took the query count for the page from around 3 to as many as 300, including up to 100 heavyweight search index queries. This fixes the two simple N+1 query problems. This general pattern of data access often occurs: COUNTEREXAMPLE $cats = load_cats(); foreach ($cats as $cat) { $cats_hats = load_hats_for_cat($cat); // ... } But this issues "N+1" queries, i.e. if you load 100 cats you issue 101 queries. It is faster to group the queries instead: $cats = load_cats(); $hats = load_all_hats_for_these_cats($cats); foreach ($cats as $cat) { $cats_hats = $hats[$cat->getID()]; } MySQL can execute one query which returns all the results much faster than 100 queries which return one result, especially if the database is not local (i.e., over the network). However, this doesn't save a ton of time. The bigger issue is that I didn't have the right keys on the relationship tables in the search engine. This adds them, and reduces the search engine lookup cost from 25-80ms (for secure.phabricator.com) down to 1-3ms. I still probably want to get this out of the loop at some point but it's okay for now and the page loads in a few ms rather than taking more than a second. Test Plan: Used "services" tab, "xhprof" and "EXPLAIN" to analyze page performance. I measured these changes: - Query count: 1 + (3 * N projects) -> 3 + (N projects) (e.g., 301 -> 103) - Total time spent querying, ignoring search indexes: 40ms (local.aprhont.com) -> 20ms (local.aphront.com) - Cost for search index query: 25-80ms (secure.phabricator.com) -> 1-3ms Reviewed By: cadamo Reviewers: cadamo, aran, jungejason, tuomaspelkonen CC: aran, cadamo, epriestley Differential Revision: 485 --- .../sql/patches/048.relationshipkeys.sql | 2 ++ .../list/PhabricatorProjectListController.php | 22 ++++++++++++++++--- .../project/controller/list/__init__.php | 2 ++ .../PhabricatorProjectProfileController.php | 2 +- .../PhabricatorProjectAffiliation.php | 13 +++++++++++ .../project/storage/affiliation/__init__.php | 2 ++ .../storage/project/PhabricatorProject.php | 9 ++++---- 7 files changed, 43 insertions(+), 9 deletions(-) create mode 100644 resources/sql/patches/048.relationshipkeys.sql diff --git a/resources/sql/patches/048.relationshipkeys.sql b/resources/sql/patches/048.relationshipkeys.sql new file mode 100644 index 0000000000..85cf1bd869 --- /dev/null +++ b/resources/sql/patches/048.relationshipkeys.sql @@ -0,0 +1,2 @@ +ALTER TABLE search_documentrelationship add key (relatedPHID, relation); +ALTER TABLE search_documentrelationship add key (relation, relatedPHID); diff --git a/src/applications/project/controller/list/PhabricatorProjectListController.php b/src/applications/project/controller/list/PhabricatorProjectListController.php index 0243bae9cf..852e5868c7 100644 --- a/src/applications/project/controller/list/PhabricatorProjectListController.php +++ b/src/applications/project/controller/list/PhabricatorProjectListController.php @@ -23,6 +23,21 @@ class PhabricatorProjectListController $projects = id(new PhabricatorProject())->loadAllWhere( '1 = 1 ORDER BY id DESC limit 100'); + $project_phids = mpull($projects, 'getPHID'); + + $profiles = array(); + if ($projects) { + $profiles = id(new PhabricatorProjectProfile())->loadAllWhere( + 'projectPHID in (%Ls)', + $project_phids); + $profiles = mpull($profiles, null, 'getProjectPHID'); + } + + $affil_groups = array(); + if ($projects) { + $affil_groups = PhabricatorProjectAffiliation::loadAllForProjectPHIDs( + $project_phids); + } $author_phids = mpull($projects, 'getAuthorPHID'); $handles = id(new PhabricatorObjectHandleData($author_phids)) @@ -30,12 +45,15 @@ class PhabricatorProjectListController $rows = array(); foreach ($projects as $project) { + $profile = $profiles[$project->getPHID()]; + $affiliations = $affil_groups[$project->getPHID()]; + $documents = new PhabricatorProjectTransactionSearch($project->getPHID()); // search all open documents by default $documents->setSearchOptions(); $documents = $documents->executeSearch(); - $documents_types = igroup($documents,'documentType'); + $documents_types = igroup($documents, 'documentType'); $tasks = idx( $documents_types, PhabricatorPHIDConstants::PHID_TYPE_TASK); @@ -48,8 +66,6 @@ class PhabricatorProjectListController PhabricatorPHIDConstants::PHID_TYPE_DREV); $revisions_amount = count($revisions); - $profile = $project->getProfile(); - $affiliations = $project->loadAffiliations(); $population = count($affiliations); $status = PhabricatorProjectStatus::getNameForStatus( diff --git a/src/applications/project/controller/list/__init__.php b/src/applications/project/controller/list/__init__.php index 72b8bf175b..c7ff3c35e3 100644 --- a/src/applications/project/controller/list/__init__.php +++ b/src/applications/project/controller/list/__init__.php @@ -10,6 +10,8 @@ phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'applications/project/constants/status'); phutil_require_module('phabricator', 'applications/project/controller/base'); +phutil_require_module('phabricator', 'applications/project/storage/affiliation'); +phutil_require_module('phabricator', 'applications/project/storage/profile'); phutil_require_module('phabricator', 'applications/project/storage/project'); phutil_require_module('phabricator', 'applications/project/transactions/search'); phutil_require_module('phabricator', 'view/control/table'); diff --git a/src/applications/project/controller/profile/PhabricatorProjectProfileController.php b/src/applications/project/controller/profile/PhabricatorProjectProfileController.php index ace47f9f27..e2e5ed65ac 100644 --- a/src/applications/project/controller/profile/PhabricatorProjectProfileController.php +++ b/src/applications/project/controller/profile/PhabricatorProjectProfileController.php @@ -36,7 +36,7 @@ class PhabricatorProjectProfileController if (!$project) { return new Aphront404Response(); } - $profile = $project->getProfile(); + $profile = $project->loadProfile(); if (!$profile) { $profile = new PhabricatorProjectProfile(); } diff --git a/src/applications/project/storage/affiliation/PhabricatorProjectAffiliation.php b/src/applications/project/storage/affiliation/PhabricatorProjectAffiliation.php index b53d073ccf..23c73254a8 100644 --- a/src/applications/project/storage/affiliation/PhabricatorProjectAffiliation.php +++ b/src/applications/project/storage/affiliation/PhabricatorProjectAffiliation.php @@ -23,4 +23,17 @@ class PhabricatorProjectAffiliation extends PhabricatorProjectDAO { protected $role; protected $status; + public static function loadAllForProjectPHIDs($phids) { + if (!$phids) { + return array(); + } + $default = array_fill_keys($phids, array()); + + $affiliations = id(new PhabricatorProjectAffiliation())->loadAllWhere( + 'projectPHID IN (%Ls) ORDER BY IF(status = "former", 1, 0), dateCreated', + $phids); + + return mgroup($affiliations, 'getProjectPHID') + $default; + } + } diff --git a/src/applications/project/storage/affiliation/__init__.php b/src/applications/project/storage/affiliation/__init__.php index f6cf277138..0c83cc12fb 100644 --- a/src/applications/project/storage/affiliation/__init__.php +++ b/src/applications/project/storage/affiliation/__init__.php @@ -8,5 +8,7 @@ phutil_require_module('phabricator', 'applications/project/storage/base'); +phutil_require_module('phutil', 'utils'); + phutil_require_source('PhabricatorProjectAffiliation.php'); diff --git a/src/applications/project/storage/project/PhabricatorProject.php b/src/applications/project/storage/project/PhabricatorProject.php index b8cd85ec28..d9da01e716 100644 --- a/src/applications/project/storage/project/PhabricatorProject.php +++ b/src/applications/project/storage/project/PhabricatorProject.php @@ -34,7 +34,7 @@ class PhabricatorProject extends PhabricatorProjectDAO { PhabricatorPHIDConstants::PHID_TYPE_PROJ); } - public function getProfile() { + public function loadProfile() { $profile = id(new PhabricatorProjectProfile())->loadOneWhere( 'projectPHID = %s', $this->getPHID()); @@ -42,9 +42,8 @@ class PhabricatorProject extends PhabricatorProjectDAO { } public function loadAffiliations() { - $affiliations = id(new PhabricatorProjectAffiliation())->loadAllWhere( - 'projectPHID = %s ORDER BY IF(status = "former", 1, 0), dateCreated', - $this->getPHID()); - return $affiliations; + $affils = PhabricatorProjectAffiliation::loadAllForProjectPHIDs( + array($this->getPHID())); + return $affils[$this->getPHID()]; } }