We are using Search API + Search API solr and as per the Search API solr readme have views caching disabled.
When the view is rendered the style base plugin (from core) adds cache metadata to each row. As the max age returned from the plugin is 0 - this is never saved to the render cache. However as the metadata is still added to the render array there is an unnecessarily expensive performance hit when the row id is generated as part of the cache key. This is better explained in #3026526: SearchApiTagCache::getRowId is very expensive - but essentially the same thing is happening:
In CachePluginBase::getRowId all entity objects in 'index', '_entity', '_relationship_entities' are removed, because serializing objects is expensive.
For search_api the objects are not stored in those keys. Search api has its own keys. '_item', '_object', '_relationship_objects' and 'entity:node/_object'.
That means serializing a search api row result includes all the entity objects.
Introduce a search_api_none cache plugin - extend core's none plugin and override getRowId with the solution that was implemented for #3026526: SearchApiTagCache::getRowId is very expensive
/**
* {@inheritdoc}
*/
public function getRowId(ResultRow $row) {
return $row->search_api_id;
}
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
ericgsmith commentedI'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?
Comment #4
ericgsmith commentedChanged the cache plugin from
nonetosearch_api_nonein the none display used in theViewsDisplayCachingTesttest - 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 that was enough to notice a performance regression.
Comment #5
ericgsmith commentedComment #7
drunken monkeyThanks 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.
Comment #8
drunken monkeyOh, 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.
Comment #9
ericgsmith commentedThank 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
tagcoming from views - and was being changed tononeby that line above. Now this is changed tosearch_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
ViewsQueryTypeTesttest is failing - it uses config that uses the cache typetagand expects it to be saved asnone. I assume the test should expectsearch_api_tag- just wanted to double check if there was any reason the conversion logic was previously to set tonone.*edit* - I pushed a commit to change the test - thinking its easier to revert that change if its unwanted
Comment #10
drunken monkeyThanks 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?
Comment #11
ericgsmith commentedThanks, 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!
Comment #13
drunken monkeyGood to hear, thanks for reporting back.
Merged.
Thanks again!
Comment #15
acbramley commentedThe post update hook committed here is bugged, see #3437379: search_api_post_update_views_cache_none() replaces all cache types with "search_api_none" - this should probably be resolved asap with a new minor.