Skip to content

[Messenger] Improve return type of HandlerDescriptor::getHandler() #60000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 19, 2025
Merged

[Messenger] Improve return type of HandlerDescriptor::getHandler() #60000

merged 1 commit into from
Mar 19, 2025

Conversation

TimWolla
Copy link
Contributor

@TimWolla TimWolla commented Mar 18, 2025 •
edited by OskarStark
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? no
Issues n/a
License MIT

Explanation is given in the individual commit messages. I'm repeating them here for visibility:

A return type of callable is not particularly useful, since it does not
actually guarantee that the returned value is callable by the caller. It only
guarantees that it is callable by the callee as seen in this example:

class Foo {
    public function bar(): callable
    {
        return [self::class, 'baz'];
    }

    private static function baz(): void
    {
        echo __METHOD__;
    }
}

$foo = new Foo();
$cb = $foo->bar();
$cb();

Since HandlerDescriptor::$handler is already typed \Closure and since the
class is final, we can simply adjust the return type to \Closure.

Since the previous commit this is guaranteed to be a \Closure. A microbenchmark using:

function closure_(\Closure $f) {
    $f('abc');
}

function callable_(callable $f) {
    $f('abc');
}

for ($i = 0; $i < 10000000; $i++) {
    callable_(strrev(...));
}

Indicates that using \Closure as the parameter type is roughly 10% faster:

Benchmark 1: php callable.php
  Time (mean ± σ):     708.4 ms ±   5.8 ms    [User: 696.4 ms, System: 11.5 ms]
  Range (min … max):   699.5 ms … 715.7 ms    10 runs

Benchmark 2: php closure.php
  Time (mean ± σ):     647.1 ms ±  21.0 ms    [User: 633.2 ms, System: 13.2 ms]
  Range (min … max):   626.7 ms … 683.7 ms    10 runs

Summary
  php closure.php ran
    1.09 ± 0.04 times faster than php callable.php

@carsonbot carsonbot added this to the 7.3 milestone Mar 18, 2025
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@TimWolla
Copy link
Contributor Author

I think the build failure is unrelated and resolved with #59999.

@nicolas-grekas nicolas-grekas force-pushed the messenger-handler-descriptor-getHandler-type branch from 6944fde to 85d4ad3 Compare March 19, 2025 14:08
@nicolas-grekas
Copy link
Member

Thank you @TimWolla.

@nicolas-grekas nicolas-grekas merged commit 8634b21 into symfony:7.3 Mar 19, 2025
6 checks passed
@TimWolla TimWolla deleted the messenger-handler-descriptor-getHandler-type branch March 19, 2025 16:01
@TimWolla TimWolla mentioned this pull request Mar 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Messenger Status: Needs Review
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
3 participants
@TimWolla @carsonbot @nicolas-grekas