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 only passing resolver args to resolver and canceller if callback requires them #113

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

clue
Copy link
Member

@clue clue commented Apr 24, 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 when a pending promise 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.

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

<?php

use React\Promise\Promise;

require __DIR__ . '/vendor/autoload.php';

for ($i = 0; $i < 1000000; ++$i) {
    $time = time();
    $promise = new Promise(function () { }, function () use ($time) {
        throw new Exception();
    });
    $promise->cancel();
}

var_dump(memory_get_usage());
var_dump(gc_collect_cycles());

Initially this peaked somewhere around 15 MB on my system taking 3.4s. After applying this patch, this script reports a constant memory consumption of around 0.7 MB taking 1.8s

Empirical evidence suggests that many common use cases do not use the $reject (etc.) arguments and instead simply throw an Exception. The Promise class now uses reflection to check whether the $resolver and $canceller functions actually define any arguments and if they are not expected, doesn't actually pass them. This does involve some minor runtime overhead for using reflection, but this is actually compensated by not passing any closure arguments (1M invocations that do no use arguments show ~40% performance improvements, 1M invocations that do use arguments show a ~3% performance degradation).

More importantly, not passing these arguments has the side effect of these arguments not showing up in the call stack anymore. This is particularly important when throwing an Exception, as it would otherwise keep a reference to the promise instance in its call stack (while the promise stores the reference to this exception) and as such would cause a cyclic garbage reference.

Previously, these cyclic garbage references were eventually freed by the cyclic garbage collector after accumulating around 10000 entries with somewhere between 8MB to 15 MB. These can now be freed immediately and thus no longer cause any unexpected memory growth. This implementation includes some of the ideas discussed in reactphp/promise-timer#32, #46 and reactphp/socket#113.

Note that this PR does not resolve all unexpected memory issues. However, it addresses a very common problem for many consumers of this library and makes many of the higher level work-arounds obsolete. My vote would to be get this in here now as it addresses a relevant memory issue and eventually address any additional issues on top of this. :shipit:

@WyriHaximus WyriHaximus merged commit 389551e into reactphp:2.x Apr 24, 2018
@clue clue deleted the no-resolvers branch April 24, 2018 18:39
@jsor jsor added this to the v2.6.0 milestone Apr 24, 2018
@@ -221,18 +221,36 @@ private function extract($promise)

private function call(callable $callback)
{
// Use reflection to inspect number of arguments expected by this callback.
// We did some careful benchmarking here: Using reflection to avoid unneeded
// function arguments is actually faster than blindly passing them.
Copy link

Choose a reason for hiding this comment

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

The reason for it being faster isn't avoiding the arguments being passed, but rather the three closure creations that are saved.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kelunik Do you feel that this helps with understanding the motivation for this change? If so, I encourage you to file this as a new documentation PR and maybe back this with some numbers :shipit:

Copy link

Choose a reason for hiding this comment

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

@clue I think the motivation is completely covered by the circular references, the rest is just additional background on the performance of the patch. It's not super important to change, just a minor note I had while reading.

jsor added a commit to jsor-labs/pact that referenced this pull request May 16, 2018
This incorporates the work done by @clue in the following PR's for reactphp/promise:

* reactphp/promise#113
* reactphp/promise#115
* reactphp/promise#116
* reactphp/promise#117
* reactphp/promise#118
* reactphp/promise#119

Co-authored-by: Christian Lück <christian@lueck.tv>
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.

4 participants