Created attachment 65208 [details] [review] Bug 18979: Speed up 'valid-templates.t' tests Test plan: Confirm than prove xt/author/valid-templates.t is quicker with this patch. Before I got: Files=1, Tests=840, 21 wallclock secs ( 0.18 usr 0.02 sys + 20.28 cusr 0.32 csys = 20.80 CPU) After: Files=1, Tests=840, 17 wallclock secs ( 0.06 usr 0.01 sys + 21.56 cusr 0.48 csys = 22.11 CPU)
Testing on top of Bug 18055 Testing without patch: Files=1, Tests=838, 40 wallclock secs ( 0.17 usr 0.01 sys + 28.71 cusr 0.73 csys = 29.62 CPU) With patch: Can't locate Parallel/ForkManager.pm (...) you may need to install the Parallel::ForkManager module -> Installing Parallel::ForkManager Can't locate Sys/CPU.pm ->Installing Sys::CPU Re-testing without patch (4 times): Files=1, Tests=838, 33 wallclock secs ( 0.15 usr 0.02 sys + 28.68 cusr 0.38 csys = 29.23 CPU) Files=1, Tests=838, 30 wallclock secs ( 0.14 usr 0.01 sys + 28.08 cusr 0.32 csys = 28.55 CPU) Files=1, Tests=838, 30 wallclock secs ( 0.14 usr 0.01 sys + 28.61 cusr 0.22 csys = 28.98 CPU) Files=1, Tests=838, 31 wallclock secs ( 0.15 usr 0.01 sys + 28.34 cusr 0.27 csys = 28.77 CPU) Testing with patch (4 times): Files=1, Tests=838, 36 wallclock secs ( 0.06 usr 0.00 sys + 31.04 cusr 0.60 csys = 31.70 CPU) Files=1, Tests=838, 34 wallclock secs ( 0.07 usr 0.00 sys + 29.98 cusr 0.53 csys = 30.58 CPU) Files=1, Tests=838, 32 wallclock secs ( 0.06 usr 0.00 sys + 29.53 cusr 0.65 csys = 30.24 CPU) Files=1, Tests=838, 32 wallclock secs ( 0.05 usr 0.01 sys + 29.74 cusr 0.53 csys = 30.33 CPU) Result on my vm: It seems that installing Parallel::ForkManager and Sys::CPU speeded up xt/author/valid-templates.t (31 - 33 wallclock sec instead of 40 sec, without patch) With patch, xt/author/valid-templates.t was comparable or slower (32 - 36 wallclock sec, with patch)
Created attachment 67484 [details] [review] Bug 18979: Speed up 'valid-templates.t' tests Test plan: Confirm than prove xt/author/valid-templates.t is quicker with this patch. Before I got: Files=1, Tests=840, 21 wallclock secs ( 0.18 usr 0.02 sys + 20.28 cusr 0.32 csys = 20.80 CPU) After: Files=1, Tests=840, 17 wallclock secs ( 0.06 usr 0.01 sys + 21.56 cusr 0.48 csys = 22.11 CPU) Signed-off by: David Bourgault <david.bourgault@inlibro.com>
I tested 10 times, before and after. Before it averaged 18.4 wallclock seconds per run After it average dropped slightly to 16.6 wallclock seconds It's a very minor improvement, but there seems to be one.
Files=1, Tests=842, 42 wallclock secs ( 0.20 usr 0.02 sys + 41.23 cusr 0.40 csys = 41.85 CPU) Files=1, Tests=842, 37 wallclock secs ( 0.06 usr 0.01 sys + 42.91 cusr 0.84 csys = 43.82 CPU) Does this justify another dependency ?
(In reply to Marcel de Rooy from comment #5) > Files=1, Tests=842, 42 wallclock secs ( 0.20 usr 0.02 sys + 41.23 cusr > 0.40 csys = 41.85 CPU) > > Files=1, Tests=842, 37 wallclock secs ( 0.06 usr 0.01 sys + 42.91 cusr > 0.84 csys = 43.82 CPU) > > Does this justify another dependency ? The dependency is used by bug 18055 as well, and we gain 10min
About the "use threads;" line: From the perl doc: The "interpreter-based threads" provided by Perl are not the fast, lightweight system for multitasking that one might expect or hope for. Threads are implemented in a way that make them easy to misuse. Few people know how to use them correctly or will be able to provide help. The use of interpreter-based threads in perl is officially discouraged. Do we really need it ?
Moving to Discussion for feedback. Feel free to move back later..
(In reply to Marcel de Rooy from comment #7) > About the "use threads;" line: > > Do we really need it ? hi Marcel, i think it's not needed :) i must have forgotten to remove the line before sending, i'll fix that now...
a little update here... it turns out that the 'use threads' line was needed for patch to succeed, after all the 'use threads' line seems to handle a known bug where Test::Simple generates the TAP tests out of order, and causes the tests to fail the good news is that there are some options... 1/ use the Test2::Suite module, which handles parallel tests well... https://metacpan.org/pod/Test2::Suite the only catch here is that it's new-ish, so we'll need to backport the module to deb8/jessie too 2/ just allow the 'use threads' line. Because no threads are ever actually created, it should be pretty safe fyi: i did try using forks.pm too, but sadly got lots of random fails
Created attachment 69150 [details] [review] Bug 18979: Swap threads for Test2::Suite this patch swaps threads for the Test2::Suite module to test... 1/ apply patch 2/ install Test2::Suite $ sudo apt-get install libtest2-suite-perl , or... $ cpanm Test2::Suite 3/ run test $ sudo koha-shell -c " export PERL5LIB=/home/mason/git/koha/master; prove -v xt/author/valid-templates.t" kmaster ok 1 - Testing opac bootstrap templates { ... } ok 2 - Testing staff prog templates { ... } 1..2 ok All tests successful. Result: PASS 4. note test passes Result: PASS
(In reply to Mason James from comment #10) > a little update here... > it turns out that the 'use threads' line was needed for patch to succeed, > after all > > the 'use threads' line seems to handle a known bug where Test::Simple > generates the TAP tests out of order, and causes the tests to fail > > > the good news is that there are some options... > > 1/ use the Test2::Suite module, which handles parallel tests well... > https://metacpan.org/pod/Test2::Suite > > the only catch here is that it's new-ish, so we'll need to backport the > module to deb8/jessie too > > 2/ just allow the 'use threads' line. > Because no threads are ever actually created, it should be pretty safe > > > fyi: i did try using forks.pm too, but sadly got lots of random fails Mason, Which option has your preference ? Marcel
(In reply to Marcel de Rooy from comment #12) > (In reply to Mason James from comment #10) > > a little update here... > > it turns out that the 'use threads' line was needed for patch to succeed, > > after all > > > > the 'use threads' line seems to handle a known bug where Test::Simple > > generates the TAP tests out of order, and causes the tests to fail > > > > > > the good news is that there are some options... > > > > 1/ use the Test2::Suite module, which handles parallel tests well... > > https://metacpan.org/pod/Test2::Suite > > > > the only catch here is that it's new-ish, so we'll need to backport the > > module to deb8/jessie too > > > > 2/ just allow the 'use threads' line. > > Because no threads are ever actually created, it should be pretty safe > > > > > > fyi: i did try using forks.pm too, but sadly got lots of random fails > > Mason, > Which option has your preference ? > > Marcel hiya, i'd vote for option 1/ as quick fix, and then a separate bug for a leisurely switch to Test2::Suite anyone else have an opinion?
> hiya, i'd vote for option 1/ as quick fix, and then a separate bug for a > leisurely switch to Test2::Suite oops, i meant option 2, then a follow-up for option 1 :)
(In reply to Mason James from comment #14) > > hiya, i'd vote for option 1/ as quick fix, and then a separate bug for a > > leisurely switch to Test2::Suite > > oops, i meant option 2, then a follow-up for option 1 :) Marcel, lets go for option 2 here... i'll obsolete the newer Test2::Suite patch, for now ..and set the status back to SIGNED-OFF
Marcel, could you have another look here?
Created attachment 71552 [details] [review] Bug 18979: Speed up 'valid-templates.t' tests Test plan: Confirm than prove xt/author/valid-templates.t is quicker with this patch. Before I got: Files=1, Tests=840, 21 wallclock secs ( 0.18 usr 0.02 sys + 20.28 cusr 0.32 csys = 20.80 CPU) After: Files=1, Tests=840, 17 wallclock secs ( 0.06 usr 0.01 sys + 21.56 cusr 0.48 csys = 22.11 CPU) Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Katrin Fischer from comment #16) > Marcel, could you have another look here? I added a forgotten signoff line from David. I have no objection to this patch, but also note that forking makes us unaware of the warnings raised by the tests like: # excluding template /usr/share/koha/devclone/koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc because it cannot stand on its own etc.
Pushed to master for 18.05, thanks to everybody involved!