Improve ReflectionEnum->getCases() performance

[staabm]

the method is showing up in profiles of phpstan/phpstan#10772
I am aware that this PR will not fix the root cause of the perf problem, but its a low hanging fruit on my way :)

lets remove the array_map magic and the unnecessary closure invocation

[kukulich]

@staabm yes, the best optimization would be to remove the adapter layer from PHPStan :)

[Ocramius]

I'm OK with merging this, but it will eventually be re-factored again to an array_map() later on anyway :|

I'm convinced method tracing is actually inflating this more than it actually is relevant.

I'd rather memoize cases instead, in future.

[Ocramius]

lets remove the array_map() magic and the unnecessary closure invocation

FWIW, array_map() is less magic than loops in general: it's just that the engine suuuuucks at handling it :-P

[Ocramius]

This method call is repeated in the loop: let's pull it out of here

[staabm]

FWIW, array_map() is less magic than loops in general

I feel array_map makes me read code backwards. I really like the simplicity of foreach. maybe I suck in reading it as much as the engine ;-)

[Ocramius]

You need to spend a couple months writing FP (Rust, Haskell, clojure, etc.), and then C-style loops start looking like the garbled mess that they are ;-)

[Ocramius]

Consideration here: we perhaps return list<ReflectionEnumUnitCase>|list<ReflectionEnumBackedCase> rather than list<ReflectionEnumUnitCase|ReflectionEnumBackedCase> 🤔

[staabm]

AFAIK phpstan is not able to differentiate it, but I can do it if you prefer

[Ocramius]

It's more precise for the consumer regardless, no?

[staabm]

yeah. I already changed it silently :)

[staabm]

I'd rather memoize cases instead, in future.

done

[staabm]

please advice on how to make psalm happy or how/where to move this thing ;)

[Ocramius]

I can try helping later tonight, but currently stuck with another issue at work.

[ondrejmirtes]

My opinion - this kind of micro optimization isn't worth it. It's only showing 8 % improvement in PHPStan because it's working with huge amounts of data. Once the root cause in PHPStan is fixed then the improvement isn't going to be 8 %, but barely measurable.

The root cause was already fixed: phpstan/phpstan-src#2985

[Ocramius]

Most likely, but spiking memoization as a general outcome is still worth pursuing :D

[staabm]

I am fine with closing it

[Ocramius]

I'm thinking of this, meanwhile:

/** 
 * @template T
 * @internal do not touch: you have been warned.
 */
final class Memoize
{
    private readonly mixed $cached;
    /** @param pure-Closure(): T $cb */
    public function __construct(private Closure|null $cb) {}
    public function get(): mixed {
        if ($this->cb) {
            $this->cached = ($this->cb)();
            $this->cb = null;
       }
        
        return $this->cached;
    }
}

This could be used general-purpose on most adapters, if needed.

[Ocramius]

mostly what you wrote, except I don't care if $cached is null (valid result)

[staabm]

thanks - copied it over. the SA tools don't like it ;)

[Ocramius]

Well, that's good :D

We are indeed bending the rules here: hence the @internal stuff as well.

I'll likely need to take over on this: a good measure of error suppression will be needed, but the impure-closure bit is most annoying (it shouldn't consider it impure 🤔 )