Skip to content

Conversation

@TimWolla
Copy link
Member

Apply the optimization for static method calls to new calls, since new is effectively a static method for all intents and purposes.

For:

<?php

final class MyClass
{
	private function __construct(
		private \Random\Engine $foo,
		private int $bar = 0,
	) {}

	public static function new(int $bar): self
	{
		$engine = new \Random\Engine\Xoshiro256StarStar(seed: 123);
		return new self(foo: $engine, bar: $bar);
	}
}

for ($i = 0; $i < 3_000_000; $i++) {
	MyClass::new($i);
}

This is ~1.13 faster for a gcc 13.3 release build on a Intel(R) Core(TM) i7-1365U.

Benchmark 1: /tmp/bench/php.old /tmp/bench/test6.php
  Time (mean ± σ):     409.5 ms ±   1.9 ms    [User: 406.6 ms, System: 2.2 ms]
  Range (min … max):   407.4 ms … 414.0 ms    10 runs

Benchmark 2: /tmp/bench/php.new /tmp/bench/test6.php
  Time (mean ± σ):     360.9 ms ±   1.7 ms    [User: 358.5 ms, System: 2.2 ms]
  Range (min … max):   359.2 ms … 365.0 ms    10 runs

Summary
  /tmp/bench/php.new /tmp/bench/test6.php ran
    1.13 ± 0.01 times faster than /tmp/bench/php.old /tmp/bench/test6.php

@mvorisek
Copy link
Contributor

Can this be tested somehow? (like opcache dump or so?)

@TimWolla TimWolla requested a review from dstogov as a code owner October 22, 2025 12:25
@TimWolla TimWolla requested a review from arnaud-lb October 22, 2025 12:52
TimWolla added a commit to TimWolla/Valinor that referenced this pull request Oct 22, 2025
Named parameters are measurably slower than positional parameters. `$value`
appears to be the “main property” of the Node class and is always passed, thus
passing it positionally is reasonable without forcing a “mental lookup” of
parameter names.

For:

    <?php

    use CuyZ\Valinor\MapperBuilder;

    require('vendor/autoload.php');

    final class Target
    {
        /**
         * @param   array{
         *              foo?: string,
         *              bar?: int,
         *              baz?: array<string, int>,
         *          } $foo
         * @param   array<string,string> $bar
         */
        public function __construct(
            public readonly array $foo,
            public readonly array $bar,
        )
        {
        }
    }

    $mapper = (new MapperBuilder())
        ->mapper();

    for ($i = 0; $i < 70_000; $i++) {
            $mapper->map(
                Target::class,
                [
                    "bar" => [],
                    "foo" => [
                        "foo" => "foo",
                    ]
                ]
            );
    }

This change results in:

    Benchmark 1: php -d opcache.enable_cli=1 valinor/test.php
      Time (mean ± σ):     786.9 ms ±   9.2 ms    [User: 768.5 ms, System: 16.0 ms]
      Range (min … max):   776.9 ms … 807.2 ms    10 runs

    Benchmark 2: php -d opcache.enable_cli=1 valinor.before/test.php
      Time (mean ± σ):     799.3 ms ±   7.9 ms    [User: 778.7 ms, System: 18.3 ms]
      Range (min … max):   790.1 ms … 813.2 ms    10 runs

    Summary
      php -d opcache.enable_cli=1 valinor/test.php ran
        1.02 ± 0.02 times faster than php -d opcache.enable_cli=1 valinor.before/test.php

see php/php-src#20259
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

I would also like to see an opcache test, as this is simple enough to verify. LGTM otherwise.

@TimWolla
Copy link
Member Author

I would also like to see an opcache test

Okay, test added.

Apply the optimization for static method calls to `new` calls, since `new` is
effectively a static method for all intents and purposes.

For:

    <?php

    final class MyClass
    {
    	private function __construct(
    		private \Random\Engine $foo,
    		private int $bar = 0,
    	) {}

    	public static function new(int $bar): self
    	{
    		$engine = new \Random\Engine\Xoshiro256StarStar(seed: 123);
    		return new self(foo: $engine, bar: $bar);
    	}
    }

    for ($i = 0; $i < 3_000_000; $i++) {
    	MyClass::new($i);
    }

This is ~1.13 faster for a gcc 13.3 release build on a Intel(R) Core(TM)
i7-1365U.

    Benchmark 1: /tmp/bench/php.old /tmp/bench/test6.php
      Time (mean ± σ):     409.5 ms ±   1.9 ms    [User: 406.6 ms, System: 2.2 ms]
      Range (min … max):   407.4 ms … 414.0 ms    10 runs

    Benchmark 2: /tmp/bench/php.new /tmp/bench/test6.php
      Time (mean ± σ):     360.9 ms ±   1.7 ms    [User: 358.5 ms, System: 2.2 ms]
      Range (min … max):   359.2 ms … 365.0 ms    10 runs

    Summary
      /tmp/bench/php.new /tmp/bench/test6.php ran
        1.13 ± 0.01 times faster than /tmp/bench/php.old /tmp/bench/test6.php
@TimWolla TimWolla merged commit 025e16c into php:master Oct 23, 2025
10 checks passed
@TimWolla TimWolla deleted the new-optimize branch October 23, 2025 07:28
romm pushed a commit to CuyZ/Valinor that referenced this pull request Oct 23, 2025
Named parameters are measurably slower than positional parameters.
`$value` appears to be the “main property” of the Node class and is
always passed, thus passing it positionally is reasonable without
forcing a “mental lookup” of parameter names.

For:

```php
<?php

use CuyZ\Valinor\MapperBuilder;

require('vendor/autoload.php');

final class Target
{
    /**
     * @param   array{
     *              foo?: string,
     *              bar?: int,
     *              baz?: array<string, int>,
     *          } $foo
     * @param   array<string,string> $bar
     */
    public function __construct(
        public readonly array $foo,
        public readonly array $bar,
    )
    {
    }
}

$mapper = (new MapperBuilder())
    ->mapper();

for ($i = 0; $i < 70_000; $i++) {
        $mapper->map(
            Target::class,
            [
                "bar" => [],
                "foo" => [
                    "foo" => "foo",
                ]
            ]
        );
}
```

This change results in:

```
Benchmark 1: php -d opcache.enable_cli=1 valinor/test.php
  Time (mean ± σ):     786.9 ms ±   9.2 ms    [User: 768.5 ms, System: 16.0 ms]
  Range (min … max):   776.9 ms … 807.2 ms    10 runs

Benchmark 2: php -d opcache.enable_cli=1 valinor.before/test.php
  Time (mean ± σ):     799.3 ms ±   7.9 ms    [User: 778.7 ms, System: 18.3 ms]
  Range (min … max):   790.1 ms … 813.2 ms    10 runs

Summary
  php -d opcache.enable_cli=1 valinor/test.php ran
    1.02 ± 0.02 times faster than php -d opcache.enable_cli=1 valinor.before/test.php
```

see php/php-src#20259
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.

5 participants