Improve task subpriority movement algorithm for homogenous blocks

Summary:
Fixes T7664. When there are a large number of tasks (400+) with the same subpriority (which can happen if the subpriority features are rarely used), it may take more than 30 seconds to rebalance them.

Make the algorithm more aggressive about rebalancing homogenous blocks of tasks.

This may need to get even fancier, but I'd guess it can process blocks 1-2 orders of magnitude larger, which should be ~all installs.

(If someone still hits issues with this, I'll make it fancier.)

Once a block is rebalanced, it doesn't need to be rebalanced again (at least, not as a whole block) so we basically just need to get over the initial hurdle here and then we're good.

In the worst case, we can provide `bin/maniphest rebalance` or similar and do the rebalance step offline.

And, in any case, we have more test coverage here now.

Test Plan:
  - Existing tests.
  - New tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7664

Differential Revision: https://secure.phabricator.com/D12166

src/applications/maniphest/__tests__/ManiphestTaskTestCase.php

@@ -15,9 +15,11 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
     $t2 = $this->newTask($viewer, 'Task 2');
     $t3 = $this->newTask($viewer, 'Task 3');
 
+    $auto_base = min(mpull(array($t1, $t2, $t3), 'getID'));
+
 
     // Default order should be reverse creation.
-    $tasks = $this->loadTasks($viewer);
+    $tasks = $this->loadTasks($viewer, $auto_base);
     $t1 = $tasks[1];
     $t2 = $tasks[2];
     $t3 = $tasks[3];
@@ -26,7 +28,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
 
     // Move T3 to the middle.
     $this->moveTask($viewer, $t3, $t2, true);
-    $tasks = $this->loadTasks($viewer);
+    $tasks = $this->loadTasks($viewer, $auto_base);
     $t1 = $tasks[1];
     $t2 = $tasks[2];
     $t3 = $tasks[3];
@@ -35,7 +37,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
 
     // Move T3 to the end.
     $this->moveTask($viewer, $t3, $t1, true);
-    $tasks = $this->loadTasks($viewer);
+    $tasks = $this->loadTasks($viewer, $auto_base);
     $t1 = $tasks[1];
     $t2 = $tasks[2];
     $t3 = $tasks[3];
@@ -44,7 +46,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
 
     // Repeat the move above, there should be no overall change in order.
     $this->moveTask($viewer, $t3, $t1, true);
-    $tasks = $this->loadTasks($viewer);
+    $tasks = $this->loadTasks($viewer, $auto_base);
     $t1 = $tasks[1];
     $t2 = $tasks[2];
     $t3 = $tasks[3];
@@ -53,7 +55,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
 
     // Move T3 to the first slot in the priority.
     $this->movePriority($viewer, $t3, $t3->getPriority(), false);
-    $tasks = $this->loadTasks($viewer);
+    $tasks = $this->loadTasks($viewer, $auto_base);
     $t1 = $tasks[1];
     $t2 = $tasks[2];
     $t3 = $tasks[3];
@@ -62,7 +64,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
 
     // Move T3 to the last slot in the priority.
     $this->movePriority($viewer, $t3, $t3->getPriority(), true);
-    $tasks = $this->loadTasks($viewer);
+    $tasks = $this->loadTasks($viewer, $auto_base);
     $t1 = $tasks[1];
     $t2 = $tasks[2];
     $t3 = $tasks[3];
@@ -71,7 +73,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
 
     // Move T3 before T2.
     $this->moveTask($viewer, $t3, $t2, false);
-    $tasks = $this->loadTasks($viewer);
+    $tasks = $this->loadTasks($viewer, $auto_base);
     $t1 = $tasks[1];
     $t2 = $tasks[2];
     $t3 = $tasks[3];
@@ -80,7 +82,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
 
     // Move T3 before T1.
     $this->moveTask($viewer, $t3, $t1, false);
-    $tasks = $this->loadTasks($viewer);
+    $tasks = $this->loadTasks($viewer, $auto_base);
     $t1 = $tasks[1];
     $t2 = $tasks[2];
     $t3 = $tasks[3];
@@ -88,6 +90,43 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
 
   }
 
+  public function testTaskAdjacentBlocks() {
+    $viewer = $this->generateNewTestUser();
+
+    $t = array();
+    for ($ii = 1; $ii < 10; $ii++) {
+      $t[$ii] = $this->newTask($viewer, "Task Block {$ii}");
+
+      // This makes sure this test remains meaningful if we begin assigning
+      // subpriorities when tasks are created.
+      $t[$ii]->setSubpriority(0)->save();
+    }
+
+    $auto_base = min(mpull($t, 'getID'));
+
+    $tasks = $this->loadTasks($viewer, $auto_base);
+    $this->assertEqual(
+      array(9, 8, 7, 6, 5, 4, 3, 2, 1),
+      array_keys($tasks));
+
+    $this->moveTask($viewer, $t[9], $t[8], true);
+    $tasks = $this->loadTasks($viewer, $auto_base);
+    $this->assertEqual(
+      array(8, 9, 7, 6, 5, 4, 3, 2, 1),
+      array_keys($tasks));
+
+    // When there is a large block of tasks which all have the same
+    // subpriority, they should be assigned distinct subpriorities as a
+    // side effect of having a task moved into the block.
+
+    $subpri = mpull($tasks, 'getSubpriority');
+    $unique_subpri = array_unique($subpri);
+    $this->assertEqual(
+      9,
+      count($subpri),
+      'Expected subpriorities to be distributed.');
+  }
+
   private function newTask(PhabricatorUser $viewer, $title) {
     $task = ManiphestTask::initializeNewTask($viewer);
 
@@ -103,15 +142,23 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase {
     return $task;
   }
 
-  private function loadTasks(PhabricatorUser $viewer) {
+  private function loadTasks(PhabricatorUser $viewer, $auto_base) {
     $tasks = id(new ManiphestTaskQuery())
       ->setViewer($viewer)
       ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
       ->execute();
 
-    $tasks = mpull($tasks, null, 'getID');
+    // NOTE: AUTO_INCREMENT changes survive ROLLBACK, and we can't throw them
+    // away without committing the current transaction, so we adjust the
+    // apparent task IDs as though the first one had been ID 1. This makes the
+    // tests a little easier to understand.
 
-    return $tasks;
+    $map = array();
+    foreach ($tasks as $task) {
+      $map[($task->getID() - $auto_base) + 1] = $task;
+    }
+
+    return $map;
   }
 
   private function moveTask(PhabricatorUser $viewer, $src, $dst, $is_after) {

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -679,21 +679,57 @@ final class ManiphestTransactionEditor
     // return the result.
     if ($adjacent) {
       // 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 a little
+      // close to the task we're looking at, we're going to move it and all
-      // farther down the subpriority list.
+      // tasks with the same subpriority a little farther down the subpriority
+      // scale.
       if (abs($adjacent->getSubpriority() - $base) < 0.01) {
+        $conn_w = $adjacent->establishConnection('w');
+
+        // Get all of the tasks with the same subpriority as the adjacent
+        // task, including the adjacent task itself.
+        $shift_base = $adjacent->getSubpriority();
+        $shift_all = id(new ManiphestTaskQuery())
+          ->setViewer(PhabricatorUser::getOmnipotentUser())
+          ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
+          ->withPriorities(array($adjacent->getPriority()))
+          ->withSubpriorities(array($shift_base))
+          ->setReversePaging(!$is_after)
+          ->execute();
+        $shift_last = last($shift_all);
+
+        // Find the subpriority before or after the task at the end of the
+        // block.
         list($shift_pri, $shift_sub) = self::getAdjacentSubpriority(
-          $adjacent,
+          $shift_last,
           $is_after);
 
+        $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(
-          $adjacent->establishConnection('r'),
+            $conn_w,
             'UPDATE %T SET subpriority = %f WHERE id = %d',
             $adjacent->getTableName(),
-          $shift_sub,
+            $shift_target,
-          $adjacent->getID());
+            $shift_task->getID());
 
-        $adjacent->setSubpriority($shift_sub);
+          // 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 = ($adjacent->getSubpriority() + $base) / 2;

src/applications/maniphest/query/ManiphestTaskQuery.php

@@ -37,6 +37,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
 
   private $statuses;
   private $priorities;
+  private $subpriorities;
 
   private $groupBy          = 'group-none';
   const GROUP_NONE          = 'group-none';
@@ -137,6 +138,11 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     return $this;
   }
 
+  public function withSubpriorities(array $subpriorities) {
+    $this->subpriorities = $subpriorities;
+    return $this;
+  }
+
   public function withSubscribers(array $subscribers) {
     $this->subscriberPHIDs = $subscribers;
     return $this;
@@ -506,6 +512,13 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         $this->priorities);
     }
 
+    if ($this->subpriorities) {
+      return qsprintf(
+        $conn,
+        'task.subpriority IN (%Lf)',
+        $this->subpriorities);
+    }
+
     return null;
   }
 

src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php

@@ -11,7 +11,7 @@ final class PhabricatorStorageFixtureScopeGuard {
     $this->name = $name;
 
     execx(
-      'php %s upgrade --force --namespace %s',
+      'php %s upgrade --force --no-adjust --namespace %s',
       $this->getStorageBinPath(),
       $this->name);