Created attachment 52264 [details] [review] Bug 16166: Proposed followup to use Sereal for serialization This manages to eke out a bit more performance on my machine.
% more t.pl use Koha::Cache; my $t; for my $i ( 1 .. 3 ) { for my $j ( 1 .. 100000 ) { $t->{$i}{$j} = "$i$j"; } } for ( 1 .. 10 ) { my $cache = Koha::Cache->get_instance; $cache->set_in_cache( 'my_t', $t ); $cache->get_from_cache( $t ); } % git checkout master % time perl t.pl perl t.pl 4.76s user 0.11s system 100% cpu 4.875 total % git checkout bug_16166 ( Bug 16166: Improve L1 cache performance and safety + Bug 16166: Fix t/Cache.t tests ) % time perl t.pl perl t.pl 2.28s user 0.14s system 100% cpu 2.423 total % git checkout bug_16715 ( Bug 16166: Proposed followup to use Sereal for serialization ) % time perl t.pl perl t.pl 1.75s user 0.05s system 99% cpu 1.803 total Note that I did not manage to prove the gain using Koha, I have tried an intranet search with 100 results (xslt) and the execution time is always between 13s and 13.5s. I did not notice a significant gain.
Jacek, could you have a loot at this patch please?
(In reply to Jonathan Druart from comment #3) > Jacek, could you have a loot at this patch please? Sounds kinda Freudian, but hey - why not :)
(In reply to Jonathan Druart from comment #2) > Note that I did not manage to prove the gain using Koha, I have tried an > intranet search with 100 results (xslt) and the execution time is always > between 13s and 13.5s. I did not notice a significant gain. httping -c 40 -i 0.01 'http://koha.wherever/cgi-bin/koha/opac-search.pl?q=unix' (OPACnumSearchResults = 100, XSLT enabled, CGI + memcached): - master: 40 connects, 40 ok, 0.00% failed round-trip min/avg/max = 5911.8/6032.2/6215.6 ms - master + Bug 16166 round-trip min/avg/max = 5535.5/5626.0/5846.7 ms (6.7% gain) - master + Bug 16166 + Bug 16715 round-trip min/avg/max = 5488.9/5521.9/5761.8 ms (8.5% gain) - master + Bug 16166 + Bug 16715 + Bug 16365 round-trip min/avg/max = 3572.2/3719.5/3780.2 ms (38.3% gain) httping -c 40 -i 0.01 'http://koha.wherever/cgi-bin/koha/opac-search.pl?q=asfasdffghj' (0 search results - same setup, i.e. CGI + memcached) - master round-trip min/avg/max = 879.2/912.8/1107.5 ms - master + Bug 16166 + Bug 16715 + Bug 16365 round-trip min/avg/max = 886.3/916.5/1009.8 ms
(In reply to Jonathan Druart from comment #2) > > for ( 1 .. 10 ) { > my $cache = Koha::Cache->get_instance; > $cache->set_in_cache( 'my_t', $t ); > $cache->get_from_cache( $t ); > } This test looks a bit flawed, a least in the ->get_from_cache() part, I replaced this part with my $cache = Koha::Cache->get_instance; $cache->set_in_cache( 'my_t', $t ); for ( 1 .. 100 ) { $cache->get_from_cache( 'my_t' ); } to make interpretation of the results somehow easier.
Sereal encoder is much better (2-3x) then freeze() - both speed- and sizewise (especially with "snappy" compression enabled), for all data structure types, big and small. Sereal decoder is also much faster (up to 2x) then thaw() for small and medium-sized data structures. For a very big structures, it's still faster then thaw(), but speed difference is less pronounced (e.g. for a MARC framework: sereal decode takes ~7 msec, and thaw - 8.5 msec). But it's hardly deserializers fault, most of that time is wasted in perl internals on hash keys and SVs creation. Note that destroing / garbage collection of all those hashes-of-hashes also takes significant time (3-4 msec for a MARC framework). The only drawback I see is the lack of packaged version for Wheezy..
Created attachment 52307 [details] [review] Bug 16166: Proposed followup to use Sereal for serialization This manages to eke out a bit more performance on my machine. https://bugs.koha-community.org/show_bug.cgi?id=16715 Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Created attachment 52334 [details] [review] Bug 16715: Proposed followup to use Sereal for serialization This manages to eke out a bit more performance on my machine. Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I believe patch will require https://packages.debian.org/sid/perl/libsereal-perl to be added to the community repo.
Pushed to master for 16.11, thanks Jesse!
Cancel that, will need a rebase.
Created attachment 53592 [details] [review] Bug 16715: Proposed followup to use Sereal for serialization This manages to eke out a bit more performance on my machine. Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Kyle M Hall from comment #12) > Cancel that, will need a rebase. Conflict in C4/Installer/PerlDependencies.pm fixed!
Just a note: Without Sereal, staff client crashed with a message like "missing Sereal" before it could execute C4/Installer/PerlDependencies.pm
(In reply to Marc Véron from comment #16) > Just a note: > Without Sereal, staff client crashed with a message like "missing Sereal" > before it could execute C4/Installer/PerlDependencies.pm Indeed, but I don't think we should try to avoid that. It's part of the upgrade job to check if all deps are installed.
It looks like it needs libsereal-perl libsereal-encoder-perl libsereal-decoder-perl The first is not in Jessie, the others are. So I wonder if it's necessary to get libsereal-perl backported or if it's enough to use the encoder and decoder directly. Jonathan?
> This Sereal module is a very thin wrapper around both Sereal::Encoder > and Sereal::Decoder. It depends on both and loads both. So if you have > a user of both encoder and decoder, it is enough to depend on a particular > version of Sereal and you'll get the most recent released versions of > Sereal::Encoder and Sereal::Decoder. If that is all it does, it does not make much sense for Debian, IMHO. The real modules are there, just the wrapper isn't. I don't see a benefit of backporting this.
Oh, I see that the module is experimental (or am I missing something?): http://search.cpan.org/~yves/Sereal-0.330/lib/Sereal.pm DESCRIPTION This is an experimental module. Before using it in production, please get in touch with the authors!
(In reply to Marc Véron from comment #21) > Oh, I see that the module is experimental (or am I missing something?): > http://search.cpan.org/~yves/Sereal-0.330/lib/Sereal.pm Not, it's http://search.cpan.org/~yves/Sereal-3.014/lib/Sereal.pm
(In reply to Mirko Tietgen from comment #20) > > This Sereal module is a very thin wrapper around both Sereal::Encoder > > and Sereal::Decoder. It depends on both and loads both. So if you have > > a user of both encoder and decoder, it is enough to depend on a particular > > version of Sereal and you'll get the most recent released versions of > > Sereal::Encoder and Sereal::Decoder. > > If that is all it does, it does not make much sense for Debian, IMHO. The > real modules are there, just the wrapper isn't. I don't see a benefit of > backporting this. I have quickly tried this diff --git a/Koha/Cache.pm b/Koha/Cache.pm index 0f85ed1..fa4f136 100644 --- a/Koha/Cache.pm +++ b/Koha/Cache.pm @@ -40,7 +40,8 @@ use warnings; use Carp; use Module::Load::Conditional qw(can_load); use Koha::Cache::Object; -use Sereal; +use Sereal::Encoder; +use Sereal::Decoder; use base qw(Class::Accessor); And it seems to work correctly. I can provide a patch if you need it.
Excellent! Can you also change PerlDependencies.pm please? Thanks!
Yep we definitely need a patch, at least for jessie, libsereal-perl is in stretch, but it won't be backported for jessie.
(In reply to Jonathan Druart from comment #23) > > I have quickly tried this > > diff --git a/Koha/Cache.pm b/Koha/Cache.pm > index 0f85ed1..fa4f136 100644 > --- a/Koha/Cache.pm > +++ b/Koha/Cache.pm > @@ -40,7 +40,8 @@ use warnings; > use Carp; > use Module::Load::Conditional qw(can_load); > use Koha::Cache::Object; > -use Sereal; > +use Sereal::Encoder; > +use Sereal::Decoder; > > use base qw(Class::Accessor); > > > And it seems to work correctly. > I can provide a patch if you need it. That should work just fine, I used that variant for speed differences comparisons vs Storable (I think it was on jessie, not entirely sure) without encountering any problems. Speaking of dependencies: what about some other files (debian/control*, install_misc/*.packages) in the source tree which are referencing required packages? I don't know if they may need some tweaking as well or not - or maybe it's only being done en masse before releasing next packaged version?
Created attachment 53829 [details] [review] Bug 16715: Use Sereal::Decoder and Sereal::Encoder instead of Sereal Sereal is not packaged for jessie, so let's use Sereal::Encoder and Sereal::Decoder instead.
Second patch is waiting for SO, QA then push!
(In reply to Jonathan Druart from comment #28) > Second patch is waiting for SO, QA then push! I did the exact same patch locally for the latest master package (and would have sent it today). I'll send a signoff in a few minutes.
Created attachment 53830 [details] [review] Bug 16715: Use Sereal::Decoder and Sereal::Encoder instead of Sereal Sereal is not packaged for jessie, so let's use Sereal::Encoder and Sereal::Decoder instead. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Created attachment 53861 [details] [review] [PASSED QA] Bug 16715: Use Sereal::Decoder and Sereal::Encoder instead of Sereal Sereal is not packaged for jessie, so let's use Sereal::Encoder and Sereal::Decoder instead. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Followup has been pushed to master for Koha 16.11, thanks Jonathan!
Ubuntu 14.04 has libsereal-encoder-perl and libsereal-decoder-perl 2.03. Can we lower the dependency min versions?