On the search results we are fetching the thumbnails of the local cover images using catalogue/image.pl Which means 1 GET per results on the page. Should not we have this info beforehand in order to reduce the number of hits and network traffic?
Created attachment 159785 [details] [review] Bug 35558: Do not retrieve the local image if none exists On the search results we are fetching the thumbnails of the local cover images using catalogue/image.pl Which means 1 GET per results on the page. This patch suggests to have this information beforehand in order to reduce the number of hits and network traffic? However the page load will be slightly slower ofc.
> Should not we have this info beforehand in order to reduce the number of > hits and network traffic? Yes. +1 100%.
(In reply to Lucas Gass from comment #2) > > Should not we have this info beforehand in order to reduce the number of > > hits and network traffic? > > Yes. > +1 > 100%. Ditto!
There are no instructions for testing
(In reply to Jonathan Druart from comment #1) > However the page load will be slightly slower ofc. I don't like this. I think the check should be lower if we are going to do it earlier, maybe in searchResults. I don't want to add to that routine, but I also don't think this should be done in the script.
(In reply to Nick Clemens from comment #5) > (In reply to Jonathan Druart from comment #1) > > However the page load will be slightly slower ofc. > > I don't like this. > > I think the check should be lower if we are going to do it earlier, maybe in > searchResults. I don't want to add to that routine, but I also don't think > this should be done in the script. I put it in the controller on purpose. It's not an info we want every time we call searchResults. Why shouldn't it be done in the script?
(In reply to Barbara Petritsch from comment #4) > There are no instructions for testing Add local cover image to some of your bibliographic records. Open the browser console, tab "network". Search for records. => Without this patch you will see 1 GET per result => With this patch applied you will see 1 GET per result *with a cover image*.
(In reply to Nick Clemens from comment #5) > (In reply to Jonathan Druart from comment #1) > > However the page load will be slightly slower ofc. > > I don't like this. > > I think the check should be lower if we are going to do it earlier, maybe in > searchResults. I don't want to add to that routine, but I also don't think > this should be done in the script. And it won't be faster to have this code in searchResults ;) The "slowness" (how much?) will be there if we decide to have the info anyway.
(In reply to Jonathan Druart from comment #8) > (In reply to Nick Clemens from comment #5) > > (In reply to Jonathan Druart from comment #1) > > > However the page load will be slightly slower ofc. > > > > I don't like this. > > > > I think the check should be lower if we are going to do it earlier, maybe in > > searchResults. I don't want to add to that routine, but I also don't think > > this should be done in the script. > > And it won't be faster to have this code in searchResults ;) > > The "slowness" (how much?) will be there if we decide to have the info > anyway. If we prefetch the count when getting the biblio object we can reduce calls I prefer loading the page and then making the calls because it makes the page load faster - which benefits the user
> which benefits the user Unless it overloads the server because it gets too many requests, which is the goal of this approach.
(In reply to Jonathan Druart from comment #10) > > which benefits the user > > Unless it overloads the server because it gets too many requests, which is > the goal of this approach. Is there evidence of this happening in production?
(In reply to Nick Clemens from comment #11) > (In reply to Jonathan Druart from comment #10) > > > which benefits the user > > > > Unless it overloads the server because it gets too many requests, which is > > the goal of this approach. > > Is there evidence of this happening in production? I feel like we should make sure cache expiration timestamps are sent correctly from the svc. So: - Only ask for local covers on things that have them - Only return them from the svc if they have expired on the browser
(In reply to Tomás Cohen Arazi from comment #12) > (In reply to Nick Clemens from comment #11) > > (In reply to Jonathan Druart from comment #10) > > > > which benefits the user > > > > > > Unless it overloads the server because it gets too many requests, which is > > > the goal of this approach. > > > > Is there evidence of this happening in production? While I haven't had a server overloaded by image requests, it would be nice to reduce the number of unnecessary HTTP requests. It is conceptually painful looking at 20 requests going out when you know 0 are going to return anything on a given page. That CPU time could be put to better use. Prefetching is an interesting idea. I'm not familiar enough with the DBIx::Class internals for prefetching to know how it works exactly, but in theory it might cut down on DB calls. > I feel like we should make sure cache expiration timestamps are sent > correctly from the svc. So: > > - Only ask for local covers on things that have them > - Only return them from the svc if they have expired on the browser That's a good idea too, although I think it would be a separate ticket.
(In reply to Nick Clemens from comment #11) > (In reply to Jonathan Druart from comment #10) > > > which benefits the user > > > > Unless it overloads the server because it gets too many requests, which is > > the goal of this approach. > > Is there evidence of this happening in production? Yes we have. For some reason customers like to set the result display to 100 (it's a syspref) or more and we have 499 errors as a result (with nginx it's a timeout error).
Created attachment 160059 [details] [review] Bug 35558: Do not retrieve the local image if none exists On the search results we are fetching the thumbnails of the local cover images using catalogue/image.pl Which means 1 GET per results on the page. This patch suggests to have this information beforehand in order to reduce the number of hits and network traffic? However the page load will be slightly slower ofc. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
About slowdown: I applied it on customers Koha with syspref numSearchResults as high as 500, server nginx HTTP 1.1 Before response time for search.pl (around 1200 results): 16 to 18 seconds. After: 17 to 18 seconds.
Same is need for OPAC
Created attachment 163136 [details] [review] Bug 35558: Do not retrieve the local image if none exists - OPAC
Created attachment 164295 [details] [review] Bug 35558: Do not retrieve the local image if none exists On the search results we are fetching the thumbnails of the local cover images using catalogue/image.pl Which means 1 GET per results on the page. This patch suggests to have this information beforehand in order to reduce the number of hits and network traffic? However the page load will be slightly slower ofc. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 164296 [details] [review] Bug 35558: Do not retrieve the local image if none exists - OPAC Test plan: 1 - Enable system preferences: LocalCoverImages OPACLocalCoverImages 2 - open browser tools network page 3 - Perform an intranet search 4 - Note a request for each bib like: http://localhost:8081/cgi-bin/koha/catalogue/image.pl?thumbnail=1&biblionumber=35 5 - Repeat on opac, same requests 6 - Apply patches 7 - Restart all 8 - Repeat searches, note no fetches 9 - Add local cover images to several biblios in the results 10 - Repeat searches 11 - Note the fetches are only for those records with images 12 - Confirm images still load correctly Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Note: for a library making heavy use of local covers this will have little impact
Pushed for 24.05! Well done everyone, thank you!
Nice small perf patch, I backport Pushed to 23.11.x for 23.11.06
Enhancement will not be backported to 23.05.x