Page MenuHomePhabricator

Disperse task subpriorities in blocks
ClosedPublic

Authored by epriestley on May 19 2017, 5:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 7:23 PM
Unknown Object (File)
Tue, Dec 10, 2:28 AM
Unknown Object (File)
Sat, Dec 7, 1:13 PM
Unknown Object (File)
Fri, Dec 6, 8:01 AM
Unknown Object (File)
Tue, Dec 3, 8:43 PM
Unknown Object (File)
Sat, Nov 30, 9:05 AM
Unknown Object (File)
Sat, Nov 30, 1:03 AM
Unknown Object (File)
Sat, Nov 23, 2:12 PM
Subscribers
None
Tokens
"Love" token, awarded by jboning.

Details

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.

Diff Detail

Repository
rP Phabricator
Branch
blocks1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 17135
Build 22925: Run Core Tests
Build 22924: arc lint + arc unit

Event Timeline

Often, we end up with a larger range than we need to put the minimum amount of space between tasks.

When we do, distribute the tasks evenly over the range, rather than putting them at the beginning of the range with the minimum amount of space between them. This limits the chance that we'll end up just moving tasks back and forth because we distributed tasks so they end up in exactly-maximum-density blocks.

  • Slightly more precise comment wording.
This revision is now accepted and ready to land.May 19 2017, 1:34 PM

I'm going to hold this until after release cut and bulk up the test coverage a bit, the we can kick the tires on this install for a little while before it goes out into the wild.

The fundamental "approximate a double-linked list using doubles" approach here still doesn't feel particularly great and I think probably never will, but this version of the dispersal code feels much more like a real algorithm than previous iterations did.

  • Remove withSubpriorityBetween() query stuff.
  • Add some additional tests (moving to beginning/end, moving to current position).
This revision was automatically updated to reflect the committed changes.