revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-26 15:30:58 +01:00
Code Issues Releases Wiki Activity

Disperse task subpriorities in blocks

Browse source 
Summary:
Ref T7664. The current algorithm for moving task subpriorities can end up stuck in a real sticky swamp in some unusual situations.

Instead, use an algorithm which works like this:

  - When we notice two tasks are too close together, look at the area around those tasks (just a few paces).
  - If things look pretty empty, we can just spread the tasks out a little bit.
  - But, if things are still real crowded, take another look further.
  - Keep doing that until we're looking at a real nice big spot which doesn't have too many tasks in it in total, even if they're all in one place right now.
  - Then, move 'em out!

Also:

  - Just swallow our pride and do the gross `INSERT INTO ... "", "", "", "", "", "", ... ON DUPLICATE KEY UPDATE` to bulk update.
  - Fix an issue where a single move could cause two different subpriority recalculations.

Test Plan:
  - Changed `ManiphesTaskTestCase->testTaskAdjacentBlocks()` to insert 1,000 tasks with identical subpriorities, saw them spread out in 11 queries instead of >1,000.
  - Dragged tons of tasks around on workboards.
  - Ran unit tests.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7664

Differential Revision: https://secure.phabricator.com/D17959
This commit is contained in:
epriestley 2017-05-18 21:50:27 -07:00
parent c6a7bcfe89
commit 1644b45050
4 changed files with 195 additions and 88 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
src/applications/maniphest/__tests__/ManiphestTaskTestCase.php
View file

@ -125,6 +125,34 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
9,
count($subpri),
pht('Expected subpriorities to be distributed.'));
// Move task 9 to the end.
$this->moveTask($viewer, $t[9], $t[1], true);
$tasks = $this->loadTasks($viewer, $auto_base);
$this->assertEqual(
array(8, 7, 6, 5, 4, 3, 2, 1, 9),
array_keys($tasks));
// Move task 3 to the beginning.
$this->moveTask($viewer, $t[3], $t[8], false);
$tasks = $this->loadTasks($viewer, $auto_base);
$this->assertEqual(
array(3, 8, 7, 6, 5, 4, 2, 1, 9),
array_keys($tasks));
// Move task 3 to the end.
$this->moveTask($viewer, $t[3], $t[9], true);
$tasks = $this->loadTasks($viewer, $auto_base);
$this->assertEqual(
array(8, 7, 6, 5, 4, 2, 1, 9, 3),
array_keys($tasks));
// Move task 5 to before task 4 (this is its current position).
$this->moveTask($viewer, $t[5], $t[4], false);
$tasks = $this->loadTasks($viewer, $auto_base);
$this->assertEqual(
array(8, 7, 6, 5, 4, 2, 1, 9, 3),
array_keys($tasks));
}
private function newTask(PhabricatorUser $viewer, $title) {

230
src/applications/maniphest/editor/ManiphestTransactionEditor.php
View file

@ -384,8 +384,7 @@ final class ManiphestTransactionEditor
*/
public static function getAdjacentSubpriority(
ManiphestTask $dst,
$is_after,
$allow_recursion = true) {
$is_after) {
$query = id(new ManiphestTaskQuery())
->setViewer(PhabricatorUser::getOmnipotentUser())
@ -407,76 +406,25 @@ final class ManiphestTransactionEditor
// If we find an adjacent task, we average the two subpriorities and
// return the result.
if ($adjacent) {
$epsilon = 0.01;
$epsilon = 1.0;
// If the adjacent task has a subpriority that is identical or very
// close to the task we're looking at, we're going to move it and all
// tasks with the same subpriority a little farther down the subpriority
// scale.
if ($allow_recursion &&
(abs($adjacent->getSubpriority() - $base) < $epsilon)) {
$conn_w = $adjacent->establishConnection('w');
// close to the task we're looking at, we're going to spread out all
// the nearby tasks.
$min = ($adjacent->getSubpriority() - ($epsilon));
$max = ($adjacent->getSubpriority() + ($epsilon));
// Get all of the tasks with the similar subpriorities to the adjacent
// task, including the adjacent task itself.
$query = id(new ManiphestTaskQuery())
->setViewer(PhabricatorUser::getOmnipotentUser())
->withPriorities(array($adjacent->getPriority()))
->withSubpriorityBetween($min, $max);
if (!$is_after) {
$query->setOrderVector(array('-priority', '-subpriority', '-id'));
$adjacent_sub = $adjacent->getSubpriority();
if ((abs($adjacent_sub - $base) < $epsilon)) {
$base = self::disperseBlock(
$dst,
$epsilon * 2);
if ($is_after) {
$sub = $base - $epsilon;
} else {
$query->setOrderVector(array('priority', 'subpriority', 'id'));
}
$shift_all = $query->execute();
$shift_last = last($shift_all);
// Select the most extreme subpriority in the result set as the
// base value.
$shift_base = head($shift_all)->getSubpriority();
// Find the subpriority before or after the task at the end of the
// block.
list($shift_pri, $shift_sub) = self::getAdjacentSubpriority(
$shift_last,
$is_after,
$allow_recursion = false);
$delta = ($shift_sub - $shift_base);
$count = count($shift_all);
$shift = array();
$cursor = 1;
foreach ($shift_all as $shift_task) {
$shift_target = $shift_base + (($cursor / $count) * $delta);
$cursor++;
queryfx(
$conn_w,
'UPDATE %T SET subpriority = %f WHERE id = %d',
$adjacent->getTableName(),
$shift_target,
$shift_task->getID());
// If we're shifting the adjacent task, update it.
if ($shift_task->getID() == $adjacent->getID()) {
$adjacent->setSubpriority($shift_target);
}
// If we're shifting the original target task, update the base
// subpriority.
if ($shift_task->getID() == $dst->getID()) {
$base = $shift_target;
}
$sub = $base + $epsilon;
}
} else {
$sub = ($adjacent_sub + $base) / 2;
}
$sub = ($adjacent->getSubpriority() + $base) / 2;
} else {
// Otherwise, we take a step away from the target's subpriority and
// use that.
@ -490,6 +438,156 @@ final class ManiphestTransactionEditor
return array($dst->getPriority(), $sub);
}
/**
* Distribute a cluster of tasks with similar subpriorities.
*/
private static function disperseBlock(
ManiphestTask $task,
$spacing) {
$conn = $task->establishConnection('w');
// Find a block of subpriority space which is, on average, sparse enough
// to hold all the tasks that are inside it with a reasonable level of
// separation between them.
// We'll start by looking near the target task for a range of numbers
// which has more space available than tasks. For example, if the target
// task has subpriority 33 and we want to separate each task by at least 1,
// we might start by looking in the range [23, 43].
// If we find fewer than 20 tasks there, we have room to reassign them
// with the desired level of separation. We space them out, then we're
// done.
// However: if we find more than 20 tasks, we don't have enough room to
// distribute them. We'll widen our search and look in a bigger range,
// maybe [13, 53]. This range has more space, so if we find fewer than
// 40 tasks in this range we can spread them out. If we still find too
// many tasks, we keep widening the search.
$base = $task->getSubpriority();
$scale = 4.0;
while (true) {
$range = ($spacing * $scale) / 2.0;
$min = ($base - $range);
$max = ($base + $range);
$result = queryfx_one(
$conn,
'SELECT COUNT(*) N FROM %T WHERE priority = %d AND
subpriority BETWEEN %f AND %f',
$task->getTableName(),
$task->getPriority(),
$min,
$max);
$count = $result['N'];
if ($count < $scale) {
// We have found a block which we can make sparse enough, so bail and
// continue below with our selection.
break;
}
// This block had too many tasks for its size, so try again with a
// bigger block.
$scale *= 2.0;
}
$rows = queryfx_all(
$conn,
'SELECT id FROM %T WHERE priority = %d AND
subpriority BETWEEN %f AND %f
ORDER BY priority, subpriority, id',
$task->getTableName(),
$task->getPriority(),
$min,
$max);
$task_id = $task->getID();
$result = null;
// NOTE: In strict mode (which we encourage enabling) we can't structure
// this bulk update as an "INSERT ... ON DUPLICATE KEY UPDATE" unless we
// provide default values for ALL of the columns that don't have defaults.
// This is gross, but we may be moving enough rows that individual
// queries are unreasonably slow. An alternate construction which might
// be worth evaluating is to use "CASE". Another approach is to disable
// strict mode for this query.
$extra_columns = array(
'phid' => '""',
'authorPHID' => '""',
'status' => '""',
'priority' => 0,
'title' => '""',
'originalTitle' => '""',
'description' => '""',
'dateCreated' => 0,
'dateModified' => 0,
'mailKey' => '""',
'viewPolicy' => '""',
'editPolicy' => '""',
'ownerOrdering' => '""',
'spacePHID' => '""',
'bridgedObjectPHID' => '""',
'properties' => '""',
'points' => 0,
'subtype' => '""',
);
$defaults = implode(', ', $extra_columns);
$sql = array();
$offset = 0;
// Often, we'll have more room than we need in the range. Distribute the
// tasks evenly over the whole range so that we're less likely to end up
// with tasks spaced exactly the minimum distance apart, which may
// get shifted again later. We have one fewer space to distribute than we
// have tasks.
$divisor = (double)(count($rows) - 1.0);
if ($divisor > 0) {
$available_distance = (($max - $min) / $divisor);
} else {
$available_distance = 0.0;
}
foreach ($rows as $row) {
$subpriority = $min + ($offset * $available_distance);
// If this is the task that we're spreading out relative to, keep track
// of where it is ending up so we can return the new subpriority.
$id = $row['id'];
if ($id == $task_id) {
$result = $subpriority;
}
$sql[] = qsprintf(
$conn,
'(%d, %Q, %f)',
$id,
$defaults,
$subpriority);
$offset++;
}
foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) {
queryfx(
$conn,
'INSERT INTO %T (id, %Q, subpriority) VALUES %Q
ON DUPLICATE KEY UPDATE subpriority = VALUES(subpriority)',
$task->getTableName(),
implode(', ', array_keys($extra_columns)),
$chunk);
}
return $result;
}
protected function validateAllTransactions(
PhabricatorLiskDAO $object,
array $xactions) {

22
src/applications/maniphest/query/ManiphestTaskQuery.php
View file

@ -17,8 +17,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
private $dateCreatedBefore;
private $dateModifiedAfter;
private $dateModifiedBefore;
private $subpriorityMin;
private $subpriorityMax;
private $bridgedObjectPHIDs;
private $hasOpenParents;
private $hasOpenSubtasks;
@ -112,12 +110,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
return $this;
}
public function withSubpriorityBetween($min, $max) {
$this->subpriorityMin = $min;
$this->subpriorityMax = $max;
return $this;
}
public function withSubscribers(array $subscribers) {
$this->subscriberPHIDs = $subscribers;
return $this;
@ -408,20 +400,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
$this->subpriorities);
}
if ($this->subpriorityMin !== null) {
$where[] = qsprintf(
$conn,
'task.subpriority >= %f',
$this->subpriorityMin);
}
if ($this->subpriorityMax !== null) {
$where[] = qsprintf(
$conn,
'task.subpriority <= %f',
$this->subpriorityMax);
}
if ($this->bridgedObjectPHIDs !== null) {
$where[] = qsprintf(
$conn,

3
src/applications/project/controller/PhabricatorProjectMoveController.php
View file

@ -148,6 +148,9 @@ final class PhabricatorProjectMoveController
list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority(
$task,
$is_after);
// If we find a priority on the first try, don't keep going.
break;
}
$xactions = array();