Bug 33447 - Add caching to Biblio->pickup_locations
Summary: Add caching to Biblio->pickup_locations
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 32121
Blocks: 33471
  Show dependency treegraph
 
Reported: 2023-04-07 21:08 UTC by Nick Clemens
Modified: 2023-12-28 20:43 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06


Attachments
Bug 33447: Add Cache to Biblio->pickup_locations (2.14 KB, patch)
2023-04-07 21:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33447: Adjust tests (1.09 KB, patch)
2023-04-07 21:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33447: Add Cache to Biblio->pickup_locations (2.20 KB, patch)
2023-04-10 15:31 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 33447: Adjust tests (1.15 KB, patch)
2023-04-10 15:31 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 33447: Add Cache to Biblio->pickup_locations (2.30 KB, patch)
2023-04-18 11:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33447: Adjust tests (1.25 KB, patch)
2023-04-18 11:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33447: (QA follow-up) Add comment for flushing cache (1.04 KB, patch)
2023-04-18 11:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33447: (follow-up) Fix tests and make assumption explicit (4.54 KB, patch)
2023-04-20 15:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33447: (follow-up) Fix tests and make assumption explicit (4.65 KB, patch)
2023-04-20 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33447: Make *->pickup_locations methods throw an exception on missing parameter (7.06 KB, patch)
2023-04-20 18:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-04-07 21:08:19 UTC
Biblio->pickup_locations iterates over each item and calls Item->pickup_locations for each

The routine uses a maximum of 5 data points: homebranch, holdingbranch, itype (specifically, not effective_itemtype), ccode (if using branch transfer limits by ccode), and patron branchcode

We can add L1 caching on this call to reduce simple repeated calls
Comment 1 Nick Clemens 2023-04-07 21:47:36 UTC
Created attachment 149295 [details] [review]
Bug 33447: Add Cache to Biblio->pickup_locations

This is going to have the most effect on records with large numbers of items
held by the same library, serial records and the like

To test:
1 - Add 500 items to a biblio by select myltiple copies on the add item page
2 - Place a hold via the API and note response time, I found ~3-5 seconds
3 - Apply patch
4 - Restart all
5 - Place hold using api again
6 - Note improved response time, less than 1/2 a second in my tests
Comment 2 Nick Clemens 2023-04-07 21:47:38 UTC
Created attachment 149296 [details] [review]
Bug 33447: Adjust tests
Comment 3 Emily Lamancusa 2023-04-10 15:31:14 UTC
Created attachment 149389 [details] [review]
Bug 33447: Add Cache to Biblio->pickup_locations

This is going to have the most effect on records with large numbers of items
held by the same library, serial records and the like

To test:
1 - Add 500 items to a biblio by select myltiple copies on the add item page
2 - Place a hold via the API and note response time, I found ~3-5 seconds
3 - Apply patch
4 - Restart all
5 - Place hold using api again
6 - Note improved response time, less than 1/2 a second in my tests

Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov>
Comment 4 Emily Lamancusa 2023-04-10 15:31:16 UTC
Created attachment 149390 [details] [review]
Bug 33447: Adjust tests

Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov>
Comment 5 Marcel de Rooy 2023-04-14 07:18:17 UTC
I think that it would be a very good thing to try caching here. What I am not sure about, is the theoretical possibility of getting wrong results if we would repeatedly ask for pickup_locations (while handling one request).

The change in the test confirms this actually. You do not add a test for caching, but just add a flush. Removing that one makes the test fail.

# Subtest: pickup_locations
    not ok 6 - ReservesControlBranch: PatronLibrary, biblio1, patron8 should return 3 but returns 1
    not ok 7 - ReservesControlBranch: PatronLibrary, biblio2, patron1 should return 0 but returns 2
    not ok 8 - ReservesControlBranch: PatronLibrary, biblio2, patron8 should return 3 but returns 0
    # Looks like you failed 3 tests of 9.
not ok 8 - pickup_locations

I would recommend to add the flush in Biblio->pickup_locations.
Comment 6 Nick Clemens 2023-04-14 12:01:11 UTC
As I understood, the L1 and Memory Lite caches are flushed on each request:

/etc/koha/sites/kohadev/plack.psgi:
use CGI qw(-utf8 ); # we will loose -utf8 under plack, otherwise
{
    no warnings 'redefine';
    my $old_new = \&CGI::new;
    *CGI::new = sub {
        my $q = $old_new->( @_ );
        $CGI::PARAM_UTF8 = 1;
        Koha::Caches->flush_L1_caches();
        Koha::Cache::Memory::Lite->flush();
        return $q; 
    };  
}

Am I not understanding correctly?
Comment 7 Marcel de Rooy 2023-04-14 13:12:48 UTC
(In reply to Marcel de Rooy from comment #5)
> I think that it would be a very good thing to try caching here. What I am
> not sure about, is the theoretical possibility of getting wrong results if
> we would repeatedly ask for pickup_locations (while handling one request).

(In reply to Nick Clemens from comment #6)
> As I understood, the L1 and Memory Lite caches are flushed on each request:
> 
> Am I not understanding correctly?

Sure you do. But see above.
Comment 8 Nick Clemens 2023-04-14 13:53:05 UTC
(In reply to Marcel de Rooy from comment #5)
> I think that it would be a very good thing to try caching here. What I am
> not sure about, is the theoretical possibility of getting wrong results if
> we would repeatedly ask for pickup_locations (while handling one request).
> 
> The change in the test confirms this actually. You do not add a test for
> caching, but just add a flush. Removing that one makes the test fail.
> 
> # Subtest: pickup_locations
>     not ok 6 - ReservesControlBranch: PatronLibrary, biblio1, patron8 should
> return 3 but returns 1
>     not ok 7 - ReservesControlBranch: PatronLibrary, biblio2, patron1 should
> return 0 but returns 2
>     not ok 8 - ReservesControlBranch: PatronLibrary, biblio2, patron8 should
> return 3 but returns 0
>     # Looks like you failed 3 tests of 9.
> not ok 8 - pickup_locations
> 
> I would recommend to add the flush in Biblio->pickup_locations.

Those tests fail because we have changed a syspref - we are essentially testing multiple requests there. It is not expected for a syspref to change during a request. I flush to indicate that
Comment 9 Marcel de Rooy 2023-04-17 13:14:03 UTC
(In reply to Nick Clemens from comment #8)
> (In reply to Marcel de Rooy from comment #5)
> > I think that it would be a very good thing to try caching here. What I am
> > not sure about, is the theoretical possibility of getting wrong results if
> > we would repeatedly ask for pickup_locations (while handling one request).
> > 
> > The change in the test confirms this actually. You do not add a test for
> > caching, but just add a flush. Removing that one makes the test fail.
> > 
> > # Subtest: pickup_locations
> >     not ok 6 - ReservesControlBranch: PatronLibrary, biblio1, patron8 should
> > return 3 but returns 1
> >     not ok 7 - ReservesControlBranch: PatronLibrary, biblio2, patron1 should
> > return 0 but returns 2
> >     not ok 8 - ReservesControlBranch: PatronLibrary, biblio2, patron8 should
> > return 3 but returns 0
> >     # Looks like you failed 3 tests of 9.
> > not ok 8 - pickup_locations
> > 
> > I would recommend to add the flush in Biblio->pickup_locations.
> 
> Those tests fail because we have changed a syspref - we are essentially
> testing multiple requests there. It is not expected for a syspref to change
> during a request. I flush to indicate that

Thx for explaining. Would be good to add a short comment for such things.
Comment 10 Marcel de Rooy 2023-04-17 13:15:41 UTC
(In reply to Marcel de Rooy from comment #5)
> I think that it would be a very good thing to try caching here. What I am
> not sure about, is the theoretical possibility of getting wrong results if
> we would repeatedly ask for pickup_locations (while handling one request).
> 
> I would recommend to add the flush in Biblio->pickup_locations.

Apart from the syspref issue, this comment still got a bit out of sight. The theoretical(!) chance of calling it multiple times within one(!) request..
Comment 11 Nick Clemens 2023-04-17 13:33:59 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Marcel de Rooy from comment #5)
> > I think that it would be a very good thing to try caching here. What I am
> > not sure about, is the theoretical possibility of getting wrong results if
> > we would repeatedly ask for pickup_locations (while handling one request).
> > 
> > I would recommend to add the flush in Biblio->pickup_locations.
> 
> Apart from the syspref issue, this comment still got a bit out of sight. The
> theoretical(!) chance of calling it multiple times within one(!) request..

Multiple times is fine, we don't expect sysprefs to change within a single request - even for different bibs the results are not different for the same type of item
Comment 12 Marcel de Rooy 2023-04-18 11:22:04 UTC
(In reply to Nick Clemens from comment #11)
> (In reply to Marcel de Rooy from comment #10)
> > (In reply to Marcel de Rooy from comment #5)
> > > I think that it would be a very good thing to try caching here. What I am
> > > not sure about, is the theoretical possibility of getting wrong results if
> > > we would repeatedly ask for pickup_locations (while handling one request).
> > > 
> > > I would recommend to add the flush in Biblio->pickup_locations.
> > 
> > Apart from the syspref issue, this comment still got a bit out of sight. The
> > theoretical(!) chance of calling it multiple times within one(!) request..
> 
> Multiple times is fine, we don't expect sysprefs to change within a single
> request - even for different bibs the results are not different for the same
> type of item

I had in mind to be depending on some patron attribute too, but it looks good. In pickup_locations and related calls, we only seem to actually use branchcode.
Comment 13 Marcel de Rooy 2023-04-18 11:30:12 UTC
Created attachment 149790 [details] [review]
Bug 33447: Add Cache to Biblio->pickup_locations

This is going to have the most effect on records with large numbers of items
held by the same library, serial records and the like

To test:
1 - Add 500 items to a biblio by select myltiple copies on the add item page
2 - Place a hold via the API and note response time, I found ~3-5 seconds
3 - Apply patch
4 - Restart all
5 - Place hold using api again
6 - Note improved response time, less than 1/2 a second in my tests

Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2023-04-18 11:30:14 UTC
Created attachment 149791 [details] [review]
Bug 33447: Adjust tests

Signed-off-by: emlam <emily.lamancusa@montgomerycountymd.gov>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2023-04-18 11:30:17 UTC
Created attachment 149792 [details] [review]
Bug 33447: (QA follow-up) Add comment for flushing cache

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Tomás Cohen Arazi 2023-04-19 12:22:14 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 17 Nick Clemens 2023-04-20 15:46:23 UTC
Created attachment 149968 [details] [review]
Bug 33447: (follow-up) Fix tests and make assumption explicit

The patches made an assumption that patron would always be passed. It is
within Koha, but not in the Biblios tests.

There is no scenario where we can determine pickup locations that are not in
reference to a patron (who is picking it up?) so we should always have
this parameter
Comment 18 Tomás Cohen Arazi 2023-04-20 18:08:40 UTC
Created attachment 149981 [details] [review]
Bug 33447: (follow-up) Fix tests and make assumption explicit

The patches made an assumption that patron would always be passed. It is
within Koha, but not in the Biblios tests.

There is no scenario where we can determine pickup locations that are not in
reference to a patron (who is picking it up?) so we should always have
this parameter
Comment 19 Tomás Cohen Arazi 2023-04-20 18:08:43 UTC
Created attachment 149982 [details] [review]
Bug 33447: Make *->pickup_locations methods throw an exception on missing parameter

This patch fixes the FIXME for making the methods throw an exception.

Tests are added, and POD is adapted as well.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/patrons* \
           t/db_dependent/api/v1/holds.t \
           t/db_dependent/Reserves* \
           t/db_dependent/Hold* \
           t/db_dependent/Koha/Hold* \
           t/db_dependent/Items*

=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Jacob O'Mara 2023-05-09 20:52:43 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 21 Jonathan Druart 2023-05-10 08:29:18 UTC
(In reply to Jacob O'Mara from comment #20)
> Nice work, thanks everyone!
> 
> Pushed to 22.11.x for the next release.

You broke Jenkins
  Can't locate Koha/Exceptions/Checkin.pm
Comment 22 Lucas Gass 2023-05-15 21:03:16 UTC
Missing dependencies for 22.05.x, no backport.