Bug 8017 - Remove unnecessary processing on return of GetAllIssues
Summary: Remove unnecessary processing on return of GetAllIssues
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.8
Hardware: All All
: P4 minor (vote)
Assignee: Colin Campbell
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-27 16:21 UTC by Colin Campbell
Modified: 2013-12-05 20:01 UTC (History)
3 users (show)

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


Attachments
Proposed patch (21.41 KB, patch)
2012-04-27 17:37 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 8017 reduce manipulation of GetAllIssues return (21.53 KB, patch)
2012-05-13 14:13 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Proposed Amended Patch (21.53 KB, patch)
2012-06-11 12:38 UTC, Colin Campbell
Details | Diff | Splinter Review
Patch rebased against current head (21.48 KB, patch)
2012-08-03 14:18 UTC, Colin Campbell
Details | Diff | Splinter Review
Patch rebased against current head (23.27 KB, patch)
2012-09-12 17:58 UTC, Colin Campbell
Details | Diff | Splinter Review
Patch - Version for 3.8 (21.47 KB, patch)
2012-09-14 08:31 UTC, Colin Campbell
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2012-04-27 16:21:45 UTC
This is potentially a large list used in staff and opac to show a user's circulation history. The routine itself makes an unnecessary iteration over the list to do an inefficient copy and the consumers of the list also loop over and copy the list before the final loop perdormed in instantiating the template.

Move unnecessary processing out of the loops (e.g. format dates using the tt custom plugin dont format dates we are not displaying). 

Don't increase the performabce hit of the routine
Comment 1 Colin Campbell 2012-04-27 17:37:54 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-05-13 14:13:09 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2012-05-22 08:09:03 UTC
QA Comments:

1/ You remove the FIXME in getAllIssues (sanity-check order and limit) but $order is not sanitized. Please call $dbh->quote with the $order variable.

2/ Why do you remove the parenthesis for the IF statement (template) ? It seems mandatory by the koha guidelines. But I agree with you, it is useless :)

Failed QA for 1.

Replacement of fetchrow_hashref with a more adapted fetchall_arrayref is good. I think there is others occurrences of this into the koha code.
Comment 4 Colin Campbell 2012-06-11 12:38:15 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2012-08-03 13:15:01 UTC
This patch seems already signed off. But it does not apply. Could you resubmit a rebased patch please ?
Comment 6 Colin Campbell 2012-08-03 14:18:44 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2012-08-03 14:56:21 UTC
Thanks Colin !

I did not find any regression.

So I mark it as Passed QA.
Comment 8 Paul Poulain 2012-08-31 15:29:54 UTC
I realize that this patch modify a SELECT * that is LARGE as it's a JOIN on issues / biblio / items / biblioitems

I agree it was like this before the patch. But Colin, could you improve it to return only needed columns, because that's probably also a source of the perfs problem.

Side note : there is a call to 
+my $branches = GetBranches();
+foreach my $issue ( @{$issues} ) {
+    $issue->{issuingbranch} = $branches->{ $issue->{branchcode} }->{branchname};
 }
That could be replaced by another JOIN (not sure it's a good idea, just sharing)

(not failing QA, the mistake was here before your patch. If you can't provide a follow-up, i'll push like this. But if you agree for a follow-up, it will be cool ;-) )
Comment 9 Colin Campbell 2012-09-01 13:08:15 UTC
I'll take a look at Paul's suggestions The main performance hit was the unnecessary repetitions of loops and in production the patch as is does enough to fix large sites that were having problems displaying circ history.
Comment 10 Paul Poulain 2012-09-07 13:36:19 UTC
Colin = pinging my comment 8
Comment 11 Colin Campbell 2012-09-07 14:24:48 UTC
Paul, Id be happy to see this pushed as is. I think I'll look at what can be done to improve the query as a separate piece of work
Comment 12 Paul Poulain 2012-09-12 15:01:51 UTC
Sorry, but the patch does not apply anymore, because of bug 6494.

Could you rebase & resubmit (conflict not trivial to solve: I feel most of it is tidying code, but to avoid any mistake, I let you do)
Comment 13 Colin Campbell 2012-09-12 17:58:30 UTC
Created attachment 12177 [details] [review]
Patch rebased against current head

Rebased to incorporate changes introduced by the opachtmlsummary code
Comment 14 Jonathan Druart 2012-09-13 07:26:28 UTC
switching back to passed QA
Comment 15 Paul Poulain 2012-09-13 16:55:29 UTC
Patch pushed to master.

Could be ported to 3.8 imo
Comment 16 Chris Cormack 2012-09-14 01:40:49 UTC
I would like this in 3.8.x but unfortunately does not apply cleanly


# Changes to be committed:
#
#	modified:   C4/Members.pm
#	modified:   koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt
#	modified:   members/readingrec.pl
#
# Unmerged paths:
#   (use "git add/rm <file>..." as appropriate to mark resolution)
#
#	both modified:      koha-tmpl/opac-tmpl/prog/en/modules/opac-readingrecord.tt
#	both modified:      opac/opac-readingrecord.pl
Comment 17 Colin Campbell 2012-09-14 08:31:50 UTC
Created attachment 12207 [details] [review]
Patch - Version for 3.8

This is a version of patch that should apply to 3.8 - The last rebase was to account for some new functionality in master. This omits updating that non existent code
Comment 18 Chris Cormack 2012-09-15 19:16:10 UTC
Patch contains no string changes, makes some template changes which I think are safe, pushed to 3.8.x, will be in 3.8.5