Bug 14496 - Improving opac-detail.pl performances
Summary: Improving opac-detail.pl performances
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Julian FIOL
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 7923
  Show dependency treegraph
 
Reported: 2015-07-06 13:17 UTC by Julian FIOL
Modified: 2016-12-05 21:22 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14496 : Improving opac-detail.pl performances (2.80 KB, patch)
2015-07-06 13:18 UTC, Julian FIOL
Details | Diff | Splinter Review
[Signed-off] Bug 14496 : Improving opac-detail.pl performances (3.05 KB, patch)
2015-07-09 15:01 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14496 : Improving opac-detail.pl performances (3.12 KB, patch)
2015-07-10 09:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian FIOL 2015-07-06 13:17:09 UTC
Get notes and subjects from MARC record
 ONLY when XSLT is not activated.
    
 It's useless doing it when XSLT is activated,
 because XSLT takes care of it by its own.
    
 => With this patch, we are saving precious
 milliseconds
Comment 1 Julian FIOL 2015-07-06 13:18:39 UTC Comment hidden (obsolete)
Comment 2 Fridolin SOMERS 2015-07-06 15:45:04 UTC
nice job Julian
Comment 3 David Cook 2015-07-07 00:55:35 UTC
I believe the non-XSLT detail display is deprecated or scheduled to be deprecated, no?
Comment 4 Katrin Fischer 2015-07-07 12:47:15 UTC
There were some performance issues raised about the XSLT view - I think we haven't gotten to officially deprecate it afaik. So this should be fine. Might also be worth taking a look how big the performance difference is between both views.
Comment 5 Julian FIOL 2015-07-07 13:30:18 UTC
I've made some measures without XSLT and with the Default one.
My results were :
On opac-detail with XSLT by Default is ~ 4  % longer than without any.
On opac-search with XSLT by Default is ~ 2,5% longer than without any.
Comment 6 Katrin Fischer 2015-07-07 14:07:40 UTC
Thx Julian! - Do you happen to have some times for the page loads to compare too?
Comment 7 Julian FIOL 2015-07-07 14:42:41 UTC
Here are my results (in seconds) :

Opac-details
------------
No XSLT | Default XSLT
11,222  | 12,217
11,836  | 11,334
11,246  | 11,611
11,508  | 12,236
11,288  | 12,055

average 
No XSLT --> Default XSLT
11,42s  --> 11,89s (+470ms)


Opac-search
------------
No XSLT | Default XSLT
3,092   | 3,153
3,322   | 3,248
3,088   | 3,405
3,172   | 3,183
3,119   | 3,205

average 
No XSLT --> Default XSLT
3,15s   --> 3,24s (+90ms)
Comment 8 Marc Véron 2015-07-09 15:01:27 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2015-07-10 09:17:00 UTC
(In reply to David Cook from comment #3)
> I believe the non-XSLT detail display is deprecated or scheduled to be
> deprecated, no?

Yes, and this helps the XSLT view a little bit further.
Comment 10 Marcel de Rooy 2015-07-10 09:17:34 UTC
Created attachment 40914 [details] [review]
Bug 14496 : Improving opac-detail.pl performances

Get notes and subjects from MARC record
ONLY when XSLT is not activated.

It's useless doing it when XSLT is activated,
because XSLT takes care of it by its own.

=> With this patch, we are saving precious
milliseconds

I compared the display of some records in XSLT view with and without patch, was the same (as expected).
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
On a slower server, I saw a time save of 0.0274 to 0.0908 seconds (with XSLT).
Comment 11 Tomás Cohen Arazi 2015-07-10 14:03:05 UTC
Patch pushed to master.

Nice improvement Julian!
Comment 12 Chris Cormack 2015-07-14 20:53:43 UTC
Pushed to 3.20.x will be in 3.20.2
Comment 13 Liz Rea 2015-07-16 12:15:08 UTC
Hi Julian,

We weren't quite done here - still other versions this patch might go into. :)

(So I'm reopening it)

Liz
Comment 14 Julian FIOL 2015-07-16 13:57:56 UTC
Hi Liz,

Ok, my bad :)
But before changing its status to "RESOLVED", it was in "PUSHED TO STABLE".
It may be not necessary changing it to "NEED SIGNOFF", "PUSHED TO STABLE" should be fine ?

Julian F.
Comment 15 Liz Rea 2015-07-16 21:19:01 UTC
Hi,

I don't have the permissions to take it back to that status, but Katrin does! Thanks Katrin!

Cheers,
Liz
Comment 16 Katrin Fischer 2015-07-16 22:26:51 UTC
The problem is that you have to stick to the workflow so from signed off - signed off - passed qa - pushd to stable... :)
Comment 17 Liz Rea 2015-07-20 22:17:16 UTC
Pushed to 3.18.x will be in 3.18.09
Comment 18 Mason James 2015-07-25 13:12:28 UTC
Pushed to 3.16.x, will be in 3.16.13