From c4a7d0668604a4c6de7a488653d5003ae8833f52 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= <tim@tideways-gmbh.com>
Date: Wed, 9 Oct 2024 09:24:29 +0200
Subject: [PATCH] reflection: Use fast ZPP for
 ReflectionProperty::(get|set)Value()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

During the Doctrine Core Team Meetup 2024 the Doctrine team investigated the
performance overhead of using `setRawValueWithoutLazyInitialization()` instead
of `setValue()` and came to the surprising conclusion that
`setRawValueWithoutLazyInitialization()` outperformed `setValue()`, despite
doing more work.

These two scripts are used as the benchmark:

    <?php

    class Foo
    {
        public $id;
        public $foo1;
        public $foo2;
        public $foo3;
        public $foo4;
    }

    $reflection = new ReflectionClass(Foo::class);
    $properties = $reflection->getProperties();

    for ($i = 0; $i < 1000000; $i++) {
        $foo = new Foo();
        foreach ($properties as $property) {
            $property->setValue($foo, 1);
        }
    }

and

    <?php

    class Foo
    {
        public $id;
        public $foo1;
        public $foo2;
        public $foo3;
        public $foo4;
    }

    $reflection = new ReflectionClass(Foo::class);
    $properties = $reflection->getProperties();

    for ($i = 0; $i < 1000000; $i++) {
        $foo = new Foo();
        foreach ($properties as $property) {
            $property->setRawValueWithoutLazyInitialization($foo, 1);
        }
    }

Benchmarking these with a current git master shows that `setValue()` is 50%
slower:

    $ hyperfine -L script setValue,setRawValueWithoutLazyInitialization '/tmp/php-before /tmp/test/{script}.php'
    Benchmark 1: /tmp/php-before /tmp/test/setValue.php
      Time (mean ± σ):     216.0 ms ±   5.8 ms    [User: 212.0 ms, System: 3.7 ms]
      Range (min … max):   208.2 ms … 225.3 ms    13 runs

    Benchmark 2: /tmp/php-before /tmp/test/setRawValueWithoutLazyInitialization.php
      Time (mean ± σ):     145.6 ms ±   3.6 ms    [User: 141.6 ms, System: 3.8 ms]
      Range (min … max):   140.4 ms … 152.8 ms    20 runs

    Summary
      /tmp/php-before /tmp/test/setRawValueWithoutLazyInitialization.php ran
        1.48 ± 0.05 times faster than /tmp/php-before /tmp/test/setValue.php

Looking into the “why” revealed that the `setValue()` script spent quite some
time in `zend_parse_parameters()`.

A 50% overhead can be significant, given that `setValue()` is commonly called
several thousand times in a single request when using Doctrine.

This commit changes the non-static property case of `setValue()` to make use of
the fast parameter parsing API and adjusts `getValue()` for consistency.

The resulting comparison shows that both `setValue()` and
`setRawValueWithoutLazyInitialization()` are now (almost) equal:

    $ hyperfine -L script setValue,setRawValueWithoutLazyInitialization 'sapi/cli/php /tmp/test/{script}.php'
    Benchmark 1: sapi/cli/php /tmp/test/setValue.php
      Time (mean ± σ):     143.0 ms ±   6.4 ms    [User: 139.4 ms, System: 3.4 ms]
      Range (min … max):   134.8 ms … 157.7 ms    18 runs

    Benchmark 2: sapi/cli/php /tmp/test/setRawValueWithoutLazyInitialization.php
      Time (mean ± σ):     147.0 ms ±   5.5 ms    [User: 143.0 ms, System: 3.6 ms]
      Range (min … max):   139.9 ms … 159.8 ms    19 runs

    Summary
      sapi/cli/php /tmp/test/setValue.php ran
        1.03 ± 0.06 times faster than sapi/cli/php /tmp/test/setRawValueWithoutLazyInitialization.php
---
 ext/reflection/php_reflection.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c
index 0c22bcaaed296..a5562f165f76e 100644
--- a/ext/reflection/php_reflection.c
+++ b/ext/reflection/php_reflection.c
@@ -4014,9 +4014,8 @@ ZEND_METHOD(ReflectionClassConstant, getValue)
 	reflection_object *intern;
 	zend_class_constant *ref;
 
-	if (zend_parse_parameters_none() == FAILURE) {
-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_NONE();
+
 	GET_REFLECTION_OBJECT_PTR(ref);
 
 	zval *name = reflection_prop_name(ZEND_THIS);
@@ -6033,7 +6032,6 @@ ZEND_METHOD(ReflectionProperty, setValue)
 {
 	reflection_object *intern;
 	property_reference *ref;
-	zval *object;
 	zval *value;
 	zval *tmp;
 
@@ -6064,11 +6062,13 @@ ZEND_METHOD(ReflectionProperty, setValue)
 
 		zend_update_static_property_ex(intern->ce, ref->unmangled_name, value);
 	} else {
-		if (zend_parse_parameters(ZEND_NUM_ARGS(), "oz", &object, &value) == FAILURE) {
-			RETURN_THROWS();
-		}
+		zend_object *object;
+		ZEND_PARSE_PARAMETERS_START(2, 2)
+			Z_PARAM_OBJ(object)
+			Z_PARAM_ZVAL(value)
+		ZEND_PARSE_PARAMETERS_END();
 
-		zend_update_property_ex(intern->ce, Z_OBJ_P(object), ref->unmangled_name, value);
+		zend_update_property_ex(intern->ce, object, ref->unmangled_name, value);
 	}
 }
 /* }}} */