Prevent n+1 queries on asset meta exists #446
Conversation
src/Assets/Asset.php
Outdated
| ->container($model->container) | ||
| ->path(Str::replace('//', '/', $model->folder.'/'.$model->basename)) | ||
| ->hydrateMeta($model->meta) | ||
| ->syncOriginal(); | ||
|
|
||
| Blink::put('asset-meta-'.$asset->id(), $model->meta); |
There was a problem hiding this comment.
| Blink::put('asset-meta-'.$asset->id(), $model->meta); | |
| Blink::put('eloquent-asset-'.$asset->id(), $model->meta); |
It works with the above change (you used a different key in metaExists).
You could also use Blink::put('eloquent-asset-meta-exists-'.$asset->id(), true) and remove the additional Blink::has('eloquent-asset-'.$this->id()) in metaExists
There was a problem hiding this comment.
Thanks - not quite but I've changed it around some more.
We need to cache both the model and the meta (core caches the meta so we need to do the same)
There was a problem hiding this comment.
| Blink::put('asset-meta-'.$asset->id(), $model->meta); | |
| Blink::put($asset->metaCacheKey(), $model->meta); |
metaCacheKey is a public method of Assets, so you could use this in a unified way:
https://github.com/statamic/cms/blob/eba307923eb5fe9ce7b0954bf6ceecc0357be8c1/src/Assets/Asset.php#L324-L327
There was a problem hiding this comment.
Yes thanks. Can you confirm you are still seeing a reduction in queries with the latest changes?
There was a problem hiding this comment.
Yeah, the reduction still works with the latest changes.
|
Moving my comment here: Something in this change breaks sites that are running custom models for Assets @ryanmitchell - when I get more time I can try to figure out what. We got:
In the meantime, we're downgrading to 4.23.4 - if I can figure out what breaks here I'll create issue or PR. |
|
@macaws that’s surprising - nothing here seems like it should break that. If you are able to share your repo I can take a look into it. Or if you are able to provide a test that shows the breakage so I can understand the issue then I can investigate further. |
Closes #445