Summary: | Cache relatively static info that is used often and on many pages (instead of fetching from DB) | ||
---|---|---|---|
Product: | Koha | Reporter: | Joonas Kylmälä <joonas.kylmala> |
Component: | Architecture, internals, and plumbing | Assignee: | Bugs List <koha-bugs> |
Status: | RESOLVED MOVED | QA Contact: | Testopia <testopia> |
Severity: | minor | ||
Priority: | P5 - low | CC: | dcook, fridolin.somers, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29454 | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Attachments: | Bug 28436: Add local cache to Template Plugins for branches and itemtypes |
Description
Joonas Kylmälä
2021-05-24 12:52:40 UTC
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. (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 This was probably meant to be an omnibus bug, but as there are no bugs linked, I am inclined to close this as RESOLVED MOVED for now (indicating that it's been worked on on other bugs). Please reopen if you disagree! |