The last patch requires sign off. The other two have already been signed off.

Created attachment 11400 [details] [review]
Bug 8089: Use Koha::Cache for all caching

1. Replace all instances of memoize_memcached with appropriate calls
into Koha::Cache:
* reports/guided_reports.pl
* C4::Biblio::GetMarcStructure
* C4::Languages::getFrameworkLanguages
* C4::Languages::getAllLanguages
* C4::SQLHelper::GetPrimaryKeys
* C4::SQLHelper::_get_columns

2. Replace all references to memcached with the appropriate calls into
Koha::Cache in C4::Context.

Test plan :
* have DEBUG env set to 1
* reach addbiblio page to test the patch in Biblio.pm, or setup more than 1
  language
* you should see in the logs that you're reading and writing from cache
* run the test suite twice both with and without the following environment
  variables set:
export MEMCACHED_SERVERS=127.0.0.1:11211
export MEMCACHED_NAMESPACE=KOHA
export CACHING_SYSTEM=memcached

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

I'm unsure about some of the caching times 10000 is a long long time,
but other than that, works fine.

QA Comment:
Code looks good to me.
Just thinking: Instead of hardcoding cache times here and there in some variety, could we intelligently configure them in a pref or so ? :)

Passed QA

(In reply to comment #14)
> QA Comment:
> Code looks good to me.
> Just thinking: Instead of hardcoding cache times here and there in some
> variety, could we intelligently configure them in a pref or so ? :)
> 
> Passed QA

I agree it could be configured manually. But I think we need more than one caching time.
Some informations have a long duration (like branches, that almost never change)
Some informations should have a shorter duration.

And anyway, I think we will quickly need a way to clear all cachings.
We have 2 options for that:
 * reload cache everywhere we have cache, when a value is updated
 * provide a "clear cache" in admin-home.pl, where the syslibrarian can reload everything when there is a value changed

The 1st option is better, the 2nd is easier.

For sysprefs caching = chris_c said (and he's right) that caching is not a matter of speed but of load. Caching sysprefs just one by one is maybe more expensive than not caching (will do some tests to get number). I think we'd better load *all* the sysprefs in the cache at once -in just one cache object-. It may be a little bit costly for the 1st page, but faster for the next ones.

What do you guys think of this idea ?

(delaying push of this patch a little bit, I want to test it deeply. Will push other more easier-to-test patches before)

Performances tests:

I tried to run the staff_benchmark on this patch, (with memcached) and so no noticeable performance improvement
Bug 8089

With patch and with cache
Benchmarking with 20 occurences of each operation and 30 concurrent sessions 
Step 1: staff client main page     	5955ms	3.358 pages/sec
Step 2: catalog detail page        	13270ms	1.507 biblios/sec
Step 3: catalogue search               	23628ms	0.846 biblios/sec
Step 4: patron detail page         	11629ms	1.719 borrowers/sec
Step 5: patron search page             	20355ms	1.965 borrowers/sec
Step 6a circulation (checkouts)        	15775ms	1.267 checkouts/sec
Step 6b circulation (checkins)         	15134ms	1.321 checkins/sec
all transactions at once               	112801ms	1.595 operations/sec

Without patch and with cache
Step 1: staff client main page     	5980ms	3.344 pages/sec
Step 2: catalog detail page        	13828ms	1.446 biblios/sec
Step 3: catalogue search               	21114ms	0.947 biblios/sec
Step 4: patron detail page         	11378ms	1.757 borrowers/sec
Step 5: patron search page             	20316ms	1.968 borrowers/sec
Step 6a circulation (checkouts)        	16162ms	1.237 checkouts/sec
Step 6b circulation (checkins)         	16916ms	1.182 checkins/sec
all transactions at once               	109549ms	1.643 operations/sec

With patch and without caching
Step 1: staff client main page     	5914ms	3.381 pages/sec
Step 2: catalog detail page        	13543ms	1.476 biblios/sec
Step 3: catalogue search               	20992ms	0.952 biblios/sec
Step 4: patron detail page         	11993ms	1.667 borrowers/sec
Step 5: patron search page             	20293ms	1.971 borrowers/sec
Step 6a circulation (checkouts)        	15181ms	1.317 checkouts/sec
Step 6b circulation (checkins)         	16692ms	1.198 checkins/sec
all transactions at once               	109429ms	1.644 operations/sec


checked with  perl -MCache::Memcached -MData::Dumper=Dumper -le 'print Dumper(Cache::Memcached->new(servers => ["localhost:11211"])->stats);'
that memcache was running & accessed:
'get_hits' => '1284',
'cmd_get' => '1302',

Could not find any problem, so ... patch pushed

(but I still think we [c|sh]ould store all sysprefs in a single cache entry)

OK, I had some errors in my setup, so caching was not running in fact.
I've fixed and made my perf tests again:

Step 1: staff client main page     	8722ms	2.293 pages/sec
Step 2: catalog detail page        	13303ms	1.503 biblios/sec
Step 3: catalogue search               	19606ms	1.02 biblios/sec
Step 4: patron detail page         	11635ms	1.718 borrowers/sec
Step 5: patron search page             	18413ms	2.172 borrowers/sec
Step 6a circulation (checkouts)        	17861ms	1.119 checkouts/sec
Step 6b circulation (checkins)         	15223ms	1.313 checkins/sec


=> no noticeable improvements again. Jared pointed that it could be because CHI uses Moose if it's available, it is on my setup. And Moose is heavy to load :(

Last patch needs to be tested and pushed, Master is currently broken (anything that uses UpdateInTables in C4::SQLHelper does not work)
Like ModMember

Once again the tests save us from releasing drastically broken code. 
Now we just have to get it so we run the tests before we push.

mmm... testing the follow-up, i've the same problem with and without it: modifications to patrons are not saved.

the logs says:
DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '=NULL' at line 3 at /home/paul/koha.dev/koha-community/C4/SQLHelper.pm line 197.

(In reply to comment #21)
> mmm... testing the follow-up, i've the same problem with and without it:
> modifications to patrons are not saved.
> 
> the logs says:
> DBD::mysql::st execute failed: You have an error in your SQL syntax; check
> the manual that corresponds to your MySQL server version for the right
> syntax to use near '=NULL' at line 3 at
> /home/paul/koha.dev/koha-community/C4/SQLHelper.pm line 197.

That error is unrelated to this I think.

Running the tests before the patch fail, and the borrower in the db is not modified, running the tests after the tests pass and the borrower is modified.

You might want to make sure you have flushed memcached if you have it running.

FYI, C4::SQLHelper::GetPrimaryKeys seems to be broken now. It returns 1 instead of the primary keys.

(In reply to comment #23)
> FYI, C4::SQLHelper::GetPrimaryKeys seems to be broken now. It returns 1
> instead of the primary keys.

Kyle that is precisely what this follow up patch fixes.

The follow-up does not fix the problem for me.
After discussion with Jared on IRC, the initial patches have been reverted.
Dobrica said he will work on another patch for caching.

(In reply to comment #25)
> The follow-up does not fix the problem for me.
> After discussion with Jared on IRC, the initial patches have been reverted.
> Dobrica said he will work on another patch for caching.

I have a fix for it integrated into my followup patch for Bug 7067. Someone could just steal that code out and post if up as a patch here.

Your change is line-for-line identical (other than one indentation change) to the follow-up that did not work for Paul.

(In reply to comment #27)
> Your change is line-for-line identical (other than one indentation change)
> to the follow-up that did not work for Paul.

That's both funny and odd. It definitely works for me.

Kyle

(In reply to comment #28)
> (In reply to comment #27)
> > Your change is line-for-line identical (other than one indentation change)
> > to the follow-up that did not work for Paul.
> 
> That's both funny and odd. It definitely works for me.

For both Chris and me, too. It seems to be an interaction of the patch with data on Paul's system.

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > Your change is line-for-line identical (other than one indentation change)
> > > to the follow-up that did not work for Paul.
> > 
> > That's both funny and odd. It definitely works for me.
> 
> For both Chris and me, too. It seems to be an interaction of the patch with
> data on Paul's system.

my investigations shows that the PrimaryKey is not found, resulting in a SQL that is:
UPDATE borrowers SET ... WHERE =?
which is, obviously, wrong :\

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > (In reply to comment #27)
> > > > Your change is line-for-line identical (other than one indentation change)
> > > > to the follow-up that did not work for Paul.
> > > 
> > > That's both funny and odd. It definitely works for me.
> > 
> > For both Chris and me, too. It seems to be an interaction of the patch with
> > data on Paul's system.
> 
> my investigations shows that the PrimaryKey is not found, resulting in a SQL
> that is:
> UPDATE borrowers SET ... WHERE =?
> which is, obviously, wrong :\

I've tested with and without cache and with and without Plack

Hi Jared,

Great patches!

In a first time I tested the patch and do not find anything to say.
It seems working well.

So I tried to find something to discuss, but I just found stuffs to quibble ;)
e.g.
1/ I don't understand your test (t/Cache.t):
    ok(exists $myhash->{'anotherkey'}, 'Cache reset properly');
2/ qa-tools had helped me! :)
 * Koha/Cache.pm                                                            FAIL
  pod                         FAIL
    *** ERROR: =over on line 281 without closing =back (at head2)  in file Koha/Cache.pm
  forbidden patterns          FAIL
    The patch introduces a forbidden pattern: withespace character  (369)

And I was going to sign off yours patches (bug 9434 and this one), but I had a last idea : check the perfs.

I think we expect from this patch a gain of performance.

(Results produce by NYTProf)
On the preferences.pl page (admin/preferences.pl?tab=cataloguing) I got:
  with the master branch:
    admin/preferences.pl for 839ms (of 922ms), executing 180787 statements and 30350 subroutine calls in 175 source files and 39 string evals.
  with bug 9434 and bug 9089:
    admin/preferences.pl for 1.13s (of 1.27s), executing 300952 statements and 53214 subroutine calls in 183 source files and 39 string evals.

On a search result page (catalogue/search.pl?q=d) I got:
  with the master branch:
    catalogue/search.pl for 1.51s (of 1.85s), executing 659967 statements and 122242 subroutine calls in 119 source files and 29 string evals.
Statements | time | Module
249908  147ms   Template/Parser.pm
140543  124ms   MARC/Field.pm
95389   80.1ms  C4/Biblio.pm
2552    70.9ms  C4/Reserves.pm
23      67.4ms  CGI/Session/Driver/mysql.pm
19488   37.7ms  MARC/Record.pm

  with bug 9434 and bug 9089:
    catalogue/search.pl for 2.59s (of 3.19s), executing 1136765 statements and 218314 subroutine calls in 131 source files and 29 string evals.
174117  249ms   Carp.pm
161178  153ms   Compress/Raw/Zlib.pm
249908  139ms   Template/Parser.pm
15360   134ms   Compress/Zlib.pm
5067    130ms   Storable.pm
140543  125ms   MARC/Field.pm
3378    100ms   Koha/Cache.pm

It will be very difficult to gain 1 second by caching some others stuffs.

(In reply to comment #33)
> Hi Jared,
> 
> Great patches!
> 
> In a first time I tested the patch and do not find anything to say.
> It seems working well.
> 
> So I tried to find something to discuss, but I just found stuffs to quibble
> ;)
> e.g.
> 1/ I don't understand your test (t/Cache.t):
>     ok(exists $myhash->{'anotherkey'}, 'Cache reset properly');

This is to confirm that after clearing a hash cache-tied variable values can be retrieved again. The cache was reset, but even after getting reset, the 'anotherkey' value can be retrieved 

> 2/ qa-tools had helped me! :)
>  * Koha/Cache.pm                                                           
> FAIL
>   pod                         FAIL
>     *** ERROR: =over on line 281 without closing =back (at head2)  in file
> Koha/Cache.pm
>   forbidden patterns          FAIL
>     The patch introduces a forbidden pattern: withespace character  (369)

I will fix these, and attach a new patch to bug 9434.

> And I was going to sign off yours patches (bug 9434 and this one), but I had
> a last idea : check the perfs.
> 
> I think we expect from this patch a gain of performance.

No, definitely not. I think I mentioned this in my commit message. On an low-load system, caching for something as simple as sysprefs will be slower than just retrieving the data from MySQL without any cache. We'll see a performance improvement with frameworks/languages, which are slow to load from the database, but with anything that can be loaded from the database, the cache is going to be slower than MySQL. There are two reasons why caching sysprefs is important:
1) scalability-we retrieve dozens of sysprefs on every single page load. Load ten pages simultaneously, and that's 200+ sysprefs requested. Load a hundred pages, and you have thousands of database requests simultaneously, just for system preferences.
2) right now the way we do caching is not thread-safe, and you get inconsistent results depending on what server thread you hit.

We could probably improve performance by changing the serialization method used for caching (JSON::XS is supposed to blow everything else out of the water, performance-wise), but for an initial patch, I figured something using the default Storable serialization would be best.

> (Results produce by NYTProf)
> On the preferences.pl page (admin/preferences.pl?tab=cataloguing) I got:
>   with the master branch:
>     admin/preferences.pl for 839ms (of 922ms), executing 180787 statements
> and 30350 subroutine calls in 175 source files and 39 string evals.
>   with bug 9434 and bug 9089:
>     admin/preferences.pl for 1.13s (of 1.27s), executing 300952 statements
> and 53214 subroutine calls in 183 source files and 39 string evals.
> 
> On a search result page (catalogue/search.pl?q=d) I got:
>   with the master branch:
>     catalogue/search.pl for 1.51s (of 1.85s), executing 659967 statements
> and 122242 subroutine calls in 119 source files and 29 string evals.
> Statements | time | Module
> 249908  147ms   Template/Parser.pm
> 140543  124ms   MARC/Field.pm
> 95389   80.1ms  C4/Biblio.pm
> 2552    70.9ms  C4/Reserves.pm
> 23      67.4ms  CGI/Session/Driver/mysql.pm
> 19488   37.7ms  MARC/Record.pm
> 
>   with bug 9434 and bug 9089:
>     catalogue/search.pl for 2.59s (of 3.19s), executing 1136765 statements
> and 218314 subroutine calls in 131 source files and 29 string evals.
> 174117  249ms   Carp.pm
> 161178  153ms   Compress/Raw/Zlib.pm
> 249908  139ms   Template/Parser.pm
> 15360   134ms   Compress/Zlib.pm
> 5067    130ms   Storable.pm
> 140543  125ms   MARC/Field.pm
> 3378    100ms   Koha/Cache.pm
> 
> It will be very difficult to gain 1 second by caching some others stuffs.

If people are not concerned about scalability, they can simply disable the cache. That is one of the other advantages of this implementation. If you don't want it, you can disable it with minimal impact on performance.

(In reply to comment #34)
> > 1/ I don't understand your test (t/Cache.t):
> >     ok(exists $myhash->{'anotherkey'}, 'Cache reset properly');
> 
> This is to confirm that after clearing a hash cache-tied variable values can
> be retrieved again. The cache was reset, but even after getting reset, the
> 'anotherkey' value can be retrieved

So the following test is not more complete?
  ok($myhash->{'anotherkey'} eq "anothervalue", 'Cache reset properly');

> > I think we expect from this patch a gain of performance.
> 
> No, definitely not. I think I mentioned this in my commit message. On an
> low-load system, caching for something as simple as sysprefs will be slower
> than just retrieving the data from MySQL without any cache. We'll see a
> performance improvement with frameworks/languages, which are slow to load
> from the database, but with anything that can be loaded from the database,
> the cache is going to be slower than MySQL. There are two reasons why
> caching sysprefs is important:
> 1) scalability-we retrieve dozens of sysprefs on every single page load.
> Load ten pages simultaneously, and that's 200+ sysprefs requested. Load a
> hundred pages, and you have thousands of database requests simultaneously,
> just for system preferences.
> 2) right now the way we do caching is not thread-safe, and you get
> inconsistent results depending on what server thread you hit.
> 
> We could probably improve performance by changing the serialization method
> used for caching (JSON::XS is supposed to blow everything else out of the
> water, performance-wise), but for an initial patch, I figured something
> using the default Storable serialization would be best.

You mentioned in the commit message that the patch will not improve performance, not that it will be degrade them :)
I thought a gain after a 2nd refresh of the page would have observed, but it was not the case.
1 second is so long (regarding the original 1.5s), I am wondering how it is possible to recover it.

> > It will be very difficult to gain 1 second by caching some others stuffs.
> 
> If people are not concerned about scalability, they can simply disable the
> cache. That is one of the other advantages of this implementation. If you
> don't want it, you can disable it with minimal impact on performance.

By disabling you mean remove or unset the CACHING_SYSTEM env variable?
I am not a supporter of sysprefs but is it not a good way to have a quick switch?

Sorry for all my questions, I am not an expert of cache issues :)

(In reply to comment #35)
> So the following test is not more complete?
>   ok($myhash->{'anotherkey'} eq "anothervalue", 'Cache reset properly');

is($myhash->{'anotherkey'}, 'anothervalue', 'Cache reset properly');
would work as well. I used defined because the important issue is that the value is being set in some way.

> By disabling you mean remove or unset the CACHING_SYSTEM env variable?
> I am not a supporter of sysprefs but is it not a good way to have a quick
> switch?

Right. It's funny you should mention adding a syspref. One of my follow-ups will be adding a syspref for enabling/disabling caching, and, eventually, eliminating the need for environment variables at all. One of the reasons I decided to write Koha::Cache::Object, even though it meant losing CHI, is that using caching like this means that we could control caching from a syspref or koha-conf.xml and still be able to cache sysprefs and koha-conf.xml.

You can also disable the syspref cache specifically using C4::Context->disable_syspref_cache();

> Sorry for all my questions, I am not an expert of cache issues :)

Created attachment 14891 [details] [review]
[NOT APPLY] perl testing script

Back after some others tests.
I made a simulation with  simultaneous process.
This script gives an idea of the performance I got with 4 different configurations.

The tests were executed on a Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz with 8g of RAM.

4 cases:
1/ Branch master using CGI
2/ Branch master + Bug 8089 and Bug 9434 patches using CGI
3/ Branch master using Plack
4/ Branch master + Bug 8089 and Bug 9434 patches using Plack

For 10 simultaneous processes (admin/preferences.pl / catalogue/search.pl?q=d):
1/ 2.7s / 3.1s
2/ 3.0s / 3.4s
3/ 0.8s / 1.4s
4/ 1.1s / 1.7s

For 100 simultaneous processes (admin/preferences.pl / catalogue/search.pl?q=d):
1/ 47.2s / 60.4s
2/ 30.0s / 58.2s
3/  8.0s / 13.2s
4/ 10.8s / 16.8s

These results are consistent with Jared's explanation.

Moreover, with the caching system and some simultaneous processes, this patch could be useful.
But this script does not generate a real life test case...

I will sign off these patches.

QA comment
 * koha-qa.pl OK
 * thx jonathan for the perl testing script
 * no reason not to pass QA

passed QA

questions:
 * you don't set a timeout to the syspref cache, so it's 0, so it's "no timeout", right ?
 * if we write some other caching subs and attach them to this bug, would you push them before 3.12. I was thinking of getAllLanguages and some other subs that can be cached & will improve scalability a lot (and lower SQL queries)

Created attachment 16551 [details] [review]
[PASSED QA] Bug 8089: Cache sysprefs with Koha::Cache::Object

This patch replaces the %sysprefs hash variable with a
Koha::Cache::Object-tied hash that allows the user to cache sysprefs
in memcached, fastmmap, or memory.

To test:
1) Enable the cache as described on bug 9434.
2) Try viewing some pages in the OPAC and staff client. If the syspref
   caching isn't working, you'll be redirected to the web installer
   (in the staff client) or be shown the maintenance page (in the OPAC).
3) Disable the cache by unsetting the CACHING_SYSTEM environment
   variable.
4) Try viewing some pages in the OPAC and staff client. If the syspref
   caching isn't working, you'll be redirected to the web installer
   (in the staff client) or be shown the maintenance page (in the OPAC).

Note that this will not improve performance. It does, however, eliminate
the problem of sysprefs not getting updated in all threads in
multi-threaded environments.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>

(In reply to comment #39)
> questions:
>  * you don't set a timeout to the syspref cache, so it's 0, so it's "no
> timeout", right ?

10 minutes, actually. This is a departure from current behavior, but I think it's a reasonable one.

>  * if we write some other caching subs and attach them to this bug, would
> you push them before 3.12. I was thinking of getAllLanguages and some other
> subs that can be cached & will improve scalability a lot (and lower SQL
> queries)

I don't think so, no. Too much risk of side effects. getAllLanguages already has memoize_memcached, which -- though problematic -- does work.

This patch has been pushed to master.

Reverted from master since it caused problems in t/db_dependent/Context.t.

Comment on attachment 11400 [details] [review]
Bug 8089: Use Koha::Cache for all caching

Un-obsoleting the latest version of the "Use Koha::Cache for all caching" patch.  My aim is to write some test cases and have another go.

The patch for bug 11112 that I pushed just now adds another instance of caching-via-package-variables that will need to be converted to using Koha::Cache as part of resurrecting the patch for this bug.

I found quite a few issues in Koha::Cache, so I'm improving it in bug 12041. This might affect some stuff here.

Isn't this obsolete ?

Yes, see also bug 11921 and bug 16770.