search

Roave / BetterReflection

:crystal_ball: Better Reflection is a reflection API that aims to improve and provide more features than PHP's built-in reflection API.
MIT License
1.19k stars 131 forks source link

Improve `ReflectionEnum->getCases()` performance #1410

Open staabm opened 7 months ago

staabm commented 7 months ago

the method is showing up in profiles of https://github.com/phpstan/phpstan/issues/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 :)

grafik

lets remove the array_map magic and the unnecessary closure invocation

kukulich commented 7 months ago

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

Ocramius commented 7 months ago

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 commented 7 months ago

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

staabm commented 7 months ago

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 commented 7 months ago

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 ;-)

staabm commented 7 months ago

I'd rather memoize cases instead, in future.

done

staabm commented 7 months ago

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

Ocramius commented 7 months ago

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

ondrejmirtes commented 7 months ago

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: https://github.com/phpstan/phpstan-src/pull/2985

Ocramius commented 7 months ago

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

staabm commented 7 months ago

I am fine with closing it