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.
It seems that the use of a three-level cache could have fixed this issue. Could you confirm?
I meant, at least for the regression that bug 11842 could have caused.
Created attachment 49671 [details] [review] 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.
SUGGEST_FORMAT authorised values in the sample data come with image urls by default, do we care about this?
(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
(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.
Oh, now I see Bug 16167. Why two bugs?
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.)
Created attachment 50169 [details] [review] 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>
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'.
(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.
(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.
(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).
(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..
(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.
(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).
(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.
(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 ;).
Pushed to master for 16.05 release! Thanks Jonathan!
Patches pushed to 3.22.x, will be in 3.22.7