Bug 15263 - XSLT display fetches sysprefs for every result processed
Summary: XSLT display fetches sysprefs for every result processed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Mirko Tietgen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15262
  Show dependency treegraph
 
Reported: 2015-11-30 12:02 UTC by Mirko Tietgen
Modified: 2016-12-05 21:27 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 15263 XSLT display fetches sysprefs for every result processed (6.19 KB, patch)
2015-11-30 12:07 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 15263 XSLT display fetches sysprefs for every result processed (5.76 KB, patch)
2016-03-10 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15263: (follow-up) XSLT display fetches sysprefs for every result processed (3.00 KB, patch)
2016-03-10 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15263: (follow-up) XSLT display fetches sysprefs for every result processed (3.06 KB, patch)
2016-03-11 14:17 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 15263: Bug 15263: (follow-up 2) XSLT display fetches sysprefs for every result processed (1.06 KB, patch)
2016-03-15 09:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15263: XSLT display fetches sysprefs for every result processed (5.82 KB, patch)
2016-03-24 02:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15263: (follow-up) XSLT display fetches sysprefs for every result processed (3.11 KB, patch)
2016-03-24 02:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15263: Bug 15263: (follow-up 2) XSLT display fetches sysprefs for every result processed (1.12 KB, patch)
2016-03-24 02:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15263: (QA followup) Make *shelves.pl use the new API (3.14 KB, patch)
2016-03-24 03:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15263: (QA followup) Use the new XSLTParse4Display everywhere (4.28 KB, patch)
2016-04-20 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15263: (QA followup) Use the new XSLTParse4Display everywhere (4.37 KB, patch)
2016-04-21 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 15263: XSLT display fetches sysprefs for every result processed (5.87 KB, patch)
2016-04-21 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15263: (follow-up) XSLT display fetches sysprefs for every result processed (3.17 KB, patch)
2016-04-21 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15263: Bug 15263: (follow-up 2) XSLT display fetches sysprefs for every result processed (1.17 KB, patch)
2016-04-21 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15263: (QA followup) Make *shelves.pl use the new API (3.20 KB, patch)
2016-04-21 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15263: (QA followup) Use the new XSLTParse4Display everywhere (4.42 KB, patch)
2016-04-21 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mirko Tietgen 2015-11-30 12:02:16 UTC
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.
Comment 1 Mirko Tietgen 2015-11-30 12:07:55 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-03-10 10:45:55 UTC
I think this patch would become useless with the patch Bug 11998: Add a L1 cache for sysprefs
Comment 3 Jacek Ablewicz 2016-03-10 12:03:06 UTC
(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
Comment 4 Jonathan Druart 2016-03-10 15:02:46 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-03-10 15:02:49 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-03-10 15:03:23 UTC
I have rebased the original patch and added a follow-up to make it works with other calls.
Comment 7 Jonathan Druart 2016-03-10 15:04:30 UTC
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).
Comment 8 Mirko Tietgen 2016-03-11 14:17:48 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2016-03-14 18:17:55 UTC
The functionality looks good. But it breaks t/db_dependent/Search.t
Comment 10 Jonathan Druart 2016-03-15 09:09:38 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2016-03-24 02:56:35 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2016-03-24 02:56:43 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2016-03-24 02:56:53 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2016-03-24 03:36:57 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2016-04-20 14:27:21 UTC
Doesn't opac/opac-tags.pl need to be updated for the new API as well?
Comment 16 Kyle M Hall 2016-04-20 14:30:05 UTC
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?
Comment 17 Tomás Cohen Arazi 2016-04-20 18:09:45 UTC Comment hidden (obsolete)
Comment 18 Mirko Tietgen 2016-04-21 10:17:51 UTC
The last followup uses OPACXSLTResultsDisplay for the details page, that should probably be OPACXSLTDetailsDisplay?
Comment 19 Tomás Cohen Arazi 2016-04-21 13:20:22 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2016-04-21 13:42:27 UTC
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>
Comment 21 Kyle M Hall 2016-04-21 13:42:34 UTC
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>
Comment 22 Kyle M Hall 2016-04-21 13:42:37 UTC
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>
Comment 23 Kyle M Hall 2016-04-21 13:42:40 UTC
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>
Comment 24 Kyle M Hall 2016-04-21 13:42:42 UTC
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>
Comment 25 Kyle M Hall 2016-05-04 13:46:35 UTC
Pushed to master for 16.05, thanks Mirko, Jonathan and Tomas!