search

laminas / laminas-di

Automated dependency injection for PSR-11 containers
https://docs.laminas.dev/laminas-di/
BSD 3-Clause "New" or "Revised" License
36 stars 20 forks source link

Remove redundant `uasort()`, improve `ClassDefinition` performance #64

Closed sjokkateer closed 2 years ago

sjokkateer commented 2 years ago
Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA no

Description

A minor optimization by removing a redundant call to uasort() in the ClassDefinition::reflectParameters() method.

To test for equality the following class and test were used:

class SomeClass
{
    public function __construct(
        string $first,
        int $second,
        $third
    ) {
    }
}

final class ClaimTest extends TestCase
{
    public function testRedundantUaSortInClassDefinition(): void
    {
        $reflectionClass       = new ReflectionClass(SomeClass::class);
        $constructor           = $reflectionClass->getConstructor();
        $constructorParameters = $constructor->getParameters();

        $parameters = [];
        foreach ($constructorParameters as $parameter) {
            $parameters[$parameter->getName()] = $parameter;
        }

        // Before
        static::assertEquals(
            $constructorParameters,
            array_values($parameters)
        );

        uasort(
            $parameters,
            fn ($a, $b) => $a->getPosition() - $b->getPosition()
        );

        // After
        static::assertEquals(
            $constructorParameters,
            array_values($parameters)
        );
    }
}

The result of the test:

.                                                                   1 / 1 (100%)

Time: 00:00.002, Memory: 6.00 MB

OK (1 test, 2 assertions)

The testRedundantUaSortInClassDefinition() method demonstrates the current behaviour of the ClassDefenition::reflectParameters() method.

Within this method, the current class' constructor parameters are obtained through a call to the getParameters() method. According to the documentation of the ReflectionFunctionAbstract::getParameters() method, these parameters come in the order in which they are defined in the source. Thus, in the order in which they are defined within the class' constructor.

Within the foreach loop the parameters are added by their parameter name. PHP's array is an ordered map and these will thus follow the same order in which they are defined within the class' constructor.

This means that both the original array of constructor parameters and a call to array_values() on the newly created parameters array result in two identical arrays. This is validated by the first assertion of the test.

Based on the comparison function that is provided to theuasort() we know the order will be defined by the underlying ReflectionParameter::getPosition() method, which sorts the array in increasing order as positions are unique. This will therefore result in an array identical to the result of the call toReflectionFunctionAbstract::getParameters() validated by the last assertion of the test. It should therefore be safe to delete the uasort() function call.

If there is anything I misinterpreted or overlooked please let me know.

sjokkateer commented 2 years ago

Tests needed to avoid going into a regression scenario (test must be green before/after).

Meanwhile, WDYT about adding a benchmark test suite with some minimal examples in it, like we do in https://github.com/laminas/laminas-servicemanager/tree/dc255c268124f19709f1494aa1259967d2ecab1a/benchmarks ?

I'm up for it, I'll take a closer look at the benchmark runner and the benchmark test suite soon.

Ocramius commented 2 years ago 
❯ ./vendor/bin/phpbench run --warmup=2 --revs=5 --iterations=10 --retry-threshold=5 --tag=before_patch_64
PHPBench (1.2.5) running benchmarks... #standwithukraine
with configuration file: /home/ocramius/Documents/laminas/laminas-di/phpbench.json
with PHP version 8.1.7, xdebug ❌, opcache ❌

\LaminasBench\Di\ClassDefinitionBench

    benchGetParametersBeforeTransparentCach.R2 I9 - Mo0.200μs (±0.00%)
    benchGetParametersBeforeTransparentCach.R1 I7 - Mo0.600μs (±0.00%)
    benchGetParametersBeforeTransparentCach.R2 I7 - Mo1.600μs (±0.00%)

Subjects: 3, Assertions: 0, Failures: 0, Errors: 0
Storing results ... OK
Run: 1348ad5397ac2b4cec5ad3197803445ec0b57c2f
❯ ./vendor/bin/phpbench run --warmup=2 --revs=5 --iterations=10 --retry-threshold=5 --tag=after_patch_64
PHPBench (1.2.5) running benchmarks... #standwithukraine
with configuration file: /home/ocramius/Documents/laminas/laminas-di/phpbench.json
with PHP version 8.1.7, xdebug ❌, opcache ❌

\LaminasBench\Di\ClassDefinitionBench

    benchGetParametersBeforeTransparentCach.R10 I9 - Mo0.000μs (±0.00%)
    benchGetParametersBeforeTransparentCach.R10 I9 - Mo0.200μs (±0.00%)
    benchGetParametersBeforeTransparentCach.R2 I2 - Mo1.000μs (±0.00%)

Subjects: 3, Assertions: 0, Failures: 0, Errors: 0

Deprecated: DOMDocument::createTextNode(): Passing null to parameter #1 ($data) of type string is deprecated in /home/ocramius/Documents/laminas/laminas-di/vendor/phpbench/dom/lib/Element.php on line 37
Storing results ... 
Deprecated: DOMDocument::createTextNode(): Passing null to parameter #1 ($data) of type string is deprecated in /home/ocramius/Documents/laminas/laminas-di/vendor/phpbench/dom/lib/Element.php on line 37
OK
Run: 1348ad529b868f7a619cc7024bacef3eb3f71848
laminas-di on  HEAD (79c991a) via 🐘 v8.1.7 via ❄️  impure (nix-shell) 
❯ ./vendor/bin/phpbench report --ref=before_patch_64 --ref=after_patch_64 --report=aggregate
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
| benchmark            | subject                                                                 | set | revs | its | mem_peak | mode    | rstdev |
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingNoConstructor                 |     | 5    | 10  | 1.664mb  | 0.200μs | ±0.00% |
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingNoConstructorParameters       |     | 5    | 10  | 1.664mb  | 0.600μs | ±0.00% |
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingMultipleConstructorParameters |     | 5    | 10  | 1.664mb  | 1.600μs | ±0.00% |
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
laminas-di on  HEAD (79c991a) via 🐘 v8.1.7 via ❄️  impure (nix-shell) 
❯ ./vendor/bin/phpbench report --ref=after_patch_64 --report=aggregate
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
| benchmark            | subject                                                                 | set | revs | its | mem_peak | mode    | rstdev |
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingNoConstructor                 |     | 5    | 10  | 1.664mb  | 0.000μs | ±0.00% |
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingNoConstructorParameters       |     | 5    | 10  | 1.664mb  | 0.200μs | ±0.00% |
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingMultipleConstructorParameters |     | 5    | 10  | 1.664mb  | 1.000μs | ±0.00% |
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+

Seems like a very good improvement, nice! :)

Obviously, the entire exercise was just to raise awareness of making this a measured improvement, but still, the results are visible, especially in large laminas/laminas-di dependency graphs! :+1:

Ocramius commented 2 years ago

:ship: will also do a release, thanks @sjokkateer!

sjokkateer commented 2 years ago 
❯ ./vendor/bin/phpbench run --warmup=2 --revs=5 --iterations=10 --retry-threshold=5 --tag=before_patch_64
PHPBench (1.2.5) running benchmarks... #standwithukraine
with configuration file: /home/ocramius/Documents/laminas/laminas-di/phpbench.json
with PHP version 8.1.7, xdebug ❌, opcache ❌

\LaminasBench\Di\ClassDefinitionBench

    benchGetParametersBeforeTransparentCach.R2 I9 - Mo0.200μs (±0.00%)
    benchGetParametersBeforeTransparentCach.R1 I7 - Mo0.600μs (±0.00%)
    benchGetParametersBeforeTransparentCach.R2 I7 - Mo1.600μs (±0.00%)

Subjects: 3, Assertions: 0, Failures: 0, Errors: 0
Storing results ... OK
Run: 1348ad5397ac2b4cec5ad3197803445ec0b57c2f
❯ ./vendor/bin/phpbench run --warmup=2 --revs=5 --iterations=10 --retry-threshold=5 --tag=after_patch_64
PHPBench (1.2.5) running benchmarks... #standwithukraine
with configuration file: /home/ocramius/Documents/laminas/laminas-di/phpbench.json
with PHP version 8.1.7, xdebug ❌, opcache ❌

\LaminasBench\Di\ClassDefinitionBench

    benchGetParametersBeforeTransparentCach.R10 I9 - Mo0.000μs (±0.00%)
    benchGetParametersBeforeTransparentCach.R10 I9 - Mo0.200μs (±0.00%)
    benchGetParametersBeforeTransparentCach.R2 I2 - Mo1.000μs (±0.00%)

Subjects: 3, Assertions: 0, Failures: 0, Errors: 0

Deprecated: DOMDocument::createTextNode(): Passing null to parameter #1 ($data) of type string is deprecated in /home/ocramius/Documents/laminas/laminas-di/vendor/phpbench/dom/lib/Element.php on line 37
Storing results ... 
Deprecated: DOMDocument::createTextNode(): Passing null to parameter #1 ($data) of type string is deprecated in /home/ocramius/Documents/laminas/laminas-di/vendor/phpbench/dom/lib/Element.php on line 37
OK
Run: 1348ad529b868f7a619cc7024bacef3eb3f71848
laminas-di on  HEAD (79c991a) via 🐘 v8.1.7 via ❄️  impure (nix-shell) 
❯ ./vendor/bin/phpbench report --ref=before_patch_64 --ref=after_patch_64 --report=aggregate
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
| benchmark            | subject                                                                 | set | revs | its | mem_peak | mode    | rstdev |
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingNoConstructor                 |     | 5    | 10  | 1.664mb  | 0.200μs | ±0.00% |
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingNoConstructorParameters       |     | 5    | 10  | 1.664mb  | 0.600μs | ±0.00% |
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingMultipleConstructorParameters |     | 5    | 10  | 1.664mb  | 1.600μs | ±0.00% |
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
laminas-di on  HEAD (79c991a) via 🐘 v8.1.7 via ❄️  impure (nix-shell) 
❯ ./vendor/bin/phpbench report --ref=after_patch_64 --report=aggregate
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
| benchmark            | subject                                                                 | set | revs | its | mem_peak | mode    | rstdev |
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingNoConstructor                 |     | 5    | 10  | 1.664mb  | 0.000μs | ±0.00% |
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingNoConstructorParameters       |     | 5    | 10  | 1.664mb  | 0.200μs | ±0.00% |
| ClassDefinitionBench | benchGetParametersBeforeTransparentCachingMultipleConstructorParameters |     | 5    | 10  | 1.664mb  | 1.000μs | ±0.00% |
+----------------------+-------------------------------------------------------------------------+-----+------+-----+----------+---------+--------+

Seems like a very good improvement, nice! :)

Obviously, the entire exercise was just to raise awareness of making this a measured improvement, but still, the results are visible, especially in large laminas/laminas-di dependency graphs! 👍

@Ocramius Glad I can contribute to you guys, also thank you for giving me the opportunity to learn about PHPBench!