Skip to content
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.

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

Improve memory consumption by cleaning up unneeded references #32

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

clue
Copy link
Member

@clue clue commented Apr 22, 2018

While debugging some very odd memory issues in a live application, I noticed that this component shows some unexpected memory consumption and memory would not immediately be freed as expected. Let's not call this a "memory leak", because memory was eventually freed, but this clearly caused some unexpected and significant memory growth.

One of the core issues has been located and addressed via reactphp/event-loop#164, but even with that patch applied the resolve() method behaved a bit unexpected.

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.001, function () use ($loop) {
    $promise = \React\Promise\Timer\resolve(60.0, $loop);
    $promise->cancel();
});

$loop->addPeriodicTimer(1.0, function () {
    echo memory_get_usage() . PHP_EOL;
});

$loop->run();

Initially this peaked at around 320 MB on my system. After applying the referenced patch, this went down significantly and fluctuated somewhere between 1 MB and 12 MB. After applying this patch, this script reports a constant memory consumption of around 0.7 MB.

This implementation includes some of the ideas discussed in reactphp/promise#46 and reactphp/socket#113. Eventually, we should look into providing a way to address this within our promise implementation.

My vote would to be get this in here now as it addresses a relevant memory issue and eventually address this in the upstream component (at which point this changeset also does no harm). :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants