StreamSelectLoop: Improve memory consumption and runtime performance for timers

[clue]

While debugging some very odd memory issues in a live application, I noticed that the StreamSelectLoop does not immediately free all related memory when a timer is cancelled. Let's not call this a "memory leak", because memory was eventually freed, but this clearly caused some unexpected and significant memory growth.

In many common programs, adding timeouts to operations is a very fundamental operation. As such, it's common to end up with hundred or thousands of timers which most of the time get cancelled very frequently as a successful execution will quickly cancel outstanding timeouts.

I've used the following script to demonstrate unreasonable memory growth:

<?php use React\EventLoop\Factory; require __DIR__ . '/../vendor/autoload.php'; $loop = Factory::create(); $loop->addPeriodicTimer(0.00001, function () use ($loop){$timer = $loop->addTimer(60, function (){}); $loop->cancelTimer($timer)}); $loop->addPeriodicTimer(1.0, function () use ($loop){echo memory_get_usage() . PHP_EOL}); $loop->run(); 

Running this on the current master branch, this peaked at around 320 MB of memory on my system (YMMV). After applying this patch, this script reports a constant memory consumption of around 0.7 MB.

The original implementation internally relied on a SplPriorityQueue for fast insertions of new timers, but this class lacks access to actually remove timers. This means that they would actually stay in the queue until they were originally supposed to fire.

This was apparently done for performance reasons, so this patch includes some thorough performance tests. The result here is that adding a large number of timers at the same time shows a significant performance improvement (time php examples/92-benchmark-timers.php 20000 from 15s down to 2s). Waiting for a large number of timers to fire one after another does not show a noticeable impact (time php examples/94-benchmark-timers-delay.php 20000 both at around 2.5s).

[nawarian]

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.

[clue]

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? 👍

[nawarian]

Interesting to point out. Make sense to me 👍

[enumag]

SPL data structures are so badly written that I'm not at all surprised that their performance is not good. But I'd really like to see whether the new Data Structures would be better. In theory they should be faster than arrays since arrays have too many features to be efficient. The new Data Structures have 2 implementations. One as PHP extension and another as pure PHP composer package (useful as fallback in case you can't install the extension).

[clue]

@nawarian Thank you for the review! I've addressed all things you've pointed out and believe this is ready to be merged. 👍

@enumag While I don't agree with this assessment about SPL, I very much appreciate the discussion about DS! I think it may make sense to look into this for a future version, perhaps you or somebody else feels like filing a PR for this in the future so we can evaluate this? 👍 One of the design goals for this component is that is runs everywhere and is standalone without any external dependencies, so I'm curious how this would work out eventually.

[nawarian]

lgtm

[jsor]

👍 and thanks @nawarian for the review!

[WyriHaximus]

👍 and thanks for the review @nawarian 🖖