Bug 16715 - Koha::Cache - Use Sereal for serialization
Summary: Koha::Cache - Use Sereal for serialization
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jesse Weaver
QA Contact: Testopia
URL:
Keywords: dependency, nowheezy
Depends on: 16166
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-12 07:49 UTC by Jonathan Druart
Modified: 2017-06-14 22:15 UTC (History)
7 users (show)

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


Attachments
Bug 16166: Proposed followup to use Sereal for serialization (2.87 KB, patch)
2016-06-12 07:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16166: Proposed followup to use Sereal for serialization (2.92 KB, patch)
2016-06-13 11:54 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16715: Proposed followup to use Sereal for serialization (2.94 KB, patch)
2016-06-13 16:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16715: Proposed followup to use Sereal for serialization (2.93 KB, patch)
2016-07-21 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16715: Use Sereal::Decoder and Sereal::Encoder instead of Sereal (1.35 KB, patch)
2016-08-01 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16715: Use Sereal::Decoder and Sereal::Encoder instead of Sereal (1.40 KB, patch)
2016-08-01 10:30 UTC, Mirko Tietgen
Details | Diff | Splinter Review
[PASSED QA] Bug 16715: Use Sereal::Decoder and Sereal::Encoder instead of Sereal (1.47 KB, patch)
2016-08-01 21:28 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-06-12 07:49:50 UTC

    
Comment 1 Jonathan Druart 2016-06-12 07:50:44 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-06-12 08:10:15 UTC
% 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.
Comment 3 Jonathan Druart 2016-06-12 08:11:12 UTC
Jacek, could you have a loot at this patch please?
Comment 4 Jacek Ablewicz 2016-06-13 08:29:28 UTC
(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 :)
Comment 5 Jacek Ablewicz 2016-06-13 09:17:58 UTC
(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
Comment 6 Jacek Ablewicz 2016-06-13 11:24:48 UTC
(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.
Comment 7 Jacek Ablewicz 2016-06-13 11:49:32 UTC
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..
Comment 8 Jacek Ablewicz 2016-06-13 11:54:57 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-06-13 16:35:09 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2016-07-08 14:32:24 UTC
I believe patch will require https://packages.debian.org/sid/perl/libsereal-perl to be added to the community repo.
Comment 11 Kyle M Hall 2016-07-08 14:33:15 UTC
Pushed to master for 16.11, thanks Jesse!
Comment 12 Kyle M Hall 2016-07-08 14:39:22 UTC
Cancel that, will need a rebase.
Comment 13 Jonathan Druart 2016-07-21 16:08:22 UTC
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>
Comment 14 Jonathan Druart 2016-07-21 16:09:39 UTC
(In reply to Kyle M Hall from comment #12)
> Cancel that, will need a rebase.

Conflict in C4/Installer/PerlDependencies.pm fixed!
Comment 15 Kyle M Hall 2016-07-22 17:17:58 UTC
Pushed to master for 16.11, thanks Jesse!
Comment 16 Marc Véron 2016-07-23 11:27:33 UTC
Just a note: 
Without Sereal, staff client crashed with a message like "missing Sereal" before it could execute C4/Installer/PerlDependencies.pm
Comment 17 Jonathan Druart 2016-07-25 10:45:07 UTC
(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.
Comment 18 Mirko Tietgen 2016-07-25 13:19:20 UTC
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?
Comment 20 Mirko Tietgen 2016-07-25 13:23:23 UTC
> 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.
Comment 21 Marc Véron 2016-07-25 13:38:15 UTC
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!
Comment 22 Jonathan Druart 2016-07-25 13:59:54 UTC
(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
Comment 23 Jonathan Druart 2016-07-25 14:06:42 UTC
(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.
Comment 24 Mirko Tietgen 2016-07-25 15:06:45 UTC
Excellent! Can you also change PerlDependencies.pm please? Thanks!
Comment 25 Chris Cormack 2016-07-26 04:00:58 UTC
Yep we definitely need a patch, at least for jessie, libsereal-perl is in stretch, but it won't be backported for jessie.
Comment 26 Jacek Ablewicz 2016-07-26 08:04:43 UTC
(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?
Comment 27 Jonathan Druart 2016-08-01 10:16:10 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2016-08-01 10:16:40 UTC
Second patch is waiting for SO, QA then push!
Comment 29 Mirko Tietgen 2016-08-01 10:27:39 UTC
(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.
Comment 30 Mirko Tietgen 2016-08-01 10:30:48 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2016-08-01 21:28:30 UTC
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>
Comment 32 Kyle M Hall 2016-08-02 14:29:56 UTC
Followup has been pushed to master for Koha 16.11, thanks Jonathan!
Comment 33 Mirko Tietgen 2016-11-16 10:03:14 UTC
Ubuntu 14.04 has libsereal-encoder-perl and libsereal-decoder-perl 2.03. Can we lower the dependency min versions?