Improve performance for supporting Laravel 8.77+ cast attributes #1292
Conversation
|
Nice :) |
|
Pinging also @SimonJnsson who brought us #1289 🎉 The difference can be seen in the test suite execution here too but I probably would not have noticed it: |
barryvdh#1289 calls `getReturnType` for every method on every model, which in turn calls `getReturnTypeFromDocBlock` which has this code: ```php $phpDocContext = (new ContextFactory())->createFromReflector($reflection); ``` Extracting the docblock is super slow, always has been. Now that we do this for every method, this adds up a lot. Performance on a private commercial project _before_ barryvdh#1289 was introduced: ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.857s user 0m1.835s sys 0m0.129s ``` After barryvdh#1289 : ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m54.147s user 0m47.132s sys 0m1.047s ``` However, in this case we **do not need** the phpdoc fallback (which is legitimate and by design for many other cases), because also the Laravel implementation only works by inspecting the _actual type_, see https://github.com/laravel/framework/blob/e0c2620b57be6416820ea7ca8e46fd2f71d2fe35/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L570-L575 ```php $returnType = (new ReflectionMethod($this, $method))->getReturnType(); return static::$attributeMutatorCache[get_class($this)][$key] = $returnType && $returnType instanceof ReflectionNamedType && $returnType->getName() === Attribute::class && is_callable($this->{$method}()->get); ``` This side-stepping the phpdoc parsing a) still works correctly and b) brings us back to the previous performance characteristics: ``` time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.987s user 0m1.915s sys 0m0.120s ```
|
@mfn thanks for the ping 👍 Good catch and good solution 👍 Glad someone noticed when i didn't 😅 |
|
@mfn thanks so much for the quick analysis and fix! Yes it's working just like before. Lovely and fast again! ====== Ie. If these two lines do NOT have a return type specifically specified, then nothing will be added to the docblock?
It's just because I assumed that if the function arguments had type hints then it could have been inferred and I spend 10 mins wondering why I wasn't getting anything until I noticed I didn't have a return type specified. Thanks! |
Yep, we look at the return type of the get and set methods, thus if these are not provided no doc blocks will be created. |
…ryvdh#1292) barryvdh#1289 calls `getReturnType` for every method on every model, which in turn calls `getReturnTypeFromDocBlock` which has this code: ```php $phpDocContext = (new ContextFactory())->createFromReflector($reflection); ``` Extracting the docblock is super slow, always has been. Now that we do this for every method, this adds up a lot. Performance on a private commercial project _before_ barryvdh#1289 was introduced: ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.857s user 0m1.835s sys 0m0.129s ``` After barryvdh#1289 : ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m54.147s user 0m47.132s sys 0m1.047s ``` However, in this case we **do not need** the phpdoc fallback (which is legitimate and by design for many other cases), because also the Laravel implementation only works by inspecting the _actual type_, see https://github.com/laravel/framework/blob/e0c2620b57be6416820ea7ca8e46fd2f71d2fe35/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L570-L575 ```php $returnType = (new ReflectionMethod($this, $method))->getReturnType(); return static::$attributeMutatorCache[get_class($this)][$key] = $returnType && $returnType instanceof ReflectionNamedType && $returnType->getName() === Attribute::class && is_callable($this->{$method}()->get); ``` This side-stepping the phpdoc parsing a) still works correctly and b) brings us back to the previous performance characteristics: ``` time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.987s user 0m1.915s sys 0m0.120s ```
4 participants
Summary
#1289 calls
getReturnTypefor every method on every model, which inturn calls
getReturnTypeFromDocBlockwhich has this code:Extracting the docblock is super slow, always has been. Now that we do
this for every method, this adds up a lot.
Performance on a private commercial project before #1289 was introduced:
After #1289 :
However, in this case we do not need the phpdoc fallback (which is
legitimate and by design for many other cases), because also the Laravel
implementation only works by inspecting the actual type, see
https://github.com/laravel/framework/blob/e0c2620b57be6416820ea7ca8e46fd2f71d2fe35/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L570-L575
This side-stepping the phpdoc parsing a) still works correctly and b)
brings us back to the previous performance characteristics:
Fixes #1291
Type of change
Checklist
composer fix-style