statamic / eloquent-driver

Provides support for storing your Statamic data in a database, rather than flat files.
https://statamic.dev/tips/storing-content-in-a-database
MIT License
104 stars 72 forks source link

Remove redundant cache #207

Closed SylvesterDamgaard closed 8 months ago

SylvesterDamgaard commented 10 months ago

Fixes https://github.com/statamic/eloquent-driver/issues/206

ryanmitchell commented 10 months ago

Thanks for this. I do understand that for your use case (importing) the extra cache is redundant, but for normal day to day use the extra cache does result in performance improvements which is why it was added.

I'm going to sit on this for a bit and see if I can come up with a way that solves both problems.

SylvesterDamgaard commented 10 months ago

@ryanmitchell, I completely understand your concerns. However, I genuinely feel that using an immutable data strategy for dynamic data is inherently flawed, even though it's only applied on a per-process or per-request basis. I initially discovered this issue due to performance bottlenecks, as missing metadata on images can lead to significantly slower page loads or even timeouts.

As for workarounds, using php please asset:meta is also quite slow, and this becomes increasingly problematic as the asset count grows into the thousands. Clearing the cache, as per my example, is another option but it's not ideal.

Before submitting this PR, I conducted performance tests using a real dataset of over 40,000 images. My tests showed no noticeable changes in performance since the data is already being delivered from the cache. Do you have any test results that indicate otherwise?

I hope this clarifies my position and I'm open to further discussions to find the best solution.

ryanmitchell commented 10 months ago

Yes, we have a similar number of assets on our live sites using this add-on. As I said, we noticed verifiable speed improvements which is why this was added. However there have been changes in statamic/cms that may have duplicated some of this work. I need sufficient time to investigate so it doesnt cause a regression - you can always bind your own Asset class to get around this.

SylvesterDamgaard commented 10 months ago

@ryanmitchell Got it. Thank you for the clarification.

A custom bind seems to solve this issue the right way.

<?php

namespace App\Assets;
class Asset extends \Statamic\Eloquent\Assets\Asset
{
    public function exists()
    {
        if (! $path = $this->path()) {
            return false;
        }

        return $this->container()->files()->contains($path);
    }
}