Skip to content
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.

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

[9.x] Attribute Cast Performance Improvements #43554

Merged

Conversation

serpentblade
Copy link
Contributor

@serpentblade serpentblade commented Aug 4, 2022

Performance improvements to casts-related functions:

  1. Reduce number of repeated calls to getCasts() in methods
  2. Replace strncmp with str_starts_with in cast type check
  3. Add cache for getCastType() to eliminate repeated string comparisons for cast types.

Background:
I process millions of rows via scout and was curious if there were any obvious bottlenecks in the process to help speed things along. I discovered through profiling that casts-related code represented a significant number of overall calls and time spent processing my models.

In particular calls to getCasts() were unusually high as well and the time spent in getCastType() was higher than any other function.

First step was to update any methods where getCasts() was called multiple times. getCastType() seemed to be the primary culprit for the high number of calls to getCasts().

When profiling again this helped reduce the calls to getCasts() but the time spent in getCastType() was still rather high. I found isImmutableCustomDateTimeCast() still was using strncmp comparisons when str_starts_with would do instead. This change is not only an improvement to readability while matching the code of the other similar functions, but there's also a small improvement in performance with the change as an added bonus.

I then noticed that getCastType() can be cached as the results of checking the cast types are deterministic and do not need to be repeated for the same input.


Results:
With these changes I saw a 5-25% reduction in processing time depending on the complexity, number of casts, and number of attributes of a particular model. From my tests I have noticed significant improvements on both eloquent load times and serialization.


Potential risks:
This introduces a new static member variable $castTypeCache to Model that could cause conflicts if a subclass already defines one by that name. An alternative for this that is to introduce $castTypeCache as a static variable to the getCastType() method only, which is often considered bad form but does not introduce any risk of naming collision and results in the same benefit.


End user benefits:

General users will likely see negligible benefits as the changes are only really beneficial when processing a lot of models. At scale, any bulk loading and serializing of models will yield note-worthy performance improvements.


Other things to note about casts:

There are further significant performance improvements in caching hasCast(), however that is a bit more complex as there's no easy to trust that $casts or the results of getCasts() does not change during the lifecycle of a model. If a future version of Laravel introduced immutability to $casts and an expectation of determinism for getCasts() the expensive and repeated calls to hasCast() could be cached and save a significant amount of time.

serpentblade added 3 commits Aug 4, 2022
Marginally faster, better readability, and matches similar functions
getCastType is called a lot when processing models with defined casts and contributes to a significant number of overall calls.  Caching the results of the key conversion can result in significant performance improvement when processing large numbers of models.
@serpentblade serpentblade force-pushed the feature/eloquent-attributes-performance branch from 89bb0f7 to 424e8cf Compare Aug 4, 2022
@taylorotwell taylorotwell merged commit 6c83a59 into laravel:9.x Aug 5, 2022
16 checks passed
@taylorotwell
Copy link
Member

taylorotwell commented Aug 5, 2022

Thanks!

chu121su12 pushed a commit to chu121su12/framework that referenced this pull request Aug 7, 2022
* Remove repeated calls to getCasts() in same method

* Switch strncmp to starts_with_str

Marginally faster, better readability, and matches similar functions

* Add castTypeCache

getCastType is called a lot when processing models with defined casts and contributes to a significant number of overall calls.  Caching the results of the key conversion can result in significant performance improvement when processing large numbers of models.
Ken-vdE pushed a commit to Ken-vdE/framework that referenced this pull request Aug 9, 2022
* Remove repeated calls to getCasts() in same method

* Switch strncmp to starts_with_str

Marginally faster, better readability, and matches similar functions

* Add castTypeCache

getCastType is called a lot when processing models with defined casts and contributes to a significant number of overall calls.  Caching the results of the key conversion can result in significant performance improvement when processing large numbers of models.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants