From 87baa862eab213175e88c9d2042114788484bf7b Mon Sep 17 00:00:00 2001 From: Jan Sorgalla Date: Wed, 16 May 2018 14:33:12 +0200 Subject: [PATCH] Improve memory consumption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/Promise.php | 154 ++++++++++++++++++++++++++++-------- tests/PromiseCancelTest.php | 26 ++++-- tests/PromiseRejectTest.php | 152 +++++++++++++++++++++++++++++++++++ 3 files changed, 293 insertions(+), 39 deletions(-) diff --git a/src/Promise.php b/src/Promise.php index 8b32408..582528c 100644 --- a/src/Promise.php +++ b/src/Promise.php @@ -10,6 +10,12 @@ final class Promise const STATE_FULFILLED = 3; const STATE_REJECTED = 4; + /** + * Constant used to explicitly overwrite arguments and references. + * This ensures that they do not show up in stack traces in PHP 7+. + */ + const GC_CLEANUP = '[Pact\Promise::GC_CLEANUP]'; + /** * @internal */ @@ -85,8 +91,14 @@ public function __construct($resolver = null, $canceller = null) $this->canceller = $canceller; + if (null !== $canceller) { + $canceller = self::GC_CLEANUP; + } + if (null !== $resolver) { - $this->_resolveFromCallback($resolver); + $cb = $resolver; + $resolver = self::GC_CLEANUP; + $this->_resolveFromCallback($cb); } } @@ -258,12 +270,12 @@ public function cancel() $parent->requiredCancelRequests--; if ($parent->requiredCancelRequests <= 0) { - $parentCanceller = array($parent, 'cancel'); + $parentCanceller = array(&$parent, 'cancel'); } } else { // Parent is a foreign promise, check for cancel() is already // done in _resolveCallback() - $parentCanceller = array($parent, 'cancel'); + $parentCanceller = array(&$parent, 'cancel'); } } @@ -276,6 +288,8 @@ public function cancel() \call_user_func($parentCanceller); } + $parent = self::GC_CLEANUP; + // Must be set after cancellation chain is run $this->isCancelled = true; } @@ -504,49 +518,53 @@ private function _target() return $target; } - private function _resolveFromCallback($callback, $unblock = false) + private function _resolveFromCallback($cb, $unblock = false) { - $that = $this; + $callback = $cb; + $cb = self::GC_CLEANUP; + + // 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. + // Also, this helps avoiding unnecessary function arguments in the call stack + // if the callback creates an Exception (creating garbage cycles). + if (is_array($callback)) { + $ref = new \ReflectionMethod($callback[0], $callback[1]); + } elseif (is_object($callback) && !$callback instanceof \Closure) { + $ref = new \ReflectionMethod($callback, '__invoke'); + } else { + $ref = new \ReflectionFunction($callback); + } + + $args = $ref->getNumberOfParameters(); try { + if ($args === 0) { + $callback(); + return; + } + + // Keep a reference to this promise instance for the static + // resolve/reject functions. + // See also resolveFunction() and rejectFunction() for more details. + $target = &$this; + \call_user_func( $callback, - function ($value = null) use ($that, $unblock) { - if ($unblock) { - $that->state = Promise::STATE_PENDING; - } - - $that->_resolve($value); - }, - // Allow rejecting with non-throwable reasons to ensure - // interoperability with foreign promise implementations which - // may allow arbitrary reason types or even rejecting without - // a reason. - function ($reason = null) use ($that, $unblock) { - if (null === $reason) { - if (0 === \func_num_args()) { - $reason = ReasonException::createWithoutReason(); - } else { - $reason = ReasonException::createForReason(null); - } - } elseif (!$reason instanceof \Throwable && !$reason instanceof \Exception) { - $reason = ReasonException::createForReason($reason); - } - - if ($unblock) { - $that->state = Promise::STATE_PENDING; - } - - $that->_reject($reason); - } + self::resolveFunction($target, $unblock), + self::rejectFunction($target, $unblock) ); } catch (\Exception $e) { + $target = self::GC_CLEANUP;; + if ($unblock) { $this->state = Promise::STATE_PENDING; } $this->_reject($e); } catch (\Throwable $e) { + $target = self::GC_CLEANUP; + if ($unblock) { $this->state = Promise::STATE_PENDING; } @@ -555,6 +573,76 @@ function ($reason = null) use ($that, $unblock) { } } + /** + * Creates a static resolver callback that is not bound to a promise instance. + * + * Moving the closure creation to a static method allows us to create a + * callback that is not bound to a promise instance. By passing the target + * promise instance by reference, we can still execute its resolving logic + * and still clear this reference when settling the promise. This helps + * avoiding garbage cycles if any callback creates an Exception. + * + * These assumptions are covered by the test suite, so if you ever feel like + * refactoring this, go ahead, any alternative suggestions are welcome! + */ + private static function resolveFunction(self &$target, $unblock) + { + return function ($value = null) use (&$target, $unblock) { + if (Promise::GC_CLEANUP === $target) { + return; + } + + if ($unblock) { + $target->state = Promise::STATE_PENDING; + } + + $target->_resolve($value); + $target = Promise::GC_CLEANUP; + }; + } + + /** + * Creates a static rejection callback that is not bound to a promise instance. + * + * Moving the closure creation to a static method allows us to create a + * callback that is not bound to a promise instance. By passing the target + * promise instance by reference, we can still execute its rejection logic + * and still clear this reference when settling the promise. This helps + * avoiding garbage cycles if any callback creates an Exception. + * + * These assumptions are covered by the test suite, so if you ever feel like + * refactoring this, go ahead, any alternative suggestions are welcome! + */ + private static function rejectFunction(self &$target, $unblock) + { + // Allow rejecting with non-throwable reasons to ensure + // interoperability with foreign promise implementations which + // may allow arbitrary reason types or even rejecting without + // a reason. + return function ($reason = null) use (&$target, $unblock) { + if (Promise::GC_CLEANUP === $target) { + return; + } + + if (null === $reason) { + if (0 === \func_num_args()) { + $reason = ReasonException::createWithoutReason(); + } else { + $reason = ReasonException::createForReason(null); + } + } elseif (!$reason instanceof \Throwable && !$reason instanceof \Exception) { + $reason = ReasonException::createForReason($reason); + } + + if ($unblock) { + $target->state = Promise::STATE_PENDING; + } + + $target->_reject($reason); + $target = Promise::GC_CLEANUP; + }; + } + private static function enqueue($task) { if (!self::$queue) { diff --git a/tests/PromiseCancelTest.php b/tests/PromiseCancelTest.php index 19c7418..eaed864 100644 --- a/tests/PromiseCancelTest.php +++ b/tests/PromiseCancelTest.php @@ -53,15 +53,29 @@ function ($resolve, $reject) { /** @test */ public function it_invokes_canceller_with_resolver_arguments() { - $mock = $this->createCallableMock(); - $mock - ->expects($this->once()) - ->method('__invoke') - ->with($this->isType('callable'), $this->isType('callable')); + $args = null; + $promise = new Promise(function () {}, function ($resolve, $reject) use (&$args) { + $args = func_get_args(); + }); - $promise = new Promise(function () {}, $mock); + $promise->cancel(); + + $this->assertCount(2, $args); + $this->assertInternalType('callable', $args[0]); + $this->assertInternalType('callable', $args[1]); + } + + /** @test */ + public function it_invokes_canceller_without_arguments_if_not_accessed() + { + $args = null; + $promise = new Promise(function () {}, function () use (&$args) { + $args = func_num_args(); + }); $promise->cancel(); + + $this->assertSame(0, $args); } /** @test */ diff --git a/tests/PromiseRejectTest.php b/tests/PromiseRejectTest.php index 4ca145c..43d3c94 100644 --- a/tests/PromiseRejectTest.php +++ b/tests/PromiseRejectTest.php @@ -47,6 +47,158 @@ public function it_rejects_when_resolver_throws_an_error() ->then($this->expectCallableNever(), $mock); } + /** @test */ + public function it_resolves_without_creating_garbage_cycles_if_resolver_resolves_with_an_exception() + { + gc_collect_cycles(); + + $promise = new Promise(function ($resolve) { + $resolve(new \Exception('foo')); + }); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function it_rejects_without_creating_garbage_cycles_if_resolver_throws_an_exception_without_resolver() + { + gc_collect_cycles(); + + $promise = new Promise(function () { + throw new \Exception('foo'); + }); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function it_rejects_without_creating_garbage_cycles_if_resolver_rejects_with_an_exception() + { + gc_collect_cycles(); + + $promise = new Promise(function ($resolve, $reject) { + $reject(new \Exception('foo')); + }); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function it_rejects_without_creating_garbage_cycles_if_canceller_rejects_with_an_exception() + { + gc_collect_cycles(); + + $promise = new Promise(function ($resolve, $reject) { }, function ($resolve, $reject) { + $reject(new \Exception('foo')); + }); + + $promise->cancel(); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function it_rejects_without_creating_garbage_cycles_if_parent_canceller_rejects_with_exception() + { + gc_collect_cycles(); + + $promise = new Promise(function ($resolve, $reject) { }, function ($resolve, $reject) { + $reject(new \Exception('foo')); + }); + + $promise->then()->then()->then()->cancel(); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function it_rejects_without_creating_garbage_cycles_if_resolver_throws_an_exception() + { + gc_collect_cycles(); + + $promise = new Promise(function ($resolve, $reject) { + throw new \Exception('foo'); + }); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** + * Test that checks number of garbage cycles after throwing from a canceller + * that explicitly uses a reference to the promise. This is rather synthetic, + * actual use cases often have implicit (hidden) references which ought not + * to be stored in the stack trace. + * + * Reassigned arguments only show up in the stack trace in PHP 7, so we can't + * avoid this on legacy PHP. As an alternative, consider explicitly unsetting + * any references before throwing. + * + * @test + * @requires PHP 7 + */ + public function it_rejects_without_creating_garbage_cycles_if_canceller_with_reference_throws_an_exception() + { + gc_collect_cycles(); + + $promise = new Promise(function () {}, function () use (&$promise) { + throw new \Exception('foo'); + }); + + $promise->cancel(); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** + * @test + * @requires PHP 7 + * @see self::it_rejects_without_creating_garbage_cycles_if_canceller_with_reference_throws_an_exception + */ + public function it_rejects_without_creating_garbage_cycles_if_resolver_with_reference_throws_an_exception() + { + gc_collect_cycles(); + + $promise = new Promise(function () use (&$promise) { + throw new \Exception('foo'); + }); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** + * @test + * @requires PHP 7 + * @see self::it_rejects_without_creating_garbage_cycles_if_canceller_with_reference_throws_an_exception + */ + public function it_rejects_without_creating_garbage_cycles_if_canceller_holds_reference_and_resolver_throws_an_exception() + { + gc_collect_cycles(); + + $promise = new Promise(function () { + throw new \Exception('foo'); + }, function () use (&$promise) { }); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + /** @test */ public function it_rejects_with_an_immediate_exception() {