Skip to content

StreamSelectLoop: Improve memory consumption and runtime performance for timers#164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
WyriHaximus merged 2 commits into from
Apr 23, 2018
Merged

StreamSelectLoop: Improve memory consumption and runtime performance for timers #164

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efd037c
Use sorted array to store timers and remove cancelled ones
clue Apr 20, 2018
be70d8c
Improve performance by sorting timers only on demand
clue Apr 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 34 additions & 43 deletions src/Timer/Timers.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,8 +3,6 @@
namespace React\EventLoop\Timer;

use React\EventLoop\TimerInterface;
use SplObjectStorage;
use SplPriorityQueue;

/**
* A scheduler implementation that can hold multiple timer instances
Expand All@@ -17,14 +15,9 @@
final class Timers
{
private $time;
private $timers;
private $scheduler;

public function __construct()
{
$this->timers = new SplObjectStorage();
$this->scheduler = new SplPriorityQueue();
}
private $timers = array();
private $schedule = array();
private $sorted = true;

public function updateTime()
{
Expand All@@ -38,36 +31,32 @@ public function getTime()

public function add(TimerInterface $timer)
{
$interval = $timer->getInterval();
$scheduledAt = $interval + microtime(true);

$this->timers->attach($timer, $scheduledAt);
$this->scheduler->insert($timer, -$scheduledAt);
$id = spl_object_hash($timer);
$this->timers[$id] = $timer;
$this->schedule[$id] = $timer->getInterval() + microtime(true);
$this->sorted = false;
}

public function contains(TimerInterface $timer)
{
return $this->timers->contains($timer);
return isset($this->timers[spl_oject_hash($timer)]);
}

public function cancel(TimerInterface $timer)
{
$this->timers->detach($timer);
$id = spl_object_hash($timer);
unset($this->timers[$id], $this->schedule[$id]);
}

public function getFirst()
{
while ($this->scheduler->count()){
$timer = $this->scheduler->top();

if ($this->timers->contains($timer)){
return $this->timers[$timer];
}

$this->scheduler->extract();
// ensure timers are sorted to simply accessing next (first) one
if (!$this->sorted){
$this->sorted = true;
asort($this->schedule);
}

return null;
return reset($this->schedule);
}

public function isEmpty()
Expand All@@ -77,32 +66,34 @@ public function isEmpty()

public function tick()
{
$time = $this->updateTime();
$timers = $this->timers;
$scheduler = $this->scheduler;

while (!$scheduler->isEmpty()){
$timer = $scheduler->top();
// ensure timers are sorted so we can execute in order
if (!$this->sorted){
Copy link
Contributor

@nawariannawarianApr 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have the exact same logic at line 54. You could create a private method ensureScheduleIsSorted() or something similar in order to do this checking and perform the sorting operation.

Copy link
MemberAuthor

@clueclueApr 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point! This is the very core of the event loop, so this is the critical code path and I would rather avoid any additional function calls. What do you think about this? 👍

Copy link
Contributor

@nawariannawarianApr 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting to point out. Make sense to me 👍

$this->sorted = true;
asort($this->schedule);
}

if (!isset($timers[$timer])){
$scheduler->extract();
$timers->detach($timer);
$time = $this->updateTime();

continue;
foreach ($this->schedule as $id => $scheduled){
// schedule is ordered, so loop until first timer that is not scheduled for execution now
if ($scheduled >= $time){
break;
}

if ($timers[$timer] >= $time){
break;
// skip any timers that are removed while we process the current schedule
if (!isset($this->schedule[$id]) || $this->schedule[$id] !== $scheduled){
continue;
}

$scheduler->extract();
$timer = $this->timers[$id];
call_user_func($timer->getCallback(), $timer);

if ($timer->isPeriodic() && isset($timers[$timer])){
$timers[$timer] = $scheduledAt = $timer->getInterval() + $time;
$scheduler->insert($timer, -$scheduledAt);
// re-schedule if this is a periodic timer and it has not been cancelled explicitly already
if ($timer->isPeriodic() && isset($this->timers[$id])){
$this->schedule[$id] = $timer->getInterval() + $time;
$this->sorted = false;
} else{
$timers->detach($timer);
unset($this->timers[$id], $this->schedule[$id]);
}
}
}
Expand Down