Eloquent performance: improve caching in HasAttributes::getCastType() and HasAttributes::isEncryptedCastable()

[thomas-0816]

Laravel Version

12.28

PHP Version

8.4.12

Database Driver & Version

mariadb 10.11.13

Description

I profiled a long running job with spx and got:

*** SPX Report ***

Global stats:

  Called functions    :   436.7M
  Distinct functions  :     3.1K

  Wall time           :   53.16s
  ZE memory usage     :  237.5MB

Flat profile:

 Wall time           | ZE memory usage     |
 Inc.     | *Exc.    | Inc.     | Exc.     | Called   | Function
----------+----------+----------+----------+----------+----------
   12.80s |    3.52s |     376B |  -42.8GB |    25.7M | Illuminate\Database\Eloquent\Model::getCastType
   15.63s |    1.85s |     376B |       0B |     8.3M | Illuminate\Database\Eloquent\Model::castAttribute
   12.21s |  925.6ms |       0B |       0B |     2.7M | Illuminate\Database\Eloquent\Model::originalIsEquivalent
   22.95s |  813.9ms |    9.6KB |       0B |     6.5M | Illuminate\Database\Eloquent\Model::getAttributeValue
    8.41s |  760.7ms |  203.5MB |  203.5MB |     1.1M | Illuminate\Database\Eloquent\Model::setAttribute
    3.98s |  441.6ms |      80B |       0B |   492.4K | Illuminate\Support\Collection::{closure:Illuminate\Support\Traits\EnumeratesValues::operatorForWhere():1101}
    3.46s |  340.8ms |       0B |       0B |     2.3M | Illuminate\Database\Eloquent\Model::isDateAttribute
    5.90s |  273.9ms |   43.5MB |   -1.0MB |    25.7K | Illuminate\Support\Collection::groupBy

then I changed the HasAttributes::getCastType() from:

    protected function getCastType($key)
    {
        $castType = $this->getCasts()[$key];

        if (isset(static::$castTypeCache[$castType])) {
            return static::$castTypeCache[$castType];
        }

        if ($this->isCustomDateTimeCast($castType)) {
            $convertedCastType = 'custom_datetime';
        } elseif ($this->isImmutableCustomDateTimeCast($castType)) {
            $convertedCastType = 'immutable_custom_datetime';
        } elseif ($this->isDecimalCast($castType)) {
            $convertedCastType = 'decimal';
        } elseif (class_exists($castType)) {
            $convertedCastType = $castType;
        } else {
            $convertedCastType = trim(strtolower($castType));
        }

        return static::$castTypeCache[$castType] = $convertedCastType;
    }

to:

    protected function getCastType($key)
    {
        if (isset(static::$castTypeCache[static::class][$key])) {
            return static::$castTypeCache[static::class][$key];
        }

        $castType = $this->getCasts()[$key];

        if ($this->isCustomDateTimeCast($castType)) {
            $convertedCastType = 'custom_datetime';
        } elseif ($this->isImmutableCustomDateTimeCast($castType)) {
            $convertedCastType = 'immutable_custom_datetime';
        } elseif ($this->isDecimalCast($castType)) {
            $convertedCastType = 'decimal';
        } elseif (class_exists($castType)) {
            $convertedCastType = $castType;
        } else {
            $convertedCastType = trim(strtolower($castType));
        }

        return static::$castTypeCache[static::class][$key] = $convertedCastType;
    }

The runtime of the job went from 21.7s to 17.7s (without spx).

Then I changed HasAttributes::isEncryptedCastable() from:

    protected function isEncryptedCastable($key)
    {
        return $this->hasCast($key, ['encrypted', 'encrypted:array', 'encrypted:collection', 'encrypted:json', 'encrypted:object']);
    }

to:

    protected static $encryptedCastableCache = [];

    protected function isEncryptedCastable($key)
    {
        if (isset(static::$encryptedCastableCache[static::class][$key])) {
            return static::$encryptedCastableCache[static::class][$key];
        }

        return static::$encryptedCastableCache[static::class][$key] = $this->hasCast($key, ['encrypted', 'encrypted:array', 'encrypted:collection', 'encrypted:json', 'encrypted:object']);
    }

The runtime of the job went from 17.7s to 15.1s (without spx). So a total of 30% faster.

What do you think about these changes?

Note: I can't provide a PR for this, but you and the community are free to create a PR for this.

Steps To Reproduce

Run a script using Eloquent with many Model instances.

[macropay-solutions]

Hi,
We don't use casts because we know they are just a brake for the execution but still, after your investigation it seems we are still affected by them just being a feature in the framework.

1. Can you confirm that this happens even if you don't use casts?

2. Also there is the issue of octane AND worker in daemon mode. Can you confirm that your changes are not affecting them because of the static?

3. And last, do you know the memory increase generated by these 2 static caches in long time running scripts?

[thomas-0816]

1. yes, to disable casts, $casts needs to be an empty array and $incrementing needs to be false.

2. from my tests, the change is ok for queue workers, I'm not using octane.

3. in my tests, memory_get_peak_usage(true) did not change after changing trait HasAttributes.

[thomas-0816]

I changed the code a bit to nested array $...Cache[static::class][$key] which performs even better.

The mutator caches in HasAttributes use $...Cache[get_class($this)][$key], but I think static::class should provide better performance with same functionality.

[macropay-solutions]

Interesting how this was missed when it was first implemented. The fact that the parent protected static is shared for all child models. Can't this lead to issues if in the same PID one method sets the cache for a key to something and for another model the same key is casted to another type?

Model 1

'col' needs cast fom json to array

Model 2
'col' needs cast to decimal

UPDATE:
Sorry, the above is wrong.

The initial cache holds in it ['decimal'=> 'decimal'] for any model.
The key is involved only in getting the cast not in the cache.

@thomas-0816 so you putting in there also the FQN of the model just increases the static variable needed memory. What is the advantage? The key was not part of the cache and in your versions it is.

The speed gain must be attributed to
$this->getCasts()[$key]
not being called on each function call coupled with the other change from isEncryptedCastable

[thomas-0816]

What is the advantage?

Better performance for getCastType() and isEncryptedCastable(), resulting in better performance for castAttribute().

[macropay-solutions]

@thomas-0816 can you please try this:

    protected function getCastType($key)
    {
        static $test;
        $castType = ($test[static::class] ??= $this->getCasts())[$key];

        if (isset(static::$castTypeCache[$castType])) {
            return static::$castTypeCache[$castType];
        }

        if ($this->isCustomDateTimeCast($castType)) {
            $convertedCastType = 'custom_datetime';
        } elseif ($this->isImmutableCustomDateTimeCast($castType)) {
            $convertedCastType = 'immutable_custom_datetime';
        } elseif ($this->isDecimalCast($castType)) {
            $convertedCastType = 'decimal';
        } elseif (class_exists($castType)) {
            $convertedCastType = $castType;
        } else {
            $convertedCastType = trim(strtolower($castType));
        }

        return static::$castTypeCache[$castType] = $convertedCastType;
    }

And if this is faster then you can try moving the caching in the getCasts function by static::class to see the results. That should improve also other functions that call getCasts function.

[thomas-0816]

Yes caching getCasts() also makes an improvement, great!

From the previous 15.1s it gets to 12.0s. Total from 21.7 it gives 44% better performance.

Caching isDateAttribute() gets 11.8s.

Summary of changes I made:

    protected static $getCastTypeCache = [];

    protected function getCastType($key)
    {
        if (isset(static::$getCastTypeCache[static::class][$key])) {
            return static::$getCastTypeCache[static::class][$key];
        }

        $castType = $this->getCasts()[$key];

        if ($this->isCustomDateTimeCast($castType)) {
            $convertedCastType = 'custom_datetime';
        } elseif ($this->isImmutableCustomDateTimeCast($castType)) {
            $convertedCastType = 'immutable_custom_datetime';
        } elseif ($this->isDecimalCast($castType)) {
            $convertedCastType = 'decimal';
        } elseif (class_exists($castType)) {
            $convertedCastType = $castType;
        } else {
            $convertedCastType = trim(strtolower($castType));
        }

        return static::$getCastTypeCache[static::class][$key] = $convertedCastType;
    }
    
    protected static $getCastsCache = [];

    public function getCasts()
    {
        if (isset(static::$getCastsCache[static::class])) {
            return static::$getCastsCache[static::class];
        }

        if ($this->getIncrementing()) {
            return static::$getCastsCache[static::class] = array_merge([$this->getKeyName() => $this->getKeyType()], $this->casts);
        }

        return static::$getCastsCache[static::class] = $this->casts;
    }

    protected static $isEncryptedCastableCache = [];

    protected function isEncryptedCastable($key)
    {
        if (isset(static::$isEncryptedCastableCache[static::class][$key])) {
            return static::$isEncryptedCastableCache[static::class][$key];
        }

        return static::$isEncryptedCastableCache[static::class][$key] = $this->hasCast($key, ['encrypted', 'encrypted:array', 'encrypted:collection', 'encrypted:json', 'encrypted:object']);
    }

    protected static $isDateAttributeCache = [];

    protected function isDateAttribute($key)
    {
        if (isset(static::$isDateAttributeCache[static::class][$key])) {
            return static::$isDateAttributeCache[static::class][$key];
        }

        return static::$isDateAttributeCache[static::class][$key] = (in_array($key, $this->getDates(), true) ||
            $this->isDateCastable($key));
    }

[thomas-0816]

Another performance improvement might come from using typed properties and property hooks, but I'm not sure what's the current status with Eloquent on this.

[macropay-solutions]

Another performance improvement might come from using typed properties and property hooks, but I'm not sure what's the current status with Eloquent on this.

The active record does not work well with attributes(columns) as model properties. Also property hooks are not yet mature enough because the array does not fully work. You can read more in the php documentation about it.

[macropay-solutions]

@thomas-0816

We feel that the above would NOT be a breaking change solution that can be applied also retroactively for our lib crud suite. (We support also composed pks so the bellow check for '' is needed.)

    /**
     * @inheritdoc
     */
    public function getCasts(): array
    {
        static $casts;

        return $casts[static::class] ??= ($this->getIncrementing() && ($key = (string)$this->getKeyName()) !== '' ?
            \array_merge([$key => $this->getKeyType()], $this->casts) :
            $this->casts);
    }

Adding new properties in the model is considered a breaking change.

Can you please provide the time improvement with this change only?

[thomas-0816]

Adding php method return type array to getCasts() is also a breaking change.

I think it's more important to keep the code consistent. Local static is currently not used in Eloquent, so having a protected property should be the way to go here regarding consistency. Having local static also gives no option to clear the cache.

Performance for changing only getCasts():

    public function getCasts(): array
    {
        static $casts;

        return $casts[static::class] ??= ($this->getIncrementing() && ($key = (string)$this->getKeyName()) !== '' ?
            \array_merge([$key => $this->getKeyType()], $this->casts) :
            $this->casts);
    }

=> gives 13.1s

    public function getCasts(): array
    {
        static $casts;

        return $casts[static::class] ??= ($this->getIncrementing() ?
            \array_merge([$this->getKeyName() => $this->getKeyType()], $this->casts) :
            $this->casts);
    }

=> gives 13.3s

    protected static $getCasts = [];

    public function getCasts(): array
    {
        return static::$getCasts[static::class] ??= ($this->getIncrementing() ?
            \array_merge([$this->getKeyName() => $this->getKeyType()], $this->casts) :
            $this->casts);
    }

=> gives 13.5s

    protected static $getCasts = [];

    public function getCasts(): array
    {
        return self::$getCasts[static::class] ??= ($this->getIncrementing() ?
            \array_merge([$this->getKeyName() => $this->getKeyType()], $this->casts) :
            $this->casts);
    }

=> gives 12.9s

self::class = Illuminate\Database\Eloquent\Model

static::class = <your-model-extending-eloquent-model>

[macropay-solutions]

@thomas-0816 We are trying to understand the autoincrement casting to int. Is this happening only on inserts?

Eloquent can be bypassed on lists for example see https://marius-ciclistu.medium.com/eloquent-is-faster-when-used-without-hydration-25e80c5eb135

As you can see here https://laravel-crud-wizard.com/laravel-12-free/laravel-lumen-crud-wizard#clients on list the primary key comes int not string when we bipass the eloquent.

If the cast to int is needed for autoincrement only on insert than we can avoid the cache because the casts property is the cache.

What do you think?

[macropay-solutions]

Can we get confirmation that the fact that the autoincrement is casted to int removes the need of the if from getCasts() ?

    public function getCasts()
    {
        if ($this->getIncrementing()) {
            return array_merge([$this->getKeyName() => $this->getKeyType()], $this->casts);
        }

    public function processInsertGetId(Builder $query, $sql, $values, $sequence = null)
    {
        $query->getConnection()->insert($sql, $values);

        $id = $query->getConnection()->getPdo()->lastInsertId($sequence);

        return is_numeric($id) ? (int)$id : $id;
    }

@shaedrich @mbabker @crynobone @henzeb ?

[thomas-0816]

I guess id as primary key will be automatically added if incrementing is true, so getCasts() also needs to include "id" => "int".
Depending on PDO settings (PDO::ATTR_STRINGIFY_FETCHES), id may also come as string for a select query, so it needs to be casted to int.

Also if the primary key is decimal, pdo will return a string in selects and we will need to cast it to float.

[macropay-solutions]

@thomas-0816 We can't find the initial commit that introduced that if in getCasts. Maybe it was like that from the beginning.
We use also models with autoincrement false and id as int, and we had no issues with them.

This is just a leftover from the start of laravel maybe that now we discovered together that it introduces delays in hydrating.

@taylorotwell can you shed some light on this please?

@thomas-0816 the time for:

    public function getCasts()
    {
        return $this->casts;
    }

should be less because it will not cast the pk nor it will use array_merge.

[macropay-solutions]

ATTR_STRINGIFY_FETCHES

class Connector
{
    use DetectsLostConnections;

    /**
     * The default PDO connection options.
     *
     * @var array
     */
    protected $options = [
        PDO::ATTR_CASE => PDO::CASE_NATURAL,
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_ORACLE_NULLS => PDO::NULL_NATURAL,
        PDO::ATTR_STRINGIFY_FETCHES => false,                      // it is false
        PDO::ATTR_EMULATE_PREPARES => false,
    ];

And if you set it to true makes sense to return string not int because you asked for strings.

Also if the primary key is decimal, pdo will return a string in selects and we will need to cast it to float.

Converting decimals to floats will introduce errors (calculation or representation errors). They should be handled as strings with BCMATH for example.
Can you give an example usage of PK as decimal autoincrementing?

[DanielCHood]

I had a PR that improved this drastically, although I attacked it from getAttributeValue(). It got rejected and it was targeting 11.x but there are callgrind screenshots attached that may help: #51617

The change to the unit tests in that PR is proof that casting is executed more frequently than expected.

[macropay-solutions]

@DanielCHood can you give your opinion if removing that if from getCasts function is a breaking change?

[DanielCHood]

@macropay-solutions yes, technically. The reason being is that right now the primary key, when auto incrementing, is treated as having the primary key in the $casts array as being casted to an integer. So even if you don't explicitly define it, you can expect $model->id to be an int. If you remove it, then anything not explicitly defined, will suddenly have TypeErrors if you pass it along to a method that requires an integer.

[macropay-solutions]

@DanielCHood but it is coming from db int and on inserts it is already casted to int if it is numeric.

Update. Anyway the if can be removed independently by those who don't use casts. For our demo page we did just that.

[DanielCHood]

@macropay-solutions but it's also executing that casting on rows coming from select queries. If I remove that, my data is strings by default. Commenting out that line gives me this when I select a collection and print it via toArray().

array(1098) {
  [0]=>
  array(17) {
    ["id"]=>
    string(5) "36634"

[DanielCHood]

@thomas-0816 I'm curious if you could cherry pick #51617 and run your test again.

[macropay-solutions]

@macropay-solutions but it's also executing that casting on rows coming from select queries. If I remove that, my data is strings by default. Commenting out that line gives me this when I select a collection and print it via toArray().

array(1098) {
  [0]=>
  array(17) {
    ["id"]=>
    string(5) "36634"

Thank you for the reply. For us that does not happen. What db are you using?

@thomas-0816 then your cache solution is an option if it will ever be accepted.

[DanielCHood]

@macropay-solutions MySQL PDO, attributes are

                PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
                PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => true,
                PDO::ATTR_STRINGIFY_FETCHES => true,

[macropay-solutions]

@DanielCHood that explains it... Ok thank you.

[macropay-solutions]

@DanielCHood @thomas-0816
Do you think this will be faster without the need to cache the result of getCasts by the model FQN to not load the memony?

// model

    public function __construct(array $attributes = [])
    {
        if ($this->incrementing) {
            $this->casts[$this->getKeyName()] ??= $this->getKeyType();
        }

        $this->bootIfNotBooted();

        $this->initializeTraits();

        $this->syncOriginal();

        $this->fill($attributes);
    }


// Has Attributes


    public function getCasts()
    {
        return $this->casts;
    }

This would remove the need for cashing the casts by FQN.

Also it would be fully backward compatible.

Update.
We released this retroactive feature/fix in our free crud lib v 4.3.3

[thomas-0816]

@thomas-0816 I'm curious if you could cherry pick #51617 and run your test again.

I think castedAttributeCache can only work if the casting itself is deterministic. Clearing the cache with every set operation also costs some performance. So I'm not in favor of castedAttributeCache.

[DanielCHood]

That’s fair, I opted for it for safety (worrying about to places changing the property) whilst also trying to address more than just getCasts().

I’m curious about the performance between what was implemented is vs that PR but that PR also doesn’t matter so much since it got rejected without any guidance on how to improve it.