Batch Patron Deletion (Trash) has a fairly long run/load time compared to Batch Patron Deletion (Permanent). I have attempted to improve the runtime of the Batch Patron Deletion (Trash) tool and my enhancement reduces the time to modify a batch of patrons by an average of 78.21% I discovered the extended runtime was mostly due to the overhead DBIx::Class::ResultSet which I switched to basic SQL calls to the $dbh which reduced the trashing of 1000 patrons from 24.84 seconds to 5.41 seconds, saving 19.43 seconds of resultset calls.
Created attachment 44447 [details] [review] Bug 15107: Batch Patron Deletion (Trash) Performance Improvement Batch Patron Modification has a fairly long run/load time. I have attempted to improve the runtime of the Batch Patron Modification tool by switching out more expensive resultset calls for faster more direct SQL queries. Test Plan: 1) Prepare NTYprof (http://wiki.koha-community.org/wiki/Profiling_with_Devel::NYTProf) or procure a stopwatch (stopwatch will be simpler but less accurate). 2) Ensure you have a decent number of patrons in your Koha system (>500) if you are lacking patrons to modify in your database you can import 1000 using the PatronDataCSV.csv attachement and the Import Patrons tool before moving to step 3. 4) Navigate to Home > Tools > Batch Patrons Deletion in the Koha Intranet 5) Check the box beside the "Verify you want to delete patrons" box 6) Under "who have not borrowed since" enter todays date 7) You should get a message saying: "1000 patrons will be deleted 0 checkout history will be anonymized" If not please go back to step 2 and import patrons using the PatronDataCSV.csv 8) Select the radio button option beside "Move these patrons to the trash" 9) If you are using a stopwatch, prepare your stopwatch so that you will start counting seconds from the point you click the button in the next step. 10) Click the "Finsh" button (simultaneously start your stopwatch if using one) 11) When the page appears showing completion of the change, stop your stopwatch and check the time or navigate to the folder you have set NYTProf to output. 12) Record the runtime. This is the pre-optimisation time. 13) Apply this patch. 14) Repeat steps 2-12 of this testplan with the patch applied. This will yield the post-optimisation time. 15) Compare the pre-optimisation time and post optimisation time. The second post-optimisation time should be faster.
Created attachment 44448 [details] Patron data to import for testing
Comment on attachment 44447 [details] [review] Bug 15107: Batch Patron Deletion (Trash) Performance Improvement Review of attachment 44447 [details] [review]: ----------------------------------------------------------------- Great to see someone working on performance! I realise that dropping DBIC, especially in background scripts where a startup cost is clearly a bad thing, does lead to a performance increase but I am a little apprehensive to do an about turn on dbic's use as it's only jsut picking up momentum in the project as a whole. i wonder if there is a dbic friendly way to improve performance rather than just ditching it? ::: C4/Members.pm @@ +1772,3 @@ > =cut > > sub MoveMemberToDeleted { Seems you've highlighted a bad API choice here (not your fault). This sub does not 'Move' at all.. it 'Copies'! I would prefer to see it actually do a 'Move', i.e. do the insert from select, followed by a delete wrapped in a transaction. Would you be willing to look into this as part of this piece? @@ +1773,5 @@ > > sub MoveMemberToDeleted { > + my ($borrowernumber) = shift or return; > + my $dbh = C4::Context->dbh; > + my $borrower_columns = q{ I'm not especially happy with adding a list of fields here, it means increased maintenance cost going forward as would require this script to be altered upon any db change on this table.
Comment on attachment 44447 [details] [review] Bug 15107: Batch Patron Deletion (Trash) Performance Improvement Review of attachment 44447 [details] [review]: ----------------------------------------------------------------- ::: C4/Members.pm @@ +1772,4 @@ > =cut > > sub MoveMemberToDeleted { > + my ($borrowernumber) = shift or return; I missed this one the first time around.. Have you actually checked that all calls to MoveMoverToDeleted are actually passing a borrowernumber int as a posed to a member hashref?
Hi Catherine, What sort of appetite do you have for expanding the scope of this as hinted above? I feel koha is in a somewhat transitional stage at the moment regarding performance/coding standards.. I certainly don't wan't to put you off this important work but would like to see some alternatives that actually attempt to fix up the api's rather than make 'fixes' on top of old design decisions. For instance, the 'Move' routine not being a Move at all, but rather a Copy.. this is leading to bugs elsewhere. Also, the routine not being clear as to whether it requires a borrowernumber or a member hashref is likely to lead to bugs. I also think there could be an improvement to allow for passing lists of borrowernumber as a posed to single borrowers which could lead to performance gains too. This is all skirting the issue of data integrity and the lack of good db constraints meaning that objects could well become orphans after running this routine :( Any thoughts?
I seem to be on a comment mission today, sorry ;) The routine you've optimised is called in three places: misc/cronjobs/delete_patrons.pl tools/cleanborrowers.pl members/deletemem.pl Could you verify there are no adverse effects resulting from these changes for those scripts.. and perhaps profile profiles for running with and without plack as this will effect the performance metrics greatly. Pretty please :)
I agree with Martin. I would prefer we don't move backwards. If we can find a way to improve the speed while retaining DBIC that would be best. Maybe we could cache the result set as a package variable ( use vars )? Or maybe re-engineer the subroutine to accept a list of patrons to copy? I also think Martin is correct in that it would be great if you would be willing to fix the semantic issues with the code.
Comment on attachment 44447 [details] [review] Bug 15107: Batch Patron Deletion (Trash) Performance Improvement Review of attachment 44447 [details] [review]: ----------------------------------------------------------------- While DBIx assures DB agnosticism, all this code is really ANSI stuff. I'm personally torn between speed and eventual consistency (keeping DBIx across the board). As for Martin's comment, perhaps DBIx::CopyRecord? I have no idea. ::: C4/Members.pm @@ +1788,5 @@ > + cardnumber, branchcode, categorycode, sort1, sort2, > + dateenrolled, dateexpiry, opacnote, borrowernotes, userid, > + password, flags, gonenoaddress, lost, debarred, > + debarredcomment, smsalertnumber, privacy > + }; This would be better built with C4::Member::columns()
Shame, this one seems to have lost its head of steam :-(
We are moving our code to the Koha namespace using DBIx::Class objects, I am not sure it's a good idea to go back and use DBI directly. See bug 16891 and bug 16907.