Bug 29454

Summary: Stash itemtypes in plugin objects to reduce DB calls
Product: Koha Reporter: Nick Clemens <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: arthur.suzuki, david, fridolin.somers, jonathan.druart, lucas, martin.renvoize, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28436
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26587
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30920
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.04, 21.11.11
Attachments: Bug 29454: Add local cache to Template Plugins for branches and itemtypes
Bug 29454: Add local cache to Template Plugins for branches and itemtypes
Bug 29454: Use Koha Cache Memory Lite to stash itemtype descriptions for template plugin
Bug 29454: Use Koha Cache Memory Lite to stash itemtype descriptions for template plugin
Bug 29454: Add unit test for Koha::Template::Plugin::ItemTypes
Bug 29454: Add unit test for Koha::Template::Plugin::ItemTypes
Bug 29454: (follow-up) Cover more test cases
Bug 29454: Use Koha Cache Memory Lite to stash itemtype descriptions for template plugin
Bug 29454: Add unit test for Koha::Template::Plugin::ItemTypes
Bug 29454: (follow-up) Cover more test cases

Description Nick Clemens 2021-11-10 16:50:31 UTC
Moved from bug 28436
Comment 1 Nick Clemens 2021-11-10 16:51:23 UTC
Created attachment 127515 [details] [review]
Bug 29454: 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
Comment 2 David Nind 2021-11-10 23:04:34 UTC
Created attachment 127520 [details] [review]
Bug 29454: 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

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2021-11-10 23:21:36 UTC
Testing notes using koha-testing-docker.

I found that for the detail page for the record in the staff interface that the load time with 1,001 items was significantly improved.

On the edit items page I didn't notice much, if any, improvement.

After the patch was applied I did a flush_memcached and restart_all, but not fater changing the item type and library names.

Curious as to why we don't use pagination when the there are lots of items, on the dipslay item page in the staff interface and OPAC.

Some numbers (a bit variable, but generally around the same) - YMMV:

Before patch applied
~~~~~~~~~~~~~~~~~~~~

Item page in the staff interface:

  1027 requests
  6.74 MB / 49.24 KB transferred
  Finish: 20.45 s
  DOMContentLoaded: 18.52 s
  load: 20.01 s

Edit items:

  25 requests
  5.50 MB / 82.78 KB transferred
  Finish: 1.10 min
  DOMContentLoaded: 1.10 min
  load: 1.12 min

After the patch applied
~~~~~~~~~~~~~~~~~~~~~~~

Item page:

  1030 requests
  7.19 MB / 970.08 KB transferred
  Finish: 17.07 s
  DOMContentLoaded: 14.66 s
  load: 16.11 s

Edit items page:

  27 requests
  5.63 MB / 107.81 KB transferred
  Finish: 56.97 s
  DOMContentLoaded: 56.77 s
  load: 58.19 s

After the patch applied and change to item type and library names
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Used the refresh button.

Item page:

  1027 requests
  7.21 MB / 48.92 KB transferred
  Finish: 17.27 s
  DOMContentLoaded: 15.27 s
  load: 16.87 s

Edit items:

  36 requests
  6.40 MB / 1.24 MB transferred
  Finish: 1.10 min
  DOMContentLoaded: 1.09 min
  load: 1.12 min
Comment 4 Jonathan Druart 2021-12-13 13:44:22 UTC
This is a duplicate of bug 26587. However I think the approach there is better (use of Koha::Cache::Memory::Lite), but it only deals with 'Branches'.

If you agree with using the L1 cache, could you add your QA stamp there and provide a patch here for itemtypes?
Comment 5 Nick Clemens 2021-12-13 14:48:48 UTC
Created attachment 128472 [details] [review]
Bug 29454: Use Koha Cache Memory Lite to stash itemtype descriptions for template plugin

To test:
1 - Add 1000 items to a record, of varying item types
2 - Bring up the details page
3 - Note time to load
4 - Apply patch
5 - Reload page and compare to previous
6 - Confirm information is correct
7 - Confirm some performance benefit
Comment 6 Jonathan Druart 2021-12-13 14:50:59 UTC
Thanks, Nick!
Comment 7 Fridolin Somers 2022-01-08 01:31:35 UTC
Maybe add an early return q{} if itemtypecode is undefined,
to avoid warning in concatenation :
my $cache_key    = $want_parent ? "Itemtype_parent_description:".$itemtypecode : "Itemtype_description:" . $itemtypecode;
Comment 8 Fridolin Somers 2022-06-14 21:58:27 UTC
Created attachment 136077 [details] [review]
Bug 29454: Use Koha Cache Memory Lite to stash itemtype descriptions for template plugin

Returns empty string if given item type is undefined or unknown

To test:
1 - Add 1000 items to a record, of varying item types
2 - Bring up the details page
3 - Note time to load
4 - Apply patch
5 - Reload page and compare to previous
6 - Confirm information is correct
7 - Confirm some performance benefit

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 9 Fridolin Somers 2022-06-14 21:58:43 UTC
Created attachment 136078 [details] [review]
Bug 29454: Add unit test for Koha::Template::Plugin::ItemTypes
Comment 10 Fridolin Somers 2022-06-14 22:05:00 UTC
(In reply to Fridolin Somers from comment #7)
> Maybe add an early return q{} if itemtypecode is undefined,
> to avoid warning in concatenation :
> my $cache_key    = $want_parent ?
> "Itemtype_parent_description:".$itemtypecode : "Itemtype_description:" .
> $itemtypecode;

I've added this.
And also a write of empty string in cache when item type is not found.
I signed first patch.

Added a unit test for Koha::Template::Plugin::ItemTypes.
This needs SO.

Best regards,
Run Koha, run ... ^^
Comment 11 Nick Clemens 2022-06-15 12:08:55 UTC
Created attachment 136090 [details] [review]
Bug 29454: Add unit test for Koha::Template::Plugin::ItemTypes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2022-06-15 12:08:58 UTC
Created attachment 136091 [details] [review]
Bug 29454: (follow-up) Cover more test cases
Comment 13 Fridolin Somers 2022-06-15 21:44:04 UTC
Great thanks.

BTW
shouldn't there be some space around "->" when there is a parent ?
"Translated itemtype A->Translated itemtype B"
Comment 14 Martin Renvoize 2022-06-16 11:27:45 UTC
Created attachment 136149 [details] [review]
Bug 29454: Use Koha Cache Memory Lite to stash itemtype descriptions for template plugin

Returns empty string if given item type is undefined or unknown

To test:
1 - Add 1000 items to a record, of varying item types
2 - Bring up the details page
3 - Note time to load
4 - Apply patch
5 - Reload page and compare to previous
6 - Confirm information is correct
7 - Confirm some performance benefit

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-06-16 11:27:49 UTC
Created attachment 136150 [details] [review]
Bug 29454: Add unit test for Koha::Template::Plugin::ItemTypes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-06-16 11:27:53 UTC
Created attachment 136151 [details] [review]
Bug 29454: (follow-up) Cover more test cases

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-06-16 11:28:33 UTC
Nice work.. works well, tests passing and coverage is good..

PQA
Comment 18 Tomás Cohen Arazi 2022-06-24 14:59:47 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 19 Lucas Gass 2022-07-29 15:57:12 UTC
Backported to 22.05.x for 22.05.04
Comment 20 Arthur Suzuki 2022-08-11 09:30:39 UTC
backported to 21.11.x for 21.11.11
Comment 21 Victor Grousset/tuxayo 2022-08-13 16:10:05 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.