Performance issue in cache plugin getRowId for Search API views with disabled cache

I'm not sure if other search api backends recommend disabling views caching - perhaps this is more specific to the Search API solr module issue queue?

Changed the cache plugin from none to search_api_none in the none display used in the ViewsDisplayCachingTest test - passes locally but CI seems to have unrelated issue.

We probably then want an update hook to convert displays from none to search_api_none? Will wait for maintainer feedback before doing any more here.

For context, we picked this up as an change in metatag module means the meta tags are now a computed field so the meta tag values are generated when the node is serialized, on a display with many search record rows enabled that was enough to notice a performance regression.

drunken monkey → made their first commit to this issue’s fork.

Thanks for reporting this issue! Pretty bad if a cache plugin that is supposed to do nothing actively hurts performance. Your solution seems sensible, so thanks for that! I made a few updates, including an update hook and generally the same treatment for this new cache plugin as we introduced for the existing ones back in #2809469: Set search api caching as default for new search api views → . Please test/review!

Disabling Views caching is definitely a relevant use case for other backends, too. In general, it can be tricky to achieve a reasonable cache hit ratio for searches. Plus, other backends have an indexing delay as well, leading to the same problem with caching as when using Solr.

Oh, and you’ll be glad to hear that MR testing should now work correctly in this module, so I expect the tests to pass now.

Thank you Thomas!

Tested the additional changes and it looks good - the update hook ran as expected and converted my views 👍

Only noticed one side effect from the change / fix from https://git.drupalcode.org/project/search_api/-/merge_requests/109/diffs...

Previously when creating a new view the default cache value is tag coming from views - and was being changed to none by that line above. Now this is changed to search_api_tag - which seems to be a sensible thing to expect - but is a change in behavior so just noting that.

But this also is why the ViewsQueryTypeTest test is failing - it uses config that uses the cache type tag and expects it to be saved as none. I assume the test should expect search_api_tag - just wanted to double check if there was any reason the conversion logic was previously to set to none.

Thanks for pointing this out!
The change by me was indeed on purpose, but now that I think about it again I’m not sure anymore. It seems logical, of course, to “auto-correct”, e.g., the incompatible tag-based cache plugin to the compatible one.
On the other hand, defaulting to “None” previously was of course also on purpose, since this is still the “safest” plugin for Search API views, where the intricacies of searching with an external server can often mess up proper caching. So, if someone manages to select an incompatible plugin (which shouldn’t even be possible, really, in the UI), it seems safer to just set the view to have no caching so the user explicitly needs to select the correct option and maybe is motivated to look a bit closer into caching of searches.

Hard to decide, but in the end it seems much safer to keep the behavior we had before (for over seven years, it seems?) and only think about adapting it in a new issue, if it bothers enough people and we can reach a clear vote on changing the behavior.
I adapted the code accordingly.

Would you say this is ready to be merged now?

Thanks, that makes sense and I agree with you that keeping the existing behavior is safest.

And yes - this looks good to merge to me, Thank you!

Good to hear, thanks for reporting back.
Merged.
Thanks again!

Automatically closed - issue fixed for 2 weeks with no activity.

The post update hook committed here is bugged, see 🐛 search_api_post_update_views_cache_none() replaces all cache types with "search_api_none" RTBC - this should probably be resolved asap with a new minor.