From f5c7dd68d2175587575d6d6fb732268f049dadb4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 21 Oct 2013 16:58:12 -0700 Subject: [PATCH] Clean up some ordering and strata edge cases in Phrequent Summary: Ref T3569. Two issues: # Since `sort()` is not stable, instantaneous events (ending on the same second they start) would sometime sort wrong and produce the wrong results. Guarantee they sort correctly. # Because events can end at any time, there are some additional special cases the algorithm didn't handle properly. Draw a bunch of ASCII art diagrams so these cases work properly. Test Plan: - No more fatal when tracking an object for the first time. - Unit tests. Reviewers: btrahan Reviewed By: btrahan CC: skyronic, aran Maniphest Tasks: T3569 Differential Revision: https://secure.phabricator.com/D7350 --- .../phrequent/storage/PhrequentTimeBlock.php | 129 ++++++++++++++++-- .../__tests__/PhrequentTimeBlockTestCase.php | 100 +++++++++++++- 2 files changed, 220 insertions(+), 9 deletions(-) diff --git a/src/applications/phrequent/storage/PhrequentTimeBlock.php b/src/applications/phrequent/storage/PhrequentTimeBlock.php index 866275ac08..60721e45ca 100644 --- a/src/applications/phrequent/storage/PhrequentTimeBlock.php +++ b/src/applications/phrequent/storage/PhrequentTimeBlock.php @@ -31,10 +31,12 @@ final class PhrequentTimeBlock extends Phobject { $timeline = array(); $timeline[] = array( + 'event' => $event, 'at' => $event->getDateStarted(), 'type' => 'start', ); $timeline[] = array( + 'event' => $event, 'at' => nonempty($event->getDateEnded(), $now), 'type' => 'end', ); @@ -46,10 +48,12 @@ final class PhrequentTimeBlock extends Phobject { foreach ($preempts as $preempt) { $same_object = ($preempt->getObjectPHID() == $base_phid); $timeline[] = array( + 'event' => $preempt, 'at' => $preempt->getDateStarted(), 'type' => $same_object ? 'start' : 'push', ); $timeline[] = array( + 'event' => $preempt, 'at' => nonempty($preempt->getDateEnded(), $now), 'type' => $same_object ? 'end' : 'pop', ); @@ -58,36 +62,108 @@ final class PhrequentTimeBlock extends Phobject { // Now, figure out how much time was actually spent working on the // object. - $timeline = isort($timeline, 'at'); + usort($timeline, array(__CLASS__, 'sortTimeline')); $stack = array(); $depth = null; + // NOTE: "Strata" track the separate layers between each event tracking + // the object we care about. Events might look like this: + // + // |xxxxxxxxxxxxxxxxx| + // |yyyyyyy| + // |xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx| + // 9AM 5PM + // + // ...where we care about event "x". When "y" is popped, that shouldn't + // pop the top stack -- we need to pop the stack a level down. Each + // event tracking "x" creates a new stratum, and we keep track of where + // timeline events are among the strata in order to keep stack depths + // straight. + + $stratum = null; + $strata = array(); + + $ranges = array(); foreach ($timeline as $timeline_event) { - switch ($timeline_event['type']) { + $id = $timeline_event['event']->getID(); + $type = $timeline_event['type']; + + switch ($type) { case 'start': $stack[] = $depth; $depth = 0; + $stratum = count($stack); + $strata[$id] = $stratum; $range_start = $timeline_event['at']; break; case 'end': - if ($depth == 0) { - $ranges[] = array($range_start, $timeline_event['at']); + if ($strata[$id] == $stratum) { + if ($depth == 0) { + $ranges[] = array($range_start, $timeline_event['at']); + $depth = array_pop($stack); + } else { + // Here, we've prematurely ended the current stratum. Merge all + // the higher strata into it. This looks like this: + // + // V + // V + // |zzzzzzzz| + // |xxxxx| + // |yyyyyyyyyyyyy| + // |xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx| + + $depth = array_pop($stack) + $depth; + } + } else { + // Here, we've prematurely ended a deeper stratum. Merge higher + // stata. This looks like this: + // + // V + // V + // |aaaaaaa| + // |xxxxxxxxxxxxxxxxxxx| + // |zzzzzzzzzzzzz| + // |xxxxxxx| + // |yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy| + // |xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx| + + $extra = $stack[$strata[$id]]; + unset($stack[$strata[$id] - 1]); + $stack = array_values($stack); + $stack[$strata[$id] - 1] += $extra; } - $depth = array_pop($stack); + + // Regardless of how we got here, we need to merge down any higher + // strata. + $target = $strata[$id]; + foreach ($strata as $strata_id => $id_stratum) { + if ($id_stratum >= $target) { + $strata[$strata_id]--; + } + } + $stratum = count($stack); + + unset($strata[$id]); break; case 'push': + $strata[$id] = $stratum; if ($depth == 0) { $ranges[] = array($range_start, $timeline_event['at']); } $depth++; break; case 'pop': - $depth--; - if ($depth == 0) { - $range_start = $timeline_event['at']; + if ($strata[$id] == $stratum) { + $depth--; + if ($depth == 0) { + $range_start = $timeline_event['at']; + } + } else { + $stack[$strata[$id]]--; } + unset($strata[$id]); break; } } @@ -152,4 +228,41 @@ final class PhrequentTimeBlock extends Phobject { return $result; } + + /** + * Sort events in timeline order. Notably, for events which occur on the same + * second, we want to process end events after start events. + */ + public static function sortTimeline(array $u, array $v) { + // If these events occur at different times, ordering is obvious. + if ($u['at'] != $v['at']) { + return ($u['at'] < $v['at']) ? -1 : 1; + } + + $u_end = ($u['type'] == 'end' || $u['type'] == 'pop'); + $v_end = ($v['type'] == 'end' || $v['type'] == 'pop'); + + $u_id = $u['event']->getID(); + $v_id = $v['event']->getID(); + + if ($u_end == $v_end) { + // These are both start events or both end events. Sort them by ID. + if (!$u_end) { + return ($u_id < $v_id) ? -1 : 1; + } else { + return ($u_id < $v_id) ? 1 : -1; + } + } else { + // Sort them (start, end) if they're the same event, and (end, start) + // otherwise. + if ($u_id == $v_id) { + return $v_end ? -1 : 1; + } else { + return $v_end ? 1 : -1; + } + } + + return 0; + } + } diff --git a/src/applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php b/src/applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php index 0c110c36d5..6bb09b5497 100644 --- a/src/applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php +++ b/src/applications/phrequent/storage/__tests__/PhrequentTimeBlockTestCase.php @@ -99,7 +99,6 @@ final class PhrequentTimeBlockTestCase extends PhabricatorTestCase { ), $ranges); - $event = $this->newEvent('T2', 100, 300); $event->attachPreemptingEvents( array( @@ -118,8 +117,107 @@ final class PhrequentTimeBlockTestCase extends PhabricatorTestCase { $ranges); } + public function testTimelineSort() { + $e1 = $this->newEvent('X1', 1, 1)->setID(1); + + $in = array( + array( + 'event' => $e1, + 'at' => 1, + 'type' => 'start', + ), + array( + 'event' => $e1, + 'at' => 1, + 'type' => 'end', + ), + ); + + usort($in, array('PhrequentTimeBlock', 'sortTimeline')); + + $this->assertEqual( + array( + 'start', + 'end', + ), + ipull($in, 'type')); + } + + public function testInstantaneousEvent() { + + $event = $this->newEvent('T1', 8, 8); + $event->attachPreemptingEvents(array()); + + $block = new PhrequentTimeBlock(array($event)); + + $ranges = $block->getObjectTimeRanges(1800); + $this->assertEqual( + array( + 'T1' => array( + array(8, 8), + ), + ), + $ranges); + } + + public function testPopAcrossStrata() { + + $event = $this->newEvent('T1', 1, 1000); + $event->attachPreemptingEvents( + array( + $this->newEvent('T2', 100, 300), + $this->newEvent('T1', 200, 400), + $this->newEvent('T3', 250, 275), + )); + + $block = new PhrequentTimeBlock(array($event)); + + $ranges = $block->getObjectTimeRanges(1000); + + $this->assertEqual( + array( + 'T1' => array( + array(1, 100), + array(200, 250), + array(275, 1000), + ), + ), + $ranges); + } + + public function testEndDeeperStratum() { + $event = $this->newEvent('T1', 1, 1000); + $event->attachPreemptingEvents( + array( + $this->newEvent('T2', 100, 900), + $this->newEvent('T1', 200, 400), + $this->newEvent('T3', 300, 800), + $this->newEvent('T1', 350, 600), + $this->newEvent('T4', 380, 390), + )); + + $block = new PhrequentTimeBlock(array($event)); + + $ranges = $block->getObjectTimeRanges(1000); + + $this->assertEqual( + array( + 'T1' => array( + array(1, 100), + array(200, 300), + array(350, 380), + array(390, 600), + array(900, 1000), + ), + ), + $ranges); + } + private function newEvent($object_phid, $start_time, $end_time) { + static $id = 0; + return id(new PhrequentUserTime()) + ->setID(++$id) ->setObjectPHID($object_phid) ->setDateStarted($start_time) ->setDateEnded($end_time);