Description
Fridolin Somers
2018-10-12 12:07:57 UTC
Created attachment 80470 [details] [review] Bug 21560: report ODS export optimization SQL reports can be exported with CSV, TSV of ODS format. When report has thousands of rows, using ODS format is around 10 times longer than other formats. It also loads CPU and RAM a lot. The longest call is expandTable(). I found that the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan : 1) Don't apply patch yet 2) Create a new SQL report : Home > Reports > Create from SQL 3) Enter a SQL that will return thousands of results 4) Run report 5) Click Download > Semicolon separated text (.csv), look execution time 6) Click Download > Open Document Spreadsheet, look execution time 7) Apply patch 8) Redo 5) and 6) and compare times, CSV export should be the same but ODS export should be better My tests shows 1,5 seconds for CSV export. And for ODS export : 18 seconds without patch and 8 seconds with patch. Tested on BibLibre sandbox six with two different reports - these were selected randomly. Records with item count (https://wiki.koha-community.org/wiki/SQL_Reports_Library#Records_with_item_count): 4843 results; csv - about 3 seconds; ods - about 20 seconds before patch and 9 seconds after patch applied. Accession Register Sorted by Barcode Number Report (https://wiki.koha-community.org/wiki/SQL_Reports_Library#Accession_Register_Sorted_by_Barcode_Number_Report): 6015 results; csv - about 2.5 seconds; ods - about 30 seconds before patch and 13 seconds after patch applied. Patch tested with a sandbox, by David Nind <david@davidnind.com> Created attachment 80565 [details] [review] Bug 21560: report ODS export optimization SQL reports can be exported with CSV, TSV of ODS format. When report has thousands of rows, using ODS format is around 10 times longer than other formats. It also loads CPU and RAM a lot. The longest call is expandTable(). I found that the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan : 1) Don't apply patch yet 2) Create a new SQL report : Home > Reports > Create from SQL 3) Enter a SQL that will return thousands of results 4) Run report 5) Click Download > Semicolon separated text (.csv), look execution time 6) Click Download > Open Document Spreadsheet, look execution time 7) Apply patch 8) Redo 5) and 6) and compare times, CSV export should be the same but ODS export should be better My tests shows 1,5 seconds for CSV export. And for ODS export : 18 seconds without patch and 8 seconds with patch. Signed-off-by: David Nind <david@davidnind.com> Fridolin, I think it would make sense to fix misc/cronjobs/gather_print_notices.pl as well on the same bug report. Created attachment 80683 [details] [review] Bug 21560: report ODS export optimization SQL reports can be exported with CSV, TSV of ODS format. When report has thousands of rows, using ODS format is around 10 times longer than other formats. It also loads CPU and RAM a lot. The longest call is expandTable(). I found that the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan : 1) Don't apply patch yet 2) Create a new SQL report : Home > Reports > Create from SQL 3) Enter a SQL that will return thousands of results 4) Run report 5) Click Download > Semicolon separated text (.csv), look execution time 6) Click Download > Open Document Spreadsheet, look execution time 7) Apply patch 8) Redo 5) and 6) and compare times, CSV export should be the same but ODS export should be better My tests shows 1,5 seconds for CSV export. And for ODS export : 18 seconds without patch and 8 seconds with patch. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> (In reply to Jonathan Druart from comment #5) > Fridolin, I think it would make sense to fix > misc/cronjobs/gather_print_notices.pl as well on the same bug report. Indeed, I will create a second patch Created attachment 82951 [details] [review] Bug 21560: Optimize ODS export of gather_print_notices.pl Like previous patch, the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan inspired from Bug 11679. Test plan: - define your ODUE notice for the print template as: cardnumber:patron:email:item <<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item> - define overdues rules for a patron category - check-out 2 items using a due date in order to generate the overdue notices - check-in these 2 items - launch the script /misc/cronjobs/overdue_notices.pl - the 'message_queue' table should now contain 2 new entries - launch the gather_print_notices cronjob : perl misc/cronjobs/gather_print_notices.pl /tmp/test --ods --letter_code=OVERDUE -d=: - A ods file should be generated in your /tmp/test directory - Compare times with and without patch Created attachment 84258 [details] [review] Bug 21560: report ODS export optimization SQL reports can be exported with CSV, TSV of ODS format. When report has thousands of rows, using ODS format is around 10 times longer than other formats. It also loads CPU and RAM a lot. The longest call is expandTable(). I found that the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan : 1) Don't apply patch yet 2) Create a new SQL report : Home > Reports > Create from SQL 3) Enter a SQL that will return thousands of results 4) Run report 5) Click Download > Semicolon separated text (.csv), look execution time 6) Click Download > Open Document Spreadsheet, look execution time 7) Apply patch 8) Redo 5) and 6) and compare times, CSV export should be the same but ODS export should be better My tests shows 1,5 seconds for CSV export. And for ODS export : 18 seconds without patch and 8 seconds with patch. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: David Nind <david@davidnind.com> Created attachment 84259 [details] [review] Bug 21560: Optimize ODS export of gather_print_notices.pl Like previous patch, the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan inspired from Bug 11679. Test plan: - define your ODUE notice for the print template as: cardnumber:patron:email:item <<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item> - define overdues rules for a patron category - check-out 2 items using a due date in order to generate the overdue notices - check-in these 2 items - launch the script /misc/cronjobs/overdue_notices.pl - the 'message_queue' table should now contain 2 new entries - launch the gather_print_notices cronjob : perl misc/cronjobs/gather_print_notices.pl /tmp/test --ods --letter_code=OVERDUE -d=: - A ods file should be generated in your /tmp/test directory - Compare times with and without patch Signed-off-by: David Nind <david@davidnind.com> An updated test plan for the second patch (Optimize ODS export of gather_print_notices.pl) Test plan --------- Pre-requisites: To test the gather_print_notices.pl patch you need to setup some overdue items to generate overdue notices. The basic steps to do this are included in the test plan. 1) Define your ODUE notice for the print template (Tools > Patrons and circulation > Notices & slips > edit the ODUE notice > expand the Print section): cardnumber:patron:email:item <<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item> 2) Define the overdue rules for a patron category, for example: . Check that 'Overdue notice required' is set to 'Yes' for the patron category (Administration > Patrons and circulation > Patron categories) . Check that the 'Loan period' for the 'Standard rules all libraries' is set to 5 days (Administration > Patrons and circulation > Circulation and fine rules) . Check that a trigger is set for the patron category, for example: Delay = 1, Letter = Overdue Notice, Print = ticked (Tools > Patrons and circulation > Overview notice/status triggers) 3) Check out 2 items for a patron in the same patron category defined in step 2 - set the due date so that it will generate the overdue notices. (Circulation > Check out > enter patron details (card number or partial name) > enter a barcode for an item > specifiy a due date by expanding 'Checkout settings' and entering a date at least 5 days before the current date > Check out > confirm check out.) 4) Run the overdue notices cronjob from the command line: perl misc/cronjobs/overdue_notices.pl 5) Check that the 'message_queue' table in the database contains 2 new entries (from the command line 'koha-mysql kohadev' and then enter the query 'select * from message_queue;') 6) Run the gather_print_notices cronjob from the command line and record how long it takes to run (use the time command* if available to record this): time perl misc/cronjobs/gather_print_notices.pl /tmp --ods --letter_code=ODUE -d=: 7) An ods file should be generated in the /tmp directory 8) Apply the patch 9) Run step 6 again to get the time after the patch is applied * If using the bash shell and the time package is installed you can record how long a script takes to run. Result before the patch was applied: real 0m1.702s user 0m1.336s sys 0m0.172s Result after the patch was applied: real 0m1.541s user 0m1.404s sys 0m0.132s Note: got several different times, but all were quicker. Created attachment 84693 [details] [review] Bug 21560: report ODS export optimization SQL reports can be exported with CSV, TSV of ODS format. When report has thousands of rows, using ODS format is around 10 times longer than other formats. It also loads CPU and RAM a lot. The longest call is expandTable(). I found that the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan : 1) Don't apply patch yet 2) Create a new SQL report : Home > Reports > Create from SQL 3) Enter a SQL that will return thousands of results 4) Run report 5) Click Download > Semicolon separated text (.csv), look execution time 6) Click Download > Open Document Spreadsheet, look execution time 7) Apply patch 8) Redo 5) and 6) and compare times, CSV export should be the same but ODS export should be better My tests shows 1,5 seconds for CSV export. And for ODS export : 18 seconds without patch and 8 seconds with patch. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 84694 [details] [review] Bug 21560: Optimize ODS export of gather_print_notices.pl Like previous patch, the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan inspired from Bug 11679. Test plan: - define your ODUE notice for the print template as: cardnumber:patron:email:item <<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item> - define overdues rules for a patron category - check-out 2 items using a due date in order to generate the overdue notices - check-in these 2 items - launch the script /misc/cronjobs/overdue_notices.pl - the 'message_queue' table should now contain 2 new entries - launch the gather_print_notices cronjob : perl misc/cronjobs/gather_print_notices.pl /tmp/test --ods --letter_code=OVERDUE -d=: - A ods file should be generated in your /tmp/test directory - Compare times with and without patch Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice little optimization, passing QA scripts and no regressions found.. Passed QA This seems to be the same code in two scripts - any chance we could move this to a subroutine? (In reply to Nick Clemens from comment #15) > This seems to be the same code in two scripts - any chance we could move > this to a subroutine? Good idea. But can this be pushed and I create a new Bug for a subroutine ? I'm on a subroutine Created attachment 85264 [details] [review] Bug 21560: create Koha::Util::OpenDocument with subroutine for ODS generation There is nearly the same code in misc/cronjobs/gather_print_notices.pl and reports/guided_reports.pl. This patch creates a new module with subroutine called by both scripts. If file path or content is undefined, subroutine will just return undef. If content is an empty arrayref, empty ods file is still generated. Unicode encoding is kept outisde because it may not be necessary. Note that for print notices the first line of message is always the column names so it is extracted only from first message. Test plan : Run previous patches test plans and check ODS files are the same. Created attachment 85265 [details] [review] Bug 21560: Unit test for Koha::Util::OpenDocument Run prove t/Koha/Util/OpenDocument.t Should this be considered a duplicate of Bug 13847? (In reply to Owen Leonard from comment #20) > Should this be considered a duplicate of Bug 13847? I think yes Bug 13847 can be closed as duplicate of this one *** Bug 13847 has been marked as a duplicate of this bug. *** Comment on attachment 85264 [details] [review] Bug 21560: create Koha::Util::OpenDocument with subroutine for ODS generation Review of attachment 85264 [details] [review]: ----------------------------------------------------------------- Generally, this is a nice improvement and refactors thanks.. one tiny remark on two lines and I think we can push this over the finish line. ::: misc/cronjobs/gather_print_notices.pl @@ +275,5 @@ > } > } > + > + # Process > + use Koha::Util::OpenDocument; I realise the script has inlined the use statements previously, but any chance we could use this opportunity to tidy up a little and put this at the top of the file for consistency.. I nearly failed the patch thinking it was missing. ::: reports/guided_reports.pl @@ +938,4 @@ > } > > + # Process > + use Koha::Util::OpenDocument; Another case of can we move the use statement to the top. > I realise the script has inlined the use statements previously, but any
> chance we could use this opportunity to tidy up a little and put this at the
> top of the file for consistency.. I nearly failed the patch thinking it was
> missing.
OK I'm on it
Created attachment 86084 [details] [review] Bug 21560: (follow-up) move use at the top Created attachment 86515 [details] [review] Bug 21560: report ODS export optimization SQL reports can be exported with CSV, TSV of ODS format. When report has thousands of rows, using ODS format is around 10 times longer than other formats. It also loads CPU and RAM a lot. The longest call is expandTable(). I found that the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan : 1) Don't apply patch yet 2) Create a new SQL report : Home > Reports > Create from SQL 3) Enter a SQL that will return thousands of results 4) Run report 5) Click Download > Semicolon separated text (.csv), look execution time 6) Click Download > Open Document Spreadsheet, look execution time 7) Apply patch 8) Redo 5) and 6) and compare times, CSV export should be the same but ODS export should be better My tests shows 1,5 seconds for CSV export. And for ODS export : 18 seconds without patch and 8 seconds with patch. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86516 [details] [review] Bug 21560: Optimize ODS export of gather_print_notices.pl Like previous patch, the call on OpenOffice-OODoc lib can be optimized. Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table Test plan inspired from Bug 11679. Test plan: - define your ODUE notice for the print template as: cardnumber:patron:email:item <<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item> - define overdues rules for a patron category - check-out 2 items using a due date in order to generate the overdue notices - check-in these 2 items - launch the script /misc/cronjobs/overdue_notices.pl - the 'message_queue' table should now contain 2 new entries - launch the gather_print_notices cronjob : perl misc/cronjobs/gather_print_notices.pl /tmp/test --ods --letter_code=OVERDUE -d=: - A ods file should be generated in your /tmp/test directory - Compare times with and without patch Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86517 [details] [review] Bug 21560: create Koha::Util::OpenDocument with subroutine for ODS generation There is nearly the same code in misc/cronjobs/gather_print_notices.pl and reports/guided_reports.pl. This patch creates a new module with subroutine called by both scripts. If file path or content is undefined, subroutine will just return undef. If content is an empty arrayref, empty ods file is still generated. Unicode encoding is kept outisde because it may not be necessary. Note that for print notices the first line of message is always the column names so it is extracted only from first message. Test plan : Run previous patches test plans and check ODS files are the same. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86518 [details] [review] Bug 21560: Unit test for Koha::Util::OpenDocument Run prove t/Koha/Util/OpenDocument.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 86519 [details] [review] Bug 21560: (follow-up) move use at the top Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice lot of tidying and optimising here.. going back to PQA after the cleanups.. Thanks Fridolin! Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.04 backported to 18.05.x for 18.05.11 Pushed to 17.11.x for 17.11.17 |