On search, every single result goes through some XSLT processing. This includes fetching the relevant sysprefs every single time. We should do it only once per search.
Created attachment 45221 [details] [review] Bug 15263 XSLT display fetches sysprefs for every result processed On search, every single result goes through some XSLT processing. This includes fetching the relevant sysprefs every single time. We should do it only once per search.
I think this patch would become useless with the patch Bug 11998: Add a L1 cache for sysprefs
(In reply to Jonathan Druart from comment #2) > I think this patch would become useless with the patch Bug 11998: Add a L1 > cache for sysprefs Not entirely - it still improves catalogue/search.pl run time by ca 80-100ms (on the fast server): - ~30ms save on eliminating 1100+ redundant syspref() calls, 21*49 string concatenations etc. - 45ms on eliminating 49 redundant C4::Languages::getlanguage() calls
Created attachment 48962 [details] [review] Bug 15263 XSLT display fetches sysprefs for every result processed On search, every single result goes through some XSLT processing. This includes fetching the relevant sysprefs every single time. We should do it only once per search.
Created attachment 48963 [details] [review] Bug 15263: (follow-up) XSLT display fetches sysprefs for every result processed Set variables ($sysxml, $xslfilename, $lang) if they are not passed to the subroutine. This happens from catalogue/detail.pl, opac/opac-shelves.pl, opac/opac-tags.pl and virtualshelves/shelves.pl.
I have rebased the original patch and added a follow-up to make it works with other calls.
Test plan: On the search result and detail page, search for regression. On the search result (with a lot of results) you should notice a perf gain (I have not compared it).
Created attachment 49031 [details] [review] Bug 15263: (follow-up) XSLT display fetches sysprefs for every result processed Set variables ($sysxml, $xslfilename, $lang) if they are not passed to the subroutine. This happens from catalogue/detail.pl, opac/opac-shelves.pl, opac/opac-tags.pl and virtualshelves/shelves.pl. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
The functionality looks good. But it breaks t/db_dependent/Search.t
Created attachment 49159 [details] [review] Bug 15263: Bug 15263: (follow-up 2) XSLT display fetches sysprefs for every result processed Don't retrieve prefs if we won't need them
Created attachment 49502 [details] [review] Bug 15263: XSLT display fetches sysprefs for every result processed On search, every single result goes through some XSLT processing. This includes fetching the relevant sysprefs every single time. We should do it only once per search. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 49503 [details] [review] Bug 15263: (follow-up) XSLT display fetches sysprefs for every result processed Set variables ($sysxml, $xslfilename, $lang) if they are not passed to the subroutine. This happens from catalogue/detail.pl, opac/opac-shelves.pl, opac/opac-tags.pl and virtualshelves/shelves.pl. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 49504 [details] [review] Bug 15263: Bug 15263: (follow-up 2) XSLT display fetches sysprefs for every result processed Don't retrieve prefs if we won't need them Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 49505 [details] [review] Bug 15263: (QA followup) Make *shelves.pl use the new API This patch makes the lists work as the search results for rendering on XSLT-driven context. No behaviour change is expected. To test: - Apply the patch - Navigate lists (OPAC and intranet) => SUCCESS: the only difference is speed (faster) - Sign off :-D
Doesn't opac/opac-tags.pl need to be updated for the new API as well?
I also see older style calls in catalogue/detail.pl and opac/opac-detail.pl that using the details style sheet. Should those be updated as well?
Created attachment 50474 [details] [review] Bug 15263: (QA followup) Use the new XSLTParse4Display everywhere Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
The last followup uses OPACXSLTResultsDisplay for the details page, that should probably be OPACXSLTDetailsDisplay?
Created attachment 50506 [details] [review] Bug 15263: (QA followup) Use the new XSLTParse4Display everywhere Edit: fixed catalogue/detail.pl and opac/opac-detail.pl so they use the right XSLT syspref. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 50507 [details] [review] Bug 15263: XSLT display fetches sysprefs for every result processed On search, every single result goes through some XSLT processing. This includes fetching the relevant sysprefs every single time. We should do it only once per search. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 50508 [details] [review] Bug 15263: (follow-up) XSLT display fetches sysprefs for every result processed Set variables ($sysxml, $xslfilename, $lang) if they are not passed to the subroutine. This happens from catalogue/detail.pl, opac/opac-shelves.pl, opac/opac-tags.pl and virtualshelves/shelves.pl. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 50509 [details] [review] Bug 15263: Bug 15263: (follow-up 2) XSLT display fetches sysprefs for every result processed Don't retrieve prefs if we won't need them Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 50510 [details] [review] Bug 15263: (QA followup) Make *shelves.pl use the new API This patch makes the lists work as the search results for rendering on XSLT-driven context. No behaviour change is expected. To test: - Apply the patch - Navigate lists (OPAC and intranet) => SUCCESS: the only difference is speed (faster) - Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 50511 [details] [review] Bug 15263: (QA followup) Use the new XSLTParse4Display everywhere Edit: fixed catalogue/detail.pl and opac/opac-detail.pl so they use the right XSLT syspref. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 16.05, thanks Mirko, Jonathan and Tomas!