Store/Get items in/from Blink instead of always calling from Cache.

[Daynnnnn]

trafficstars

Sometimes Statamic will make duplicate calls to the getItem method in Statamic\Stache\Stores\BasicStore. While using the cache will alleviate some of the slow down caused by these duplicate calls, utilising blink will make these calls much faster.

Without Blink:

With Blink:

[Daynnnnn]

Happy for this to be reviewed now, let me know if you want anything changing.

[michaelr0]

Thanks @Daynnnnn I had also just come across this issue today and logged it as https://github.com/statamic/cms/issues/3991 - I've closed that in favor of moving the discussion into your PR

On a fresh install of Statamic and multiple refreshes to get the cache populated the results are as follows.

Before:

8 x hit stache::items::collections::pages (0%) | 0μs
3 x hit stache::items::entries::pages::home (0%) | 0μs
5 x hit stache::items::collection-trees::pages::default (0%) | 0μs

After applying your changes:

1 x hit stache::items::collections::pages (0%) | 0μs
1 x hit stache::items::entries::pages::home (0%) | 0μs
1 x hit stache::items::collection-trees::pages::default (0%) | 0μs

[michaelr0]

Might be worth changing this

    protected function cacheItem($item)
    {
        $key = $this->getItemKey($item);

        $cacheKey = $this->getItemCacheKey($key);

        Blink::forget($cacheKey);
        Cache::forever($cacheKey, $item);
    }

To be this

    protected function cacheItem($item)
    {
        $key = $this->getItemKey($item);

        $cacheKey = $this->getItemCacheKey($key);

        Blink::put($cacheKey, $item);
        Cache::forever($cacheKey, $item);
    }

So that this, pushes the item to the Blink cache if it has to load it from the file?

    public function getItem($key)
    {
        $this->handleFileChanges();

        if (! $path = $this->getPath($key)) {
            return;
        }

        if ($item = $this->getCachedItem($key)) {
            return $item;
        }

        $item = $this->makeItemFromFile($path, File::get($path));

        $this->cacheItem($item);

        return $item;
    }

As that will stop these calls (on a new statamic install and a cleared stache)

1 x hit stache::items::collections::pages (0%) | 0μs
1 x hit stache::items::entries::pages::home (0%) | 0μs
1 x hit stache::items::collection-trees::pages::default (0%) | 0μs

[michaelr0]

@Daynnnnn Looks like you figured out the tests? Great stuff!

[Daynnnnn]

Some words on the changes I've made, let me know if you think any of this looks wrong:

• GraphQL tests - Switch to withSomeOfArgs and remove entry expectation, as the entry will be a cloned instance.
• BuilderTest - Set selectedQueryColumns on $searchedEntry. Due to cloning, the field being set on $retrievedEntry no longer affects $searchedEntry. Even though this isn’t the expected behaviour, it’s the easiest way to keep the test working.
• MountingTest - Save collection before also saving entries so URI is updated on entries in findByUri call.
• UpdateTaxonomyTest - I’ve added comments, but to summarise, we forget blinked taxonomies on entryOne as the taxonomy has changed, but the list hasn’t, so the cached blink response is stale. Reload taxonomies 2 and 3 as the taxonomy list has changed since they were originally created.
• UserGroupTest, AugmentedTestCase, FeatureTest, EntryRepositoryTest - swap from assertSame to assertEquals, as we’re comparing clones
• EntryTest - Save entries/collections after changes are made, and reload them before comparisons

[Daynnnnn]

Hey 👋 Don't suppose there's anything I can do to help push this along? This is contributing to some serious performance issues in a couple production sites atm, which is worrying the rest of the team when starting new projects on V3 :(

[jasonvarga]

This will probably be merged after 3.2 is out.

[duncanmcclean]

In the meantime, you may be able to pull in the fix using composer patches?

[HRR1337]

This will probably be merged after 3.2 is out.

Any update regarding merging this? :-)

[edalzell]

This will probably be merged after 3.2 is out.

Any update regarding merging this? :-)

Use a composer patch to pull this in if you need it.

[isarphilipp]

Thanks @jasonvarga for putting me here after I filed a similar PR with a similar solution because we're hitting the same problems like @Daynnnnn . Is there anything we can contribute here to make it reality?

We're seeing -30% response times on a heavy site with this change, so that's a real benefit :)

[jasonvarga]

I'm thinking this may introduce more of the same bug being encountered in #2993. If I'm understanding what's happening in there correctly (I may not be!) - there's state being set that hangs around in the queue's process. When another process updates the cache, the first process still has old state.

[Daynnnnn]

I'd need to have a test, but I believe you're right. If I'm also understanding the issue correctly, I guess we could make sure the Stache blink is empty when we initialize the bits in Statamic\Stache\ServiceProvider?

[jasonvarga]

I think the service provider only runs once per process, in Horizon. I could be wrong about that. But yes if there was something we could use to trigger a blink clear - I think that'd probably do it.

[Daynnnnn]

Hey @jasonvarga, @Zakini has suggested using the Queue::before() method here in one of the service providers (probably Stache) to clear the blink cache before each JobProcessing event, which should ensure a clean Blink before each job. Do you think this'll be alright?

[jasonvarga]

It could be! I didn't know about that method. It's likely very helpful.

[Daynnnnn]

I can confirm using Queue::before() with Blink::flush() resolves this issue locally!

[jasonvarga]

How are you testing that?

[Daynnnnn]

Attached a video below. But in words:

• Created a standard statamic install using composer create-project --prefer-dist statamic/statamic statamic-test, and applied this PR to it.
• Set the queue driver to redis
• Created a job which echos the content field.
• Created a command to dispatch the job.
• Started the worker
• Ran the test job, changed the content via the CMS, and ran the test job again

With the new changes, the second job outputs the expected results. While before, the second job would output the old content.

(if the video is low quality when you view, you should be able to download it, it takes a while for drive/youtube to process it) https://drive.google.com/file/d/1cf0mu5-F7MbaF2AQkuqwWu9poz4P_5bp/view

[Daynnnnn]

My last commit, Clear taxonomy blink on save, is in place as there seems to be an issue with the cloning in BasicStore and the blinks on collections/taxonomies in entries/terms. Imo, those blink caches should now be removed - but this will have a performance hit on the eloquent driver, so I've left in for now. If you'd like me to go ahead and remove them, let me know.

[FrittenKeeZ]

This is very promising in terms of performance! I'd love to this being merged soon :)

[michaelr0]

Just checking back in on this, it appears to still be pending? Anything we/others can do to help this along?

[MrRio]

Any word on this?

[edalzell]

Any word on this?

Yes, the word is that it's not merged yet. If it's still open, the core team hasn't gotten around to it.

In the meantime you can composer patch it in, if you'd like to use it.

[Daynnnnn]

Anything extra I can add to give us more confidence with this?

[o1y]

This PR is related to Issue #7424.

While testing this patch it turned out, that it's not working as intended. It seems that responses are much faster, but in some situations Stache returns wrong cached data which makes the returned data in CP or in frontend unreliable. I would really appreciate it, if someone here with more experience on the Stache Driver can check this PR again, as it can be improving overall performance up to 50% 🚀

Is it maybe a better approach avoiding the duplicate calls instead of changing the way how they are being cached?

[Daynnnnn]

@o1y You got any steps to reproduce this? We've been running this PR on ~10 live sites for over a year now, and we haven't hit any issues with it so far

[edalzell]

@Daynnnnn there is a merge conflict here, you wanna rebase on 3.4 and resolve?

[Daynnnnn]

Rebased and resolved conflicts, could still do with some steps to reproduce any issues though

[xuneXTW]

@Daynnnnn We recently ran into issues with multisite enabled. There where some weird behaviours when you filled content in one site and switched to an other site to fill in contents for this site on a new entry. After removing the patch it worked again. Maybe you could check if there are some issues with switching sites on new entries.