Bug 35558 - Do not fetch local image if none exists
Summary: Do not fetch local image if none exists
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on: 35557
Blocks:
  Show dependency treegraph
 
Reported: 2023-12-13 10:09 UTC by Jonathan Druart
Modified: 2024-05-28 20:40 UTC (History)
7 users (show)

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


Attachments
Bug 35558: Do not retrieve the local image if none exists (2.88 KB, patch)
2023-12-13 10:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35558: Do not retrieve the local image if none exists (2.92 KB, patch)
2023-12-19 18:13 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 35558: Do not retrieve the local image if none exists - OPAC (2.25 KB, patch)
2024-03-14 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35558: Do not retrieve the local image if none exists (2.98 KB, patch)
2024-04-02 18:35 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35558: Do not retrieve the local image if none exists - OPAC (2.87 KB, patch)
2024-04-02 18:35 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-12-13 10:09:52 UTC
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?
Comment 1 Jonathan Druart 2023-12-13 10:12:03 UTC
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.
Comment 2 Lucas Gass 2023-12-13 15:56:00 UTC
> Should not we have this info beforehand in order to reduce the number of
> hits and network traffic?

Yes. 
+1
100%.
Comment 3 David Cook 2023-12-13 22:25:17 UTC
(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!
Comment 4 Barbara Petritsch 2023-12-14 12:52:37 UTC
There are no instructions for testing
Comment 5 Nick Clemens (kidclamp) 2023-12-14 13:38:42 UTC
(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.
Comment 6 Jonathan Druart 2023-12-14 14:18:13 UTC
(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?
Comment 7 Jonathan Druart 2023-12-14 14:19:23 UTC
(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*.
Comment 8 Jonathan Druart 2023-12-14 14:20:38 UTC
(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.
Comment 9 Nick Clemens (kidclamp) 2023-12-14 14:33:31 UTC
(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
Comment 10 Jonathan Druart 2023-12-14 15:13:47 UTC
> which benefits the user

Unless it overloads the server because it gets too many requests, which is the goal of this approach.
Comment 11 Nick Clemens (kidclamp) 2023-12-14 17:57:51 UTC
(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?
Comment 12 Tomás Cohen Arazi 2023-12-14 18:12:51 UTC
(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
Comment 13 David Cook 2023-12-14 23:13:53 UTC
(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.
Comment 14 Didier Gautheron 2023-12-15 05:34:43 UTC
(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).
Comment 15 Owen Leonard 2023-12-19 18:13:35 UTC
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>
Comment 16 Didier Gautheron 2023-12-20 08:02:52 UTC
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.
Comment 17 Didier Gautheron 2024-03-14 13:11:14 UTC
Same is need for OPAC
Comment 18 Jonathan Druart 2024-03-14 14:02:21 UTC
Created attachment 163136 [details] [review]
Bug 35558: Do not retrieve the local image if none exists - OPAC
Comment 19 Nick Clemens (kidclamp) 2024-04-02 18:35:22 UTC
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>
Comment 20 Nick Clemens (kidclamp) 2024-04-02 18:35:25 UTC
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>
Comment 21 Nick Clemens (kidclamp) 2024-04-02 18:36:17 UTC
Note: for a library making heavy use of local covers this will have little impact
Comment 22 Katrin Fischer 2024-04-26 16:07:03 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 23 Fridolin Somers 2024-05-24 08:05:24 UTC
Nice small perf patch, I backport

Pushed to 23.11.x for 23.11.06
Comment 24 Lucas Gass 2024-05-28 20:40:07 UTC
Enhancement will not be backported to 23.05.x