From 4469ef8f3054d48309f520c16fe72ecd65cecaf6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 26 Jun 2011 18:50:17 -0700 Subject: [PATCH] Allow Maniphest to scale to a massive size Summary: Maniphest is missing some keys and some query strategy which will make it cumbersome to manage more than a few tens of thousands of tasks. Test Plan: Handily manipulated 100k-scale task groups. Maniphest takes about 250ms to select and render pages of 1,000 tasks and has no problem paging and filtering them, etc. We should be good to scale to multiple millions of tasks with these changes. Reviewed By: gc3 Reviewers: fratrik, jungejason, aran, tuomaspelkonen, gc3 Commenters: jungejason CC: anjali, aran, epriestley, gc3, jungejason Differential Revision: 534 --- resources/sql/patches/050.taskdenormal.sql | 20 ++++ src/__celerity_resource_map__.php | 72 +++++++------- .../tasklist/ManiphestTaskListController.php | 98 ++++++++++++++++--- .../controller/tasklist/__init__.php | 2 + .../ManiphestTransactionEditor.php | 7 ++ .../maniphest/storage/task/ManiphestTask.php | 2 + webroot/rsrc/css/aphront/list-filter-view.css | 2 +- .../application/maniphest/task-summary.css | 9 +- 8 files changed, 162 insertions(+), 50 deletions(-) create mode 100644 resources/sql/patches/050.taskdenormal.sql diff --git a/resources/sql/patches/050.taskdenormal.sql b/resources/sql/patches/050.taskdenormal.sql new file mode 100644 index 0000000000..640aaf22cf --- /dev/null +++ b/resources/sql/patches/050.taskdenormal.sql @@ -0,0 +1,20 @@ +ALTER TABLE phabricator_maniphest.maniphest_task + ADD ownerOrdering varchar(64); + +ALTER TABLE phabricator_maniphest.maniphest_task + ADD UNIQUE KEY (phid); + +ALTER TABLE phabricator_maniphest.maniphest_task + ADD KEY (priority, status); + +ALTER TABLE phabricator_maniphest.maniphest_task + ADD KEY (status); + +ALTER TABLE phabricator_maniphest.maniphest_task + ADD KEY (ownerPHID, status); + +ALTER TABLE phabricator_maniphest.maniphest_task + ADD KEY (authorPHID, status); + +ALTER TABLE phabricator_maniphest.maniphest_task + ADD KEY (ownerOrdering); diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 3903571d0d..84a976d8a3 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -72,7 +72,7 @@ celerity_register_resource_map(array( ), 'aphront-list-filter-view-css' => array( - 'uri' => '/res/e6cff171/rsrc/css/aphront/list-filter-view.css', + 'uri' => '/res/0f5ddaba/rsrc/css/aphront/list-filter-view.css', 'type' => 'css', 'requires' => array( @@ -893,7 +893,7 @@ celerity_register_resource_map(array( ), 'maniphest-task-summary-css' => array( - 'uri' => '/res/0bacdd7f/rsrc/css/application/maniphest/task-summary.css', + 'uri' => '/res/e86389c4/rsrc/css/application/maniphest/task-summary.css', 'type' => 'css', 'requires' => array( @@ -1132,7 +1132,24 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/2892314d/typeahead.pkg.js', 'type' => 'js', ), - 'cfafcff7' => + 55967526 => + array ( + 'name' => 'differential.pkg.css', + 'symbols' => + array ( + 0 => 'differential-core-view-css', + 1 => 'differential-changeset-view-css', + 2 => 'differential-revision-detail-css', + 3 => 'differential-revision-history-css', + 4 => 'differential-table-of-contents-css', + 5 => 'differential-revision-comment-css', + 6 => 'differential-revision-add-comment-css', + 7 => 'differential-revision-comment-list-css', + ), + 'uri' => '/res/pkg/55967526/differential.pkg.css', + 'type' => 'css', + ), + '8e9024dc' => array ( 'name' => 'core.pkg.css', 'symbols' => @@ -1153,7 +1170,7 @@ celerity_register_resource_map(array( 13 => 'phabricator-remarkup-css', 14 => 'syntax-highlighting-css', ), - 'uri' => '/res/pkg/cfafcff7/core.pkg.css', + 'uri' => '/res/pkg/8e9024dc/core.pkg.css', 'type' => 'css', ), 'da416e1c' => @@ -1205,35 +1222,18 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/f1d27e2a/workflow.pkg.js', 'type' => 'js', ), - 55967526 => - array ( - 'name' => 'differential.pkg.css', - 'symbols' => - array ( - 0 => 'differential-core-view-css', - 1 => 'differential-changeset-view-css', - 2 => 'differential-revision-detail-css', - 3 => 'differential-revision-history-css', - 4 => 'differential-table-of-contents-css', - 5 => 'differential-revision-comment-css', - 6 => 'differential-revision-add-comment-css', - 7 => 'differential-revision-comment-list-css', - ), - 'uri' => '/res/pkg/55967526/differential.pkg.css', - 'type' => 'css', - ), ), 'reverse' => array ( - 'aphront-crumbs-view-css' => 'cfafcff7', - 'aphront-dialog-view-css' => 'cfafcff7', - 'aphront-form-view-css' => 'cfafcff7', - 'aphront-list-filter-view-css' => 'cfafcff7', - 'aphront-panel-view-css' => 'cfafcff7', - 'aphront-side-nav-view-css' => 'cfafcff7', - 'aphront-table-view-css' => 'cfafcff7', - 'aphront-tokenizer-control-css' => 'cfafcff7', - 'aphront-typeahead-control-css' => 'cfafcff7', + 'aphront-crumbs-view-css' => '8e9024dc', + 'aphront-dialog-view-css' => '8e9024dc', + 'aphront-form-view-css' => '8e9024dc', + 'aphront-list-filter-view-css' => '8e9024dc', + 'aphront-panel-view-css' => '8e9024dc', + 'aphront-side-nav-view-css' => '8e9024dc', + 'aphront-table-view-css' => '8e9024dc', + 'aphront-tokenizer-control-css' => '8e9024dc', + 'aphront-typeahead-control-css' => '8e9024dc', 'differential-changeset-view-css' => '55967526', 'differential-core-view-css' => '55967526', 'differential-revision-add-comment-css' => '55967526', @@ -1270,13 +1270,13 @@ celerity_register_resource_map(array( 'javelin-util' => 'db95a6d0', 'javelin-vector' => 'db95a6d0', 'javelin-workflow' => 'f1d27e2a', - 'phabricator-core-buttons-css' => 'cfafcff7', - 'phabricator-core-css' => 'cfafcff7', - 'phabricator-directory-css' => 'cfafcff7', + 'phabricator-core-buttons-css' => '8e9024dc', + 'phabricator-core-css' => '8e9024dc', + 'phabricator-directory-css' => '8e9024dc', 'phabricator-keyboard-shortcut' => 'f1d27e2a', 'phabricator-keyboard-shortcut-manager' => 'f1d27e2a', - 'phabricator-remarkup-css' => 'cfafcff7', - 'phabricator-standard-page-view' => 'cfafcff7', - 'syntax-highlighting-css' => 'cfafcff7', + 'phabricator-remarkup-css' => '8e9024dc', + 'phabricator-standard-page-view' => '8e9024dc', + 'syntax-highlighting-css' => '8e9024dc', ), )); diff --git a/src/applications/maniphest/controller/tasklist/ManiphestTaskListController.php b/src/applications/maniphest/controller/tasklist/ManiphestTaskListController.php index 7d008a13cd..b70dca8fe2 100644 --- a/src/applications/maniphest/controller/tasklist/ManiphestTaskListController.php +++ b/src/applications/maniphest/controller/tasklist/ManiphestTaskListController.php @@ -18,6 +18,8 @@ class ManiphestTaskListController extends ManiphestController { + const DEFAULT_PAGE_SIZE = 1000; + private $view; public function willProcessRequest(array $data) { @@ -90,12 +92,17 @@ class ManiphestTaskListController extends ManiphestController { $view_phid = nonempty($request->getStr('phid'), $user->getPHID()); - list($tasks, $handles) = $this->loadTasks( + $page = $request->getInt('page'); + $page_size = self::DEFAULT_PAGE_SIZE; + + list($tasks, $handles, $total_count) = $this->loadTasks( $view_phid, array( 'status' => $status_map, 'group' => $grouping, 'order' => $order, + 'offset' => $page, + 'limit' => $page_size, )); @@ -158,6 +165,25 @@ class ManiphestTaskListController extends ManiphestController { 'No matching tasks.'. ''); } else { + $pager = new AphrontPagerView(); + $pager->setURI($request->getRequestURI(), 'page'); + $pager->setPageSize($page_size); + $pager->setOffset($page); + $pager->setCount($total_count); + + $cur = ($pager->getOffset() + 1); + $max = min($pager->getOffset() + $page_size, $total_count); + $tot = $total_count; + + $cur = number_format($cur); + $max = number_format($max); + $tot = number_format($tot); + + $nav->appendChild( + '
'. + "Displaying tasks {$cur} - {$max} of {$tot}.". + '
'); + foreach ($tasks as $group => $list) { $task_list = new ManiphestTaskListView(); $task_list->setUser($user); @@ -171,6 +197,8 @@ class ManiphestTaskListController extends ManiphestController { ''); $nav->appendChild($task_list); } + + $nav->appendChild($pager); } return $this->buildStandardPageResponse( @@ -224,23 +252,63 @@ class ManiphestTaskListController extends ManiphestController { break; } - switch ($dict['order']) { + $order = array(); + switch ($dict['group']) { case 'priority': - $order = 'priority DESC, dateModified DESC'; + $order[] = 'priority'; break; - case 'created': - $order = 'id DESC'; + case 'owner': + $order[] = 'ownerOrdering'; break; - default: - $order = 'dateModified DESC'; + case 'status': + $order[] = 'status'; break; } - $sql = "({$status_clause}) AND ({$extra_clause}) ORDER BY {$order}"; + switch ($dict['order']) { + case 'priority': + $order[] = 'priority'; + $order[] = 'dateModified'; + break; + case 'created': + $order[] = 'id'; + break; + default: + $order[] = 'dateModified'; + break; + } - array_unshift($argv, $sql); + $order = array_unique($order); - $data = call_user_func_array(array($task, 'loadAllWhere'), $argv); + foreach ($order as $k => $column) { + switch ($column) { + case 'ownerOrdering': + $order[$k] = "{$column} ASC"; + break; + default: + $order[$k] = "{$column} DESC"; + break; + } + } + + $order = implode(', ', $order); + + $offset = (int)idx($dict, 'offset', 0); + $limit = (int)idx($dict, 'limit', self::DEFAULT_PAGE_SIZE); + + $sql = "SELECT SQL_CALC_FOUND_ROWS * FROM %T WHERE ". + "({$status_clause}) AND ({$extra_clause}) ORDER BY {$order} ". + "LIMIT {$offset}, {$limit}"; + + array_unshift($argv, $task->getTableName()); + + $conn = $task->establishConnection('r'); + $data = vqueryfx_all($conn, $sql, $argv); + + $total_row_count = queryfx_one($conn, 'SELECT FOUND_ROWS() N'); + $total_row_count = $total_row_count['N']; + + $data = $task->loadAllFromArray($data); $handle_phids = mpull($data, 'getOwnerPHID'); $handle_phids[] = $view_phid; @@ -252,9 +320,15 @@ class ManiphestTaskListController extends ManiphestController { $data = mgroup($data, 'getPriority'); krsort($data); + // If we have invalid priorities, they'll all map to "???". Merge + // arrays to prevent them from overwriting each other. + $out = array(); foreach ($data as $pri => $tasks) { - $out[ManiphestTaskPriority::getTaskPriorityName($pri)] = $tasks; + $out[ManiphestTaskPriority::getTaskPriorityName($pri)][] = $tasks; + } + foreach ($out as $pri => $tasks) { + $out[$pri] = array_mergev($tasks); } $data = $out; @@ -297,7 +371,7 @@ class ManiphestTaskListController extends ManiphestController { break; } - return array($data, $handles); + return array($data, $handles, $total_row_count); } public function renderStatusLinks() { diff --git a/src/applications/maniphest/controller/tasklist/__init__.php b/src/applications/maniphest/controller/tasklist/__init__.php index 343f803625..ffd77b6ee2 100644 --- a/src/applications/maniphest/controller/tasklist/__init__.php +++ b/src/applications/maniphest/controller/tasklist/__init__.php @@ -14,6 +14,8 @@ phutil_require_module('phabricator', 'applications/maniphest/storage/task'); phutil_require_module('phabricator', 'applications/maniphest/view/tasklist'); phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'infrastructure/celerity/api'); +phutil_require_module('phabricator', 'storage/queryfx'); +phutil_require_module('phabricator', 'view/control/pager'); phutil_require_module('phabricator', 'view/form/base'); phutil_require_module('phabricator', 'view/form/control/togglebuttons'); phutil_require_module('phabricator', 'view/form/control/tokenizer'); diff --git a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php index a2828a8b98..37acc5d26d 100644 --- a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php @@ -89,6 +89,13 @@ class ManiphestTransactionEditor { $task->setStatus($new); break; case ManiphestTransactionType::TYPE_OWNER: + if ($new) { + $handles = id(new PhabricatorObjectHandleData(array($new))) + ->loadHandles(); + $task->setOwnerOrdering($handles[$new]->getName()); + } else { + $task->setOwnerOrdering(null); + } $task->setOwnerPHID($new); break; case ManiphestTransactionType::TYPE_CCS: diff --git a/src/applications/maniphest/storage/task/ManiphestTask.php b/src/applications/maniphest/storage/task/ManiphestTask.php index 42959023f4..717e4e0692 100644 --- a/src/applications/maniphest/storage/task/ManiphestTask.php +++ b/src/applications/maniphest/storage/task/ManiphestTask.php @@ -34,6 +34,8 @@ class ManiphestTask extends ManiphestDAO { protected $attached = array(); protected $projectPHIDs = array(); + protected $ownerOrdering; + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, diff --git a/webroot/rsrc/css/aphront/list-filter-view.css b/webroot/rsrc/css/aphront/list-filter-view.css index 09b59baabf..7641debfa2 100644 --- a/webroot/rsrc/css/aphront/list-filter-view.css +++ b/webroot/rsrc/css/aphront/list-filter-view.css @@ -6,7 +6,7 @@ background: #f6f6f6; border-bottom: 1px solid #bbbbbb; width: 100%; - margin-bottom: 2em; + margin-bottom: 1em; } .aphront-list-filter-view-buttons { diff --git a/webroot/rsrc/css/application/maniphest/task-summary.css b/webroot/rsrc/css/application/maniphest/task-summary.css index f9072a7788..3a9277960d 100644 --- a/webroot/rsrc/css/application/maniphest/task-summary.css +++ b/webroot/rsrc/css/application/maniphest/task-summary.css @@ -82,6 +82,13 @@ .maniphest-task-group-header { font-size: 18px; - margin: 1.5em 14px 0; + margin: 1em 14px 0; border-bottom: 1px solid #dddddd; } + +.maniphest-total-result-count { + text-align: right; + padding-right: 2em; + font-size: 11px; + color: #666666; +}