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.
Created attachment 3926 [details] [review] Initial patch This patch is not intended to make it into master right now, but for testing performance gains. It could serve as a basis for future work if found worth the trouble.
Created attachment 3927 [details] [review] Simplified patch Remove unneeded changes to rewrite-config.PL
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 }
Created attachment 5045 [details] [review] Simplified patch, comments added (for slef) Just commented the 'memcached stuff' part of the code.
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.
(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.
Created attachment 5537 [details] [review] Signed-off patch I tested this on a fresh install. It worked as expected with memcached running, and stopped too.
Created attachment 5559 [details] [review] Bug 6193 - Use memcached cache koha-conf.xml configuration variables Basically I add some code to Context.pm so it stores koha-conf.xml config vars in memcached server. It is arguable whether this is an improvement or not, tests are needed to conclude something about this. Error handling was an issue with Cache::Memcached, I tried to do it the simplest I could. Note: As it was pointless to read the xml file to get the memcached server info for accessing them for retreiving the info in the koha-conf.xml file... I passed those values through apache's SetEnv. These variables are set acordingly through the install scripts and commented out if koha was setup not to use it. Bug 6193 - Properly comment the code Required by slef. Regards To+ Signed-off-by: Federico Rinaudo <frinaudo@infocpt.com.ar> Signed-off-by: MJ Ray <mjr@phonecoop.coop>
Updating version : This ENH could be in Koha 3.8
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)
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.
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+
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.
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.
patch pushed, please test
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.
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 ?
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
Created attachment 6971 [details] [review] Follow up: use SetEnv and remove memcached from koha-conf.xml I have a SetEnv-only version, but thought htis would be simple and clean.
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
linking this patch to 7248 and setting to "in discussion", as they overlap
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!
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"
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
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.