Bug 6193 - Use memcached cache koha-conf.xml configuration variables
Summary: Use memcached cache koha-conf.xml configuration variables
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 6000 7119
Blocks: 7248 11167 11921 16579
  Show dependency treegraph
 
Reported: 2011-04-14 13:11 UTC by Tomás Cohen Arazi
Modified: 2016-06-13 16:21 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Initial patch (5.13 KB, patch)
2011-04-14 17:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Simplified patch (4.28 KB, patch)
2011-04-14 18:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Simplified patch, comments added (for slef) (4.52 KB, patch)
2011-08-17 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Signed-off patch (4.55 KB, patch)
2011-09-22 13:04 UTC, Federico Rinaudo
Details | Diff | Splinter Review
Bug 6193 - Use memcached cache koha-conf.xml configuration variables (4.58 KB, patch)
2011-09-23 10:35 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
Follow up: use SetEnv and remove memcached from koha-conf.xml (5.56 KB, patch)
2011-12-30 12:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 6193 - Follow up: use SetEnv and remove memcached from koha-conf.xml (5.88 KB, patch)
2012-02-13 16:25 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2011-04-14 13:11:42 UTC
Using memcached to avoid parsing the koha-conf.xml each time C4::Context is created would mean an interesting speed improvement, depending on the host system.
Comment 1 Tomás Cohen Arazi 2011-04-14 17:52:40 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2011-04-14 18:25:49 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2011-07-07 17:44:39 UTC
I'd like to state FRT that I find interesting the use of C4::Context->ismemcached and C4::Context->memcached outside C4::Context, as
1) C4::Context is used everywhere memcached is used
2) It will simplify code in those places:

Biblio.pm:

    my $servers = C4::Context->config('memcached_servers');
    if ($servers) {
        require Memoize::Memcached;
        import Memoize::Memcached qw(memoize_memcached);

        my $memcached = {
            servers    => [$servers],
            key_prefix => C4::Context->config('memcached_namespace') || 'koha',
        };
        memoize_memcached( 'GetMarcStructure', memcached => $memcached, expire_time => 600 );    #cache for 10 minutes
    }

To:

    if (C4::Context->ismemcached) {
        require Memoize::Memcached;
        import Memoize::Memcached qw(memoize_memcached);
        memoize_memcached( 'GetMarcStructure',
                            memcached => C4::Context->memcached,
                            expire_time => 600 );    #cache for 10 minutes
    }
Comment 4 Tomás Cohen Arazi 2011-08-17 13:20:13 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2011-09-19 12:37:21 UTC
I am a bit confused that this is set to 'Signed off' - I can't find a comment or signed-off patch. I am resetting it to 'Needs Signoff' so that it does not get missed. - Plz change back if I missed something.
Comment 6 Tomás Cohen Arazi 2011-09-20 20:04:53 UTC
(In reply to comment #5)
> I am a bit confused that this is set to 'Signed off' - I can't find a comment
> or signed-off patch. I am resetting it to 'Needs Signoff' so that it does not
> get missed. - Plz change back if I missed something.

Kf, don't know what to say. Maybe the biblibre folks forgot to send the patch. It's a pity.
Comment 7 Federico Rinaudo 2011-09-22 13:04:39 UTC Comment hidden (obsolete)
Comment 8 MJ Ray (software.coop) 2011-09-23 10:35:25 UTC Comment hidden (obsolete)
Comment 9 Paul Poulain 2011-10-24 11:40:13 UTC
Updating version : This ENH could be in Koha 3.8
Comment 10 Paul Poulain 2011-10-25 15:05:46 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 11 Paul Poulain 2011-11-25 17:55:30 UTC
Some performance tests:
(using an updated version of benchmark_circulation i'll push soon)

WITHOUT THIS PATCH:
misc/load_testing/benchmark_circulation.pl http://koha-community:8080/cgi-bin/koha/ test test
Authentication successful
--------------
Koha circulation benchmarking utility
--------------
Benchmarking with 20 occurences of each operation and 30 concurrent sessions 
Load testing staff client dashboard page	27460ms	0.728332119446468 pages/sec
Load testing catalog detail page	29092ms	0.687474219716761 biblios/sec
Load testing patron detail page	32413ms	0.617036374294265 borrowers/sec
Load testing patron search page	60148ms	0.665026268537607 borrowers/sec

WITH THIS PATCH:
misc/load_testing/benchmark_circulation.pl http://koha-community:8080/cgi-bin/koha/ test test
Authentication successful
--------------
Koha circulation benchmarking utility
--------------
Benchmarking with 20 occurences of each operation and 30 concurrent sessions 
Load testing staff client dashboard page	26654ms	0.750356419299167 pages/sec
Load testing catalog detail page	25688ms	0.77857365306758 biblios/sec
Load testing patron detail page	29681ms	0.67383174421347 borrowers/sec
Load testing patron search page	56895ms	0.703049477106951 borrowers/sec

=> the gain is not huge, but real. This is due to the fact that the decoding of the XML is replaced by the time for Koha to reach memcache server & get an answer.
We should not miss that memcache is more dedicated to removing load on mySQL than gaining speed.
I also have a patch from Ian that change the conf into a YAML file. It seems decoding a YAML is very fast. I'll run the tests as well, attach ian patch here and report the results.
Thus we will see which patch give the best speed and decide which we can/should apply.
Comment 12 Tomás Cohen Arazi 2011-11-25 18:04:17 UTC
I heard about using YAML and agree it would be a good improvement. Just would like to say that in our production sites the main concern was filesystem I/O, as everything is stored in a SAN, and have multiple sites on the same virtual VPS.

I also added in #c3 that i find more suitable the way memcached is handled in C4::Context and posibbly reused across the Koha modules, meaning having a single memcached object, and ismemcached variable to test.

Regards
To+
Comment 13 Chris Cormack 2011-11-25 18:06:52 UTC
memcached is about scalability, In my opinion even if YAML is faster, both should be applied.

People who need to scale up, can switch on the memcaching that way.
Comment 14 Marcel de Rooy 2011-12-19 09:35:30 UTC
QA Comment:
Patch has been tested and signed off by two individuals.
Code looks good and promises performance increase when (optionally) using memcached.
Marking as Passed QA.
Comment 15 Paul Poulain 2011-12-27 16:18:35 UTC
patch pushed, please test
Comment 16 Paul Poulain 2011-12-28 16:51:25 UTC
NYTPROF testing:
before running NYTPROF, you must export variable & load at least one page (to fill memcache)
This is done by (on your shell):
export MEMCACHED_SERVERS=127.0.0.1:11211
export MEMCACHED_NAMESPACE=xxxx (xxxx= whatever you've set in your Apache)

Then, load mainpage on firefox at least once (to store the config file in memcache)

then, you can run:
perl -d:NYTProf mainpage.pl

The results show you don't call read_config_file sub anymore. The timing go from 230ms to 2.56ms on my test computer:

$self = $memcached->get('kohaconf');
# spent 2.56ms making 2 calls to Cache::Memcached::get, avg 1.28ms/call

(side question: why the hell does my last benchmarks show no gain ?)

BEFORE THE PATCH:
Profile of mainpage.pl for 2.12s (of 2.74s), executing 197477 statements and 59679 subroutine calls in 279 source files and 80 string evals.
AFTER THE PATCH:
Profile of mainpage.pl for 1.70s (of 2.14s), executing 137323 statements and 33090 subroutine calls in 273 source files and 79 string evals.
Comment 17 Paul Poulain 2011-12-28 17:10:28 UTC
QA question:
this patch requires that the memcache parameters are in Apache SetEnv variables. BUT they are still in the Koha config file.
It means we have them in 2 places, and that's wrong.

tcohen, did I miss something ? Why haven't you updated memcache to use only SetEnv everywhere ?
Comment 18 Paul Poulain 2011-12-28 17:34:16 UTC
IRC with tcohen a few minuts ago:
<tcohen> i've been conservative in the way I propose several enhancements
<tcohen> assuming big changes can lead the enhancement not to be included
<tcohen> I can send a patch that solves this
<tcohen> and can even send a patch that reuses Context->memcached 
<paul_p> tcohen, mmm... in fact, I think that if I had seen this problem before, I could have proposed "failed QA", even if it's usefull. You're highly welcomed to send a patch finishing the job !
<tcohen> ok
<tcohen> a patch against master, that uses setenv
Comment 19 Tomás Cohen Arazi 2011-12-30 12:25:25 UTC Comment hidden (obsolete)
Comment 20 Paul Poulain 2011-12-30 13:22:35 UTC
Comment on attachment 5559 [details] [review]
Bug 6193 - Use memcached cache koha-conf.xml configuration variables

This patch has been pushed. follow-up still to test & push
Comment 21 Paul Poulain 2012-02-03 09:12:55 UTC
linking this patch to 7248 and setting to "in discussion", as they overlap
Comment 22 Jared Camins-Esakov 2012-02-12 19:36:21 UTC
I can't figure out how to test this follow-up which is supposed to change it so that the system uses SetEnv. It doesn't have an example of the use of SetEnv. All it does is remove the memcached configuration from koha-conf.xml. Help!
Comment 23 Jared Camins-Esakov 2012-02-13 16:25:59 UTC
Created attachment 7630 [details] [review]
Bug 6193 - Follow up: use SetEnv and remove memcached from koha-conf.xml

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Confirmed that memcached is still being used after the memcached configuration
in koha-conf.xml was removed, and the following two lines were added to
both virtual hosts in koha-httpd.conf:
SetEnv MEMCACHED_SERVERS "127.0.0.1:11211"
SetEnv MEMCACHED_NAMESPACE "KOHA"
Comment 24 Paul Poulain 2012-02-20 22:30:38 UTC
QA comment:
this patch switches all memoize calls to ENV / koha-httpd.conf.
code is OK, but does not apply to new/bug_6193 branch due to some fixes chris_c made to expiry_time.
I'll have to made a tiny change to the patch to remove expiry_time from the follow-up and move it to C4::Context and push this patch to master branch directly
Comment 25 Paul Poulain 2012-02-21 12:17:48 UTC
Patch pushed

Note that the expiry_time set to 600 seconds has been moved at the right place (in the memcached handler), this is an extension of bug 7432 that chris_c made a few weeks ago.