We should try to cache (Koha::Cache) some of the most common info that is used on many of our OPAC and intranet pages in order to prevent making very slow SQL queries. E.g. I can cut the load time of biblio detail.pl page by several seconds by just storing the Koha::Template::Plugin::Branches::GetName results in cache/RAM (when the bib has around 1000 items). Similar caching should be done for Koha::Template::Plugin::Categories::GetName, and so on.
Created attachment 124655 [details] [review] Bug 28436: Add local cache to Template Plugins for branches and itemtypes This patch simply stores the branches an itemtypes in a hash for lookup. This improves efficiency on pages where we are loading many items To test: 1 - Add ~1000 items to a record 2 - Note load time 3 - Apply patch 4 - Reload and note faster load 5 - Edit a branchname/itemtype description 6 - Reload page 7 - Confirm the updated name is reflected in display
We can definitely add more plugins here. This feels a bit hacky, however, it reduces to a single lookup per page, avoids problems with cache coherence and is readable
(In reply to Nick Clemens from comment #2) > We can definitely add more plugins here. > > This feels a bit hacky, however, it reduces to a single lookup per page, > avoids problems with cache coherence and is readable Actually, it looks like there might be a cache coherence problem, since the our-scoped variable will exist for the life of the process. You could wind up with stale data if data is cached, the worker stays alive, data is changed in the staff interface, and then requests are sent to that original worker. I'd suggest just caching at the request-level. I would suggest creating a new() method which sets an object variable with the required data. According to http://www.template-toolkit.org/docs/modules/Template/Plugin.html, the new() method will be called in the USE directive. This means that at template processing time you'll get 1 lookup, and then you can reuse that data for the life of that request. There are even cache examples on that TT link above.
It would be more robust and more readable. Or if we wanted to persist the cache beyond the request-level we could use Koha::Cache, and rely on the staff interface for Libraries and Item Types to invalidate the cache. That would be even better. That Koha::Cache could also be placed in the respective modules and have something like Koha::ItemTypes->get_list() or whatever and have the specific module handle it's own caching logic. Lots of options which would be less hacky and more robust.
But super keen to see this work go ahead. We do far too many unnecessary DB calls in Koha...
Since the patch is for template plugins, it's probably worth changing the title to reflect that the update is just for template plugins too
(In reply to David Cook from comment #4) > It would be more robust and more readable. > > Or if we wanted to persist the cache beyond the request-level we could use > Koha::Cache, and rely on the staff interface for Libraries and Item Types to > invalidate the cache. > > That would be even better. > > That Koha::Cache could also be placed in the respective modules and have > something like Koha::ItemTypes->get_list() or whatever and have the specific > module handle it's own caching logic. > > Lots of options which would be less hacky and more robust. Indeed. I think we should use Memcached for those datas, they are not often changed.
Duplicate of Bug 29454 no ?
(In reply to Fridolin Somers from comment #8) > Duplicate of Bug 29454 no ? Yeah I'm not sure why that was opened as a new one..
(In reply to David Cook from comment #9) > (In reply to Fridolin Somers from comment #8) > > Duplicate of Bug 29454 no ? > > Yeah I'm not sure why that was opened as a new one.. Joonas opened this one with broader context, and comments indicated that memcached would be a good way to go. I moved mine to a new bug as it deals narrowly with 2 plugins and doesn't use the Koha cache, I could have changed the title and reopened a new bug, but moving my patches seemed cleaner