Bug 16041 - StaffAuthorisedValueImages & AuthorisedValueImages preferences - impact on search performance
Summary: StaffAuthorisedValueImages & AuthorisedValueImages preferences - impact on se...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15262 16167
  Show dependency treegraph
 
Reported: 2016-03-10 12:33 UTC by Jacek Ablewicz
Modified: 2016-12-05 21:25 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16041: Turn off StaffAuthorisedValueImages by default (5.03 KB, patch)
2016-03-30 07:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16041: Turn off StaffAuthorisedValueImages by default (5.20 KB, patch)
2016-04-13 08:11 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 16041: Turn off StaffAuthorisedValueImages by default (5.30 KB, patch)
2016-04-25 11:20 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2016-03-10 12:33:18 UTC
Those preferences (StaffAuthorisedValueImages and AuthorisedValueImages) have currently (after Bug 11842) big negative impact on the search performance. If you don't use those features, disabling them will result in considerable search speed gain in staff and OPAC respectivelly (around 500-700 miliseconds on the fast server - depending of the cache system in use).

Also note that StaffAuthorisedValueImages preference is enabled by default in the fresh installs, so it probably affects a lot of people which don't use / don't even know about this feature.
Comment 1 Jonathan Druart 2016-03-29 16:02:06 UTC
It seems that the use of a three-level cache could have fixed this issue. Could you confirm?
Comment 2 Jonathan Druart 2016-03-29 16:02:39 UTC
I meant, at least for the regression that bug 11842 could have caused.
Comment 3 Jonathan Druart 2016-03-30 07:56:05 UTC Comment hidden (obsolete)
Comment 4 Liz Rea 2016-03-31 20:39:08 UTC
SUGGEST_FORMAT authorised values in the sample data come with image urls by default, do we care about this?
Comment 5 Jonathan Druart 2016-04-04 07:13:02 UTC
(In reply to Liz Rea from comment #4)
> SUGGEST_FORMAT authorised values in the sample data come with image urls by
> default, do we care about this?

What do you mean? The pref will be disabled if none of the authorised_values.imageurl is set.
Moreover I don't find where SUGGEST_FORMAT would be inserted with an image url
Comment 6 Owen Leonard 2016-04-12 13:00:27 UTC
(In reply to Jonathan Druart from comment #5)
> Moreover I don't find where SUGGEST_FORMAT would be inserted with an image
> url

I don't see it either. 

In fact I don't see where authorized value images are displayed anywhere but in views which are now deprecated in favor of XSLT views. Am  I missing something? Is there any reason to keep either of these preferences and the associated code?

Authorized value images are displayed on the advanced search pages in the OPAC and staff client if you have search by collection code enabled. But the display of the images is not tied to the preferences.

I think both StaffAuthorisedValueImages and AuthorisedValueImages should be removed.
Comment 7 Owen Leonard 2016-04-12 13:02:21 UTC
Oh, now I see Bug 16167.

Why two bugs?
Comment 8 Jonathan Druart 2016-04-13 07:53:32 UTC
This patch will only turn the pref off.
It's the quickest way to remove the useless processing done regarding these prefs. Especially in C4::Search::searchResults.

On the second bug report I suggest to completely remove the image management (DB column, templates and scripts changes, etc.)
Comment 9 Marc Véron 2016-04-13 08:11:43 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2016-04-25 11:20:11 UTC
Created attachment 50628 [details] [review]
[PASSED QA] Bug 16041: Turn off StaffAuthorisedValueImages by default

This feature is enabled by default, but the users are not aware of it
and it costs a lot of time processing to get the images.

There are 2 prefs to drive this feature: StaffAuthorisedValueImages and
AuthorisedValueImages. AuthorisedValueImages is not added by sysprefs.sql and
does not appear in updatedatabase.pl, we could easily imagine that
nobody uses it.
With XSLT enabled, the feature is only visible on a record detail page
at the OPAC, if AuthorisedValueImages is set.
Otherwise you need to turn the XSLT off. In this case you will see the
images on the result list (OPAC+Staff interfaces) and OPAC detail page,
but not the Staff detail page.

The idea of this patch is to introduce a quick switch if the feature is
not used by the library.

Test plan:
1/ Turn the pref on and set authorised_values.imageurl to NULL
Execute the DB entry
=> The pref have been turned off
2/ Turn the pref on and set an image for an authorised value
Execute the DB entry
You will get a warning
3/ Turn the pref off and set an image for an authorised value
Execute the DB entry
You will get a warning
4/ Turn the pref off and set authorised_values.imageurl to NULL
Execute the DB entry
You won't get a warning

Note that the opac detail page now checks the pref before retrieving the
images.

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Fixed update message 'that means'.
Comment 11 Marcel de Rooy 2016-04-25 11:55:55 UTC
(In reply to Jonathan Druart from comment #3)
> This feature is enabled by default, but the users are not aware of it
> and it costs a lot of time processing to get the images.
What is a lot? I see e.g. 0.06sec

> There are 2 prefs to drive this feature: StaffAuthorisedValueImages and
> AuthorisedValueImages. AuthorisedValueImages is not added by sysprefs.sql and
> does not appear in updatedatabase.pl, we could easily imagine that
> nobody uses it.
AuthorisedValueImages should have been there. But its absence does not prevent anyone to enable the pref.
Comment 12 Jacek Ablewicz 2016-04-25 12:55:02 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Jonathan Druart from comment #3)
> > This feature is enabled by default, but the users are not aware of it
> > and it costs a lot of time processing to get the images.
> What is a lot? I see e.g. 0.06sec

Total cost: (get_biblio_authorised_values() call cost) * (number of search results)

get_biblio_authorised_values() cost: GetMarcStructure() + ca 8 msec

GetMarcStructure() cost:

   master w/ memcached: 62 msec

   master w/o memcached: 40 msec
   (depending on the mysql config - Cache::Memory is currently nonfunctional,
   framework structures are fetched from DB directly - see Bug 16104 comment #8)

   master w/ memcached + Bug 16221: 18 msec

   master w/ memcached + Bug 16166: 10 msec

   master w/ memcached + 2nd patch from Bug 16140: 0.3 msec


For eg. 50 search results per page, it currently adds up to 3.5 seconds (master w/ memcached) or ~2 seconds (master w/o memcached) - on the relatively fast server.
Comment 13 Jonathan Druart 2016-04-25 15:40:37 UTC
(In reply to Marcel de Rooy from comment #11)
> > There are 2 prefs to drive this feature: StaffAuthorisedValueImages and
> > AuthorisedValueImages. AuthorisedValueImages is not added by sysprefs.sql and
> > does not appear in updatedatabase.pl, we could easily imagine that
> > nobody uses it.
> AuthorisedValueImages should have been there. But its absence does not
> prevent anyone to enable the pref.

And the only impact will be to cause perf issues :)
Unless the non-XSLT view is used (which has been deprecated).
Comment 14 Marcel de Rooy 2016-04-27 09:12:06 UTC
(In reply to Jacek Ablewicz from comment #12)
> (In reply to Marcel de Rooy from comment #11)
> > (In reply to Jonathan Druart from comment #3)
> > > This feature is enabled by default, but the users are not aware of it
> > > and it costs a lot of time processing to get the images.
> > What is a lot? I see e.g. 0.06sec
> 
> Total cost: (get_biblio_authorised_values() call cost) * (number of search
> results)
> 
> get_biblio_authorised_values() cost: GetMarcStructure() + ca 8 msec
> 
> GetMarcStructure() cost:
> 
>    master w/ memcached: 62 msec
> 
>    master w/o memcached: 40 msec
>    (depending on the mysql config - Cache::Memory is currently nonfunctional,
>    framework structures are fetched from DB directly - see Bug 16104 comment
> #8)
> 
>    master w/ memcached + Bug 16221: 18 msec
> 
>    master w/ memcached + Bug 16166: 10 msec
> 
>    master w/ memcached + 2nd patch from Bug 16140: 0.3 msec
> 
> 
> For eg. 50 search results per page, it currently adds up to 3.5 seconds
> (master w/ memcached) or ~2 seconds (master w/o memcached) - on the
> relatively fast server.

First, please note that I fully support this report. I was about to pass QA on it when Katrin already did. But I could not resist to bring the performance gain into some perspective.
It is only very good to prevent useless calls to get_authorised_value_images and get_biblio_authorised_values. There should definitely be a cheaper way to get some imageurls than going thru GetMarcStructure etc. These two routines are quite horrible actually.

But forgive me noting that I do not agree with multiplying this gain with factor 50 :) This patch only deals with opac-detail.pl..
Comment 15 Jacek Ablewicz 2016-04-27 09:41:31 UTC
(In reply to Marcel de Rooy from comment #14)

> But forgive me noting that I do not agree with multiplying this gain with
> factor 50 :) This patch only deals with opac-detail.pl..

Err, it doesn't deal with opac-details.pl only - it also turns off StaffAuthorisedValueImages (on by default) in the libraries which don't have any images assigned to the authorised values / don't use this feature (probably 99.99% libraries or so). A "factor of 50" might be a bit exaggerated though ;), AFAIRC by default there are only 20 search results displayed per page in staff and OPAC, not 50.
Comment 16 Marcel de Rooy 2016-04-27 09:51:21 UTC
(In reply to Jacek Ablewicz from comment #15)
> (In reply to Marcel de Rooy from comment #14)
> 
> > But forgive me noting that I do not agree with multiplying this gain with
> > factor 50 :) This patch only deals with opac-detail.pl..
> 
> Err, it doesn't deal with opac-details.pl only - it also turns off
> StaffAuthorisedValueImages (on by default) in the libraries which don't have
> any images assigned to the authorised values / don't use this feature
> (probably 99.99% libraries or so). A "factor of 50" might be a bit
> exaggerated though ;), AFAIRC by default there are only 20 search results
> displayed per page in staff and OPAC, not 50.

OK Sorry, did overlook the effect on searchResults here for the staff side (opac goes via the other one, which was not even in sysprefs.sql).
Comment 17 Jacek Ablewicz 2016-04-27 10:57:28 UTC
(In reply to Marcel de Rooy from comment #14)

> It is only very good to prevent useless calls to get_authorised_value_images
> and get_biblio_authorised_values. There should definitely be a cheaper way
> to get some imageurls than going thru GetMarcStructure etc. These two
> routines are quite horrible actually.

I'm working on the patch which reduces GetMarcStructure() cost to ~1 msec (in the places where it matters - something like 2nd patch from Bug 16140, but more manageable / controllable), that should resolve most of the performance issues related to those 2 features. But get_biblio_authorised_values() and get_authorised_value_images() will still remain pretty horrible - not quite sure if Bug 16167 is a move in the right direction though, it looks like a bit of an overkill to me.
Comment 18 Jacek Ablewicz 2016-04-27 13:07:40 UTC
(In reply to Jacek Ablewicz from comment #17)

> I'm working on the patch which reduces GetMarcStructure() cost to ~1 msec
> (in the places where it matters - something like 2nd patch from Bug 16140,
> but more manageable / controllable), that should resolve most of the
> performance issues related to those 2 features.

Bug 16365. Probably a bit to late for 3.24^W 16.05 release ;).
Comment 19 Kyle M Hall 2016-04-29 13:52:20 UTC
Pushed to master for 16.05 release! Thanks Jonathan!
Comment 20 Julian Maurice 2016-05-16 07:35:02 UTC
Patches pushed to 3.22.x, will be in 3.22.7