New Custom Casting For Eloquent Models Has Excessive set() calls

[ralphschindler]

• Laravel Version: 7.x
• PHP Version: 7.4.x
• Database Driver & Version:
  n/a

Description:

When attempting to write a CustomCast that will convert between a complex Object and the serialization for the database, CastAttributes::set() is called an excessive amount of times. This could be problematic (and unnecessary) if the work to serialize is non-trivial (like converting points into a binary representation.)

This is due to excessive calls to HasAttributes::mergeAttributesFromClassCasts() that originate from multiple places in both Model::getAttributes() and Model::save() calls:

• from save() itself: https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Model.php#L670
• then from save() calling performInsert() calling getAttributes():
  https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Model.php#L827
• then from save() calling finishSave() calling isDirty() calling getDirty() calling getAttributes(): https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L1326
• then from save() calling finishSave() calling syncOriginal() calling getAttributes(): https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L1205

There is probably more to unpack here, and perhaps custom attributes should only be doing lightweight transformation, but it feels like this could be a good tool if Eloquent could manage to only transform the values to a savable value once before insert/update. Should we continue this discussion?

Steps To Reproduce:

7.x...ralphschindler:custom-cast-excessive-calls
(branch now based off master: master...ralphschindler:custom-cast-excessive-calls)

[driesvints]

Going to leave this to Taylor to review

[taylorotwell]

Yeah, probably worth exploring if we can get rid of some of those redundant calls for sure.

[driesvints]

Even though nothing's broken, I'm gonna mark this as a bug so we can take a look at this later on. Appreciating prs in the meantime!

[gentcys]

Suppose we set the attributes of Eloquent Model through the way specified in the test:

$model->address = $address = new Address('110 Kingsbrook St.', 'My Childhood House');
$address->lineOne = '117 Spencer St.';

The mergeAttributesFromClassCasts method guarantees that every castable attributes set by the way above will be cast before save.

What if we add a property like $originalClassCastCache(named it just an example) as a comparison to $classCastCache, each element in latter property is a deep copy of element in former property.

$classCastCache['address'] = unserialize(serialize($originalClassCastCache['address']));

And the mergeAttributesFromClassCasts method make a comparison of each element referenced by the same key(like 'address' => ). If it's different then do the merge action.

protected function mergeAttributesFromClassCasts()
{
    foreach ($this->classCastCache as $key => $value) {
        if ($this->classCastCache[$key] == $this->originalClassCastCache[$key] ) {
            continue;
        }
        
        $caster = $this->resolveCasterClass($key);

        $this->attributes = array_merge(
            $this->attributes,
            $caster instanceof CastsInboundAttributes
                   ? [$key => $value]
                   : $this->normalizeCastClassResponse($key, $caster->set($this, $key, $value, $this->attributes))
        );
    }
}

@driesvints @taylorotwell

[taylorotwell]

@ralphschindler I made a fairly simple change that had a yuge reduction in set calls:

3752aa8

[ralphschindler]

Having a look this weekend, thanks!

[ralphschindler]

Unfortunately doesn't reduce the calls (the test I have in my branch still sees 5 when expecting 1. I will dig more into this.

[linaspasv]

@ralphschindler the main cause of this is that there is no proper cache for casted properties. Actually, this is the problem for many years, even before custom casts were introduced, just somehow no one noticed or cared about this, e.g. if you had 'array' cast in your models it kept doing json_decode each time you tried to access that property.

Anyway, I have also tested the latest changes and I did not notice any reduction of set calls for the following case...

class Model extends Eloquent
{
    protected $table = 'models';

    protected $casts = [
        'options' => CustomCast::class
    ];

    .....
}

$model = Model::find(1);

$model->options->get('value');   // <-- get called
$model->options->get('value');   // <-- set called
$model->options->get('value');   // <-- set called
$model->options->get('value');   // <-- set called

$model->save();     // <-- for some reason set is executed 4 times here

If you have a bit more complex logic in your set method, this could quickly become very ugly... when you do expect a single call on save but you receive N hits...

[linaspasv]

I did some further investigation on this. There are two main problems with current architecture:

1. There is no valid cache for already cached casts and attribute is being casted each time someone tries to access it. This could be easly solved by resolving cast once per model life time only.

2. Model::save() has an excessive calls to getDirty method. I have counted that a single Model::save() makes atleast 3 getDirty method calls. How does getDirty work is that it keeps looping through every attribute (including casts) and checks if these have changed (also generating an additional set calls). Basically, we end up with 3 loops for N (attributes) and this does not sound very efficient as there is no particular reason doing this.

[netpok]

I thought about this, the problem with the first point is that most objects are not immutable so it would make it behave like a singleton.

Example:

// no cast
$var = $model->number; // = 2
$var = 5;
echo $model->number; // 2

// custom cast (for example a carbon object)
$var = $model->custom; // = 2020-06-01
$var->addDays(7);
echo $model->custom; // 2020-06-08 instead of 2020-06-01

[linaspasv]

Yeh, I do understand your point. There is no way to tell when someone wants to make a copy or just access property / method of the object. That's why clone keyword exists in PHP.

Anyway, I see that there is a classCastCache variable that probably tries to do this for custom casts already?

[netpok]

I would say that would be a HUGE breaking change, I thin it would affect a substantial part of the userbase.

Also clone has it's limitation: https://github.com/myclabs/DeepCopy#why, if the cycles aren't a problem the target class still needs to implement __clone if has other objects

[linaspasv]

Yes, you are right. There is no need to cache native casts, and it seems there is $classCastCache that was used to cache custom cast objects by design, so I did not change or added anything regards this but corrected some parts.

[linaspasv]

@taylorotwell @ralphschindler I have done some work on the issue regards Excessive ::set() calls. My change should result in a single ::get() (on first access) and single ::set() call (on save).

Would you mind to check / test this out - linaspasv/framework@06f189123bb17762d9473697201a1c831df77fac (updated - branch is based on master) ?

I would love to get some feedback before PR submission. Thank you.

My setup:

$model = Model::find(1);

$model->options->get(10); // <--- ::get() is executed
// before the update used to execute ::set()
$model->options->get(20); // <--- reading from $classCastCache
// before the update used to execute ::set()
$model->options->put('test', 30); // <---- updating object at $classCastCache

// before the update:
//  - 7 ::set() calls when custom cast value has been changed
//  - 4 ::set() calls when custom cast value has not been changed

$model->save();  // <--- single set() is executed

class Model extends Eloquent
{
    protected $table = 'models';

    protected $casts = [
        'options' => CollectionCast::class
    ];
}

class CollectionCast implements CastsAttributes
{
    public function get($model, string $key, $value, array $attributes)
    {
        return new Collection(json_decode($value, true));
    }
    
    public function set($model, string $key, $value, array $attributes)
    {
        if ($value instanceof Collection) {
            $value = $value->toArray();
        }

        return json_encode($value);
    }
}

[netpok]

I think you left out part of an if statement, I left a comment for it.

[linaspasv]

I have also created a small CustomCastsOptimizer trait for testing purposes, see gist at ad74e7077d212d3f6b79b8e51118267b .

I did a quick benchmark between Eloquent model with and without optimizer trait on my server:

1. 30-50% speed increase when saving with a changing custom cast value (on every iteration)
   un-optimized (27.93ms)
optimized (11.73ms)

2. 3-6 times speed increase when saving with a constant custom cast value
   un-optimized (8.28ms)
optimized (1.88ms)

p.s. this should also benefit models without custom casts too... mainly because (unnecessary) isDirty checks were greatly reduced in a ::save() method.

note: test performed on PHP 7.4.5 (opcache disabled), Laravel 7.12.0

Custom cast:

class CollectionCast implements CastsAttributes
{
    public function get($model, string $key, $value, array $attributes)
    {
        return new Collection(json_decode($value, true));
    }
    
    public function set($model, string $key, $value, array $attributes)
    {
        if ($value instanceof Collection) {
            $value = $value->toArray();
        }

        return json_encode($value);
    }
}

Models:

class Model extends Eloquent
{
    protected $table = 'models';

    protected $casts = [
        'options' => CollectionCast::class
    ];
}

class ModelOptimized extends Eloquent
{
    use CustomCastsOptimizer;

    protected $table = 'models';

    protected $casts = [
        'options' => CollectionCast::class
    ];
}

Benchmark:

Debugbar::measure('un-optimized', function () {
    $model = Model::find(1);

    for ($i = 0; $i < 10; $i++) {
        $model->options->get(10);
        $model->options->get(20);
        $model->options->put('test', 100);
        // $model->options->put('test', $i);   // test for changing custom cast value

        $model->save();
    }
});

Debugbar::measure('optimized', function () {
    $model = ModelOptimized::find(1);

    for ($i = 0; $i < 10; $i++) {
        $model->options->get(10);
        $model->options->get(20);
        $model->options->put('test', 100);
        // $model->options->put('test', $i);  // test for changing custom cast value

        $model->save();
    }
});

[ralphschindler]

Hi @linaspasv (just dropping a quick note, I will dig in more later today). I am less concerned with optimization in terms of speed, but optimization in terms of the actual number of calls (which would in turn affect speed when custom casting may be expensive, as it is in my case).

If you look at my attached branch (above), I have a good example test case that tracks calls:

master...ralphschindler:custom-cast-excessive-calls#diff-b50af0acefc58e2c0d89569159ab38d5R179-R196

I also notice there are two different use cases that Laravel is exposing: 1) pure custom casting, but also 2) aggregate casting (for lack of a better term) where multiple columns are exposed as a single cast object.

It is possible a fix can move forward without breaking changes too much, if we add or separate these use cases with a new Interface in addition to the existing one.

More later after I've had some time to explore your code and discussions above.

[linaspasv]

@ralphschindler thank you for your response. I will check on your test cases.

The main goal was to reduce an excessive ::set calls to the very minimum amount. The speed increase came as a nice bonus.

There are still problems to be solved but might be a good starting point.

I am looking forward to your feedback!

[linaspasv]

Hi @ralphschindler, I have included your tests and did some minor changes. It seems eloquent tests still pass including your tests.

linaspasv/framework@06f189123bb17762d9473697201a1c831df77fac

Also there are two ways to solve this:

1. Limit to a single call to ::isDirty() method per normal fetch save cycle. (currently using this approach)
2. Pre-cache dirty attributes when they change instead of looping and checking on each ::isDirty() method call.

[gentcys]

Maybe my thought in my previous comment isn't good solution.
The key of the problem is getAttributes() calling mergeAttributesFromClassCasts() which is used to ensure that custom class attributes be updated through the way specified in #31778 (comment), though a set method(mergeAttributesFromClassCasts()) should not be called in a get method(getAttributes()).

[Broutard]

Agree with that.
getAttributes() should not call $caster->set() method.

This can produce some huge performance issue.

[maiconmva]

Same issue for me!! for list 200 records, call cast set 10k times!!!

[Lumrenion]

I stumbled across exactly this problem. I have an Eloquent model with many JSON columns in the database and each one of them has a classCast. I did a rough microtime measure. Before filling the classCastCache, calling $model->getKey() 1000 times takes 0.021195888519287 seconds. Afterwards it takes 2.5050020217896 seconds. All of my 20 attributes are cast back and forth for no reason at all. I would call that an unwanted sideeffect when getting an attribute.

[sergotail]

I encountered this problem when trying to use separate classes to write and read some attribute. Reader class cannot be serialized with jsonSerialize() and Builder class cannot be retrieved from the database with fromArray().

And exception is thrown when i tried to access model attribute: $serviceTask->result calls set() in my cast class, which calls jsonSerialize() on Reader, which throws an exception.
I solved this problem with ServiceResultReaderInterface and ServiceResultBuilderInterface interfaces. Reader class implements ServiceResultReaderInterface and Buider class implements ServiceResultBuilderInterface.
And in my cast set() method i just check if $value is instance of ServiceResultReaderInterface, and if so, i just return an empty array.

Maybe this will help someone.

My custom cast class looks like:

<?php

namespace App\Casts;

use Illuminate\Contracts\Database\Eloquent\CastsAttributes;

use App\Exceptions\ServiceResultCastException;
use App\Tools\Services\Contracts\ServiceResultReaderInterface;
use App\Tools\Services\MyService\ServiceResultReader;

class ServiceResult implements CastsAttributes
{
    public function get($model, $key, $value, $attributes)
    {
        return ServiceResultReader::fromArray(json_decode($value, true));
    }

    public function set($model, $key, $value, $attributes)
    {
        if ($value instanceof ServiceResultReaderInterface) {
            return [];
        }

        $result = json_encode($value);

        if ($result === false) {
            throw new ServiceResultCastException(
                "Service result serialization failed"
            );
        }

        return $result;
    }
}

[novack96]

Hi @ralphschindler !

Is there any update on this issue? I also encountered your issue while I was creating a custom cast to simplify file uploads.
The uploaded file is deleted every time I call the save() method on the model. 😄
@linaspasv Is there any PR open for your optimized trait to be merged into the Eloquent Model?

Thank you for your contribution! 👍

Best regards,
Attila

[linaspasv]

Hi @novack96 , I never did open a PR for this but using a trait that solves all the problems for the very long time.

<?php

namespace App\Foundation\Database\Eloquent\Traits;

use Illuminate\Contracts\Database\Eloquent\CastsInboundAttributes;
use Illuminate\Database\Eloquent\Builder;

trait CustomCastsOptimizer
{
    /**
     * Determine if the model or any of the given attribute(s) have been modified.
     *
     * @param  array|string|null  $attributes
     * @return bool
     */
    public function isDirty($attributes = null)
    {
        $attributes = is_array($attributes) ? $attributes : func_get_args();

        foreach ($attributes ?: array_keys($this->attributes) as $key) {
            if (! array_key_exists($key, $this->attributes)) {
                continue;
            }

            $this->mergeAttributeFromClassCasts($key);

            if (! $this->originalIsEquivalent($key)) {
                return true;
            }
        }

        return false;
    }

    /**
     * Get an attribute from the model.
     *
     * @param  string  $key
     * @return mixed
     */
    public function getAttribute($key)
    {
        if (! $key) {
            return;
        }

        if (isset($this->classCastCache[$key])) {
            return $this->classCastCache[$key];
        }

        return parent::getAttribute($key);
    }

    /**
     * Get an attribute from the $attributes array.
     *
     * @param  string  $key
     * @return mixed
     */
    protected function getAttributeFromArray($key)
    {
        $this->mergeAttributeFromClassCasts($key);

        return $this->attributes[$key] ?? null;
    }

    /**
     * Sync multiple original attribute with their current values.
     *
     * @param  array|string  $attributes
     * @return $this
     */
    public function syncOriginalAttributes($attributes)
    {
        $attributes = is_array($attributes) ? $attributes : func_get_args();

        foreach ($attributes as $attribute) {
            $this->original[$attribute] = $this->getAttributeFromArray($attribute);
        }

        return $this;
    }

    /**
     * Merge a single cast class attribute back into the model.
     *
     * @param  string  $key
     * @return void
     */
    protected function mergeAttributeFromClassCasts($key)
    {
        if (! isset($this->classCastCache[$key])) {
            return;
        }

        $value = $this->classCastCache[$key];

        $caster = $this->resolveCasterClass($key);

        $this->attributes = array_merge(
            $this->attributes,
            $caster instanceof CastsInboundAttributes
                ? [$key => $value]
                : $this->normalizeCastClassResponse($key, $caster->set($this, $key, $value, $this->attributes))
        );
    }

    /**
     * Merge the cast class attributes back into the model.
     *
     * @return void
     */
    protected function mergeAttributesFromClassCasts()
    {
        foreach ($this->classCastCache as $key => $value) {
            $this->mergeAttributeFromClassCasts($key);
        }
    }

    /**
     * Perform a model update operation.
     *
     * @param  \Illuminate\Database\Eloquent\Builder  $query
     * @return bool
     */
    protected function performUpdate(Builder $query)
    {
        // Once we have run the update operation, we will fire the "updated" event for
        // this model instance. This will allow developers to hook into these after
        // models are updated, giving them a chance to do any special processing.
        $changes = $this->getDirty();

        if ($changes) {
            // If the updating event returns false, we will cancel the update operation so
            // developers can hook Validation systems into their models and cancel this
            // operation if the model does not pass validation. Otherwise, we update.
            if ($this->fireModelEvent('updating') === false) {
                return false;
            }

            // First we need to create a fresh query instance and touch the creation and
            // update timestamp on the model which are maintained by us for developer
            // convenience. Then we will just continue saving the model instances.
            if ($this->usesTimestamps()) {
                $this->updateTimestamps();
            }

            $this->setKeysForSaveQuery($query)->update($changes);

            $this->changes = $changes;

            $this->fireModelEvent('updated', false);
        }

        return true;
    }

    /**
     * Save the model to the database.
     *
     * @param  array  $options
     * @return bool
     */
    public function save(array $options = [])
    {
        $query = $this->newModelQuery();

        // If the "saving" event returns false we'll bail out of the save and return
        // false, indicating that the save failed. This provides a chance for any
        // listeners to cancel save operations if validations fail or whatever.
        if ($this->fireModelEvent('saving') === false) {
            return false;
        }

        // If the model already exists in the database we can just update our record
        // that is already in this database using the current IDs in this "where"
        // clause to only update this model. Otherwise, we'll just insert them.
        if ($this->exists) {
            $saved = $this->performUpdate($query);
        }

        // If the model is brand new, we'll insert it into our database and set the
        // ID attribute on the model to the value of the newly inserted row's ID
        // which is typically an auto-increment value managed by the database.
        else {
            $saved = $this->performInsert($query);

            if (! $this->getConnectionName() &&
                $connection = $query->getConnection()) {
                $this->setConnection($connection->getName());
            }
        }

        // If the model is successfully saved, we need to do a few more things once
        // that is done. We will call the "saved" method here to run any actions
        // we need to happen after a model gets successfully saved right here.
        if ($saved) {
            $this->finishSave($options);
        }

        return $saved;
    }

    /**
     * Perform any actions that are necessary after the model is saved.
     *
     * @param  array  $options
     * @return void
     */
    protected function finishSave(array $options)
    {
        $this->fireModelEvent('saved', false);

        $wasChanged = ($this->wasRecentlyCreated && empty($this->original)) || $this->changes;

        if ($wasChanged && ($options['touch'] ?? true)) {
            $this->touchOwners();
        }

        $this->original = $this->attributes;
    }
}

so I just include this in my Model class that extends original Model class, and use it for my models. Works perfectly for many years already. :-)

abstract class Model extends BaseModel
{
    use CustomCastsOptimizer;
    
    ```

[novack96]

Thank you very much! :-D Yeah it's working for me as well :-)

[linaspasv]

Happy it solved your problems! Might offer this as a PR in the future but Tailor is very strict with what he accepts into a core, especially for Eloquent. :-)

[vibonacci]

Any chance we can get a solution to this in the main framework? I was rather confused and surprised why set was called so many times for my custom Caster that implements CastsAttributes.

[Sophist-UK]

@linaspasv said:

Might offer this as a PR in the future but Tailor is very strict with what he accepts into a core, especially for Eloquent. :-)

Yes - Taylor is understandably strict about accepting PRs which have any risk of creating backwards compatibility issues i.e. a breaking change within a Laravel version - breaking changes are reserved for new Laravel versions.

But if these changes are straight forward and fix an obvious bug with no side effects, then he will accept it.

I think the issue would come if you had a setter which did not produce the same result each time (as an example, say a counter that incremented every time there was a change) and where your code is now expecting the result from having been called multiple times. Yes, IMO this is an edge case rather than a common coding paradigm, nevertheless it might create a backwards compatibility issue.

It is probably NOT too late to submit this as a (potentially) breaking change for Laravel 11 which is due out in a few weeks, especially if there is a pedigree of the trait working without issues for some time.

[Sophist-UK]

Also I note that early on both Taylor and Dries have said that they consider this to be a bug and have invited PRs!! So go for it!!