Improve memory consumption

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 <[email protected]>
jsor-labs · May 16, 2018 · 87baa86 · 87baa86
1 parent 18b7514
commit 87baa86
Show file tree

Hide file tree

Showing 3 changed files with 293 additions and 39 deletions.
diff --git 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
@@ -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 */