[Bug 16088] New: Excessive CGI->new() calls hurting cache performace under plack
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Bug ID: 16088 Summary: Excessive CGI->new() calls hurting cache performace under plack Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: abl@biblos.pk.edu.pl QA Contact: testopia@bugs.koha-community.org Under plack, CGI->new is overriden (in misc/plack/koha.psgi etc.) - we are flushing L1 syspref cache in there. C4::Language::getlanguage() subroutine calls CGI->new().. Problem is, getlanguage() is getting called surprisingly often in Koha (e.g., ~100 times per single catalogue search), and that hurts syspref caching performance under plack very significantly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16044 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15263 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Bug 15263 should help a bit -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 49322 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49322&action=edit Bug 16088: [POC] Koha::CGI - Do no create a new CGI object to access the language -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion Depends on| |16044, 11998 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- For discussion With this patch, I get only 1 call to CGI->new (hard to do less:)) instead of 344 on a search with 20 results (default XSLT used). It's quite dirty but I have no idea how we could do another way (we could store it in the L1 cache instead?) Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998 [Bug 11998] Syspref caching issues https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16044 [Bug 16044] Define a L1 cache for all objects set in cache -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16044 | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|16044 | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16044 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16044 [Bug 16044] Define a L1 cache for all objects set in cache -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Jacek, btw it's an excellent catch, thanks for that! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pianohacker@gmail.com, | |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #49322|0 |1 is obsolete| | --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 49489 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49489&action=edit Bug 16088: [POC] Koha::CGI - Do no create a new CGI object to access the language -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #49489|0 |1 is obsolete| | --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 49588 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49588&action=edit Bug 16088: [POC] Koha::CGI - Do no create a new CGI object to access the language -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Just a rebase. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=16140 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- See benchmarks on bug 16140 for more details on this patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #9 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- Trying to evaluate this patch, but I'm having trouble understanding how it works. $Koha::CGI::language is a package variable (persistent under plack); if some stript calls Languages::getlanguage() without calling Koha::CGI->new() somewhere earlier, it may pick "outdated" language code (set by some other script which was calling getlanguage() first). Not sure if the parameters in "use Koha::CGI qw('-no_undef_params' -utf8 )" will get propagated from catalogue/search.pl to Koha::CGI somehow, there is no "use CGI qw( -utf8 );" in the Koha/CGI.pm, there is only commented out "# use base CGI;" (?). BTW, I wonder why there is "use CGI" instead of "use CGI (-uft8)" in C4::Languages, looks like some kind of exception or omission - is there a correlation with Bug 13815? Can't find an explanation in Bug 13815 why exactly plack was losing that uft8 flag in the 1st place.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #10 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jonathan Druart from comment #3)
For discussion With this patch, I get only 1 call to CGI->new (hard to do less:)) instead of 344 on a search with 20 results (default XSLT used). It's quite dirty but I have no idea how we could do another way (we could store it in the L1 cache instead?)
We certainly don't want language code to be propagated to shared persitent L2 cache ;). I think that some kind of extra simple L1 memory-only cache implementation may come in handy anyway, for caching things that are specific to the given script run. Something as simple as possible - eg. Koha/Cache/Memory/Lite.pm, with get(), set() and clear() methods - no expiration, deep copying etc. needed. Having something like that, we can: 1) cache CGI object first (in plack startup CGI->new() override and eg. in C4::Auth::get_template_and_user() as well) 2) use that cached CGI object in getlanguage() later - if it's a 1st call for a given run 3) cache language code obtained in the 1st call for possible later uses so this subroutine would not need to check CGI object again in subsequent calls 4) clear extra-simpe-L1-memory-only cache in the CGI override By the look of it, language code returned by getlanguage() stays always the same for a given script run, so it shouldn't introduce any regressions, at least in theory. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |ASSIGNED --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jacek Ablewicz from comment #9)
Trying to evaluate this patch, but I'm having trouble understanding how it works.
$Koha::CGI::language is a package variable (persistent under plack); if some stript calls Languages::getlanguage() without calling Koha::CGI->new() somewhere earlier, it may pick "outdated" language code (set by some other script which was calling getlanguage() first).
I am feeling a bit silly, this patch did not make any sense, indeed... It seems that I was looking at the performances too much and did not keep in mind that the patch should not be buggy :)
Not sure if the parameters in "use Koha::CGI qw('-no_undef_params' -utf8 )" will get propagated from catalogue/search.pl to Koha::CGI somehow, there is no "use CGI qw( -utf8 );" in the Koha/CGI.pm, there is only commented out "# use base CGI;" (?).
BTW, I wonder why there is "use CGI" instead of "use CGI (-uft8)" in C4::Languages, looks like some kind of exception or omission - is there a correlation with Bug 13815? Can't find an explanation in Bug 13815 why exactly plack was losing that uft8 flag in the 1st place..
I guess we should add the flag to C4::Languages. I do not remember why the flag is getting lost, but it is :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |jonathan.druart@bugs.koha-c | |ommunity.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Jacek, what about caching the language (as usual) using the sessionID in the cache key (with a limited TTL)? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #12)
Jacek, what about caching the language (as usual) using the sessionID in the cache key (with a limited TTL)?
Forget that, it does not make sense... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jesse Weaver <jweaver@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|pianohacker@gmail.com |jweaver@bywatersolutions.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #49588|0 |1 is obsolete| | --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 51335 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51335&action=edit Bug 16088: Use Koha::Cache to cache the language The goal of this patch is to avoid unecessary flush of the L1 cache on creating a new CGI object each time C4::Languages::getlanguage is called without a CGI object. Test plan: At the OPAC and the intranet interfaces: - Clear the cookies of your browser - Choose a language and browse - Change the language and browse Try to use a language which is not defined: Add &language=es-ES (if es-ES is not translated) to the url, you should not see the Spanish interface. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #15 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jonathan Druart from comment #14)
Created attachment 51335 [details] [review] Bug 16088: Use Koha::Cache to cache the language
This new version looks a bit half-baked: you define new subroutine set_in_L1_cache_only(), but it's not used anywhere, instead you are using set_in_cache() / get_from_cache() so language code gets propagated to L2, if L2 == memcached this will have some "interesting" side effects. Under plack, there still will be one unnecessary L1 flush - that's way better than hundreds of unnecessary flushes, but that one extra flush is probably avoidable as well. IMO it would be better to have separate L1 memory-only cache for goals like that (implemented in different package, or separate hash + a set of subroutines to deal with it in Koha::Cache). Storing things that we don't want to propagate to L2 in the same hash (%L1_cache) will make further improvements (both Bug 16140 and Bug 16166 for example) much more complicated. I suggested to implement such a cache in a new package because having subroutines like get_from_separate_L1_memory_only_cache() in Koha::Cache may look kind of silly ;). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51335|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 51392 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51392&action=edit Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language The goal of this patch is to avoid unecessary flush of the L1 cache on creating a new CGI object each time C4::Languages::getlanguage is called without a CGI object. The new class Koha::Cache::Memory::Lite must be flushed by the CGI constructor overide done in the psgi file. This new class will ease caching of specific stuffs used by running script. Test plan: At the OPAC and the intranet interfaces: Open 2 different browser session to simulate several users - Clear the cookies of the browsers - User 1 (U1) an User 2 (U2) should be set to the default language (depending on the browser settings) - U1 chooses another language - U2 refreshes and the language used must be the default one - U2 chooses a third language - U1 refreshes and must be still using the one he has choosen. Try to use a language which is not defined: Add &language=es-ES (if es-ES is not translated) to the url, you should not see the Spanish interface. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51392|0 |1 is obsolete| | --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 51398 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51398&action=edit Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language The goal of this patch is to avoid unecessary flush of the L1 cache on creating a new CGI object each time C4::Languages::getlanguage is called without a CGI object. The new class Koha::Cache::Memory::Lite must be flushed by the CGI constructor overide done in the psgi file. This new class will ease caching of specific stuffs used by running script. Test plan: At the OPAC and the intranet interfaces: Open 2 different browser session to simulate several users - Clear the cookies of the browsers - User 1 (U1) an User 2 (U2) should be set to the default language (depending on the browser settings) - U1 chooses another language - U2 refreshes and the language used must be the default one - U2 chooses a third language - U1 refreshes and must be still using the one he has choosen. Try to use a language which is not defined: Add &language=es-ES (if es-ES is not translated) to the url, you should not see the Spanish interface. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #18 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- That part of the code if ( $cgi and not $cgi->param('language') ) { my $cached = $memory_cache->get_from_cache($cache_key); still doesn't look quite right IMO; getlanguage() is almost never called with $cgi parameter, and the cached value will be hardly ever used. Shoudn't it be something like that instead: unless ($cgi && $cgi->param('language')) { -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51398|0 |1 is obsolete| | --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 51405 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51405&action=edit Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language The goal of this patch is to avoid unecessary flush of the L1 cache on creating a new CGI object each time C4::Languages::getlanguage is called without a CGI object. The new class Koha::Cache::Memory::Lite must be flushed by the CGI constructor overide done in the psgi file. This new class will ease caching of specific stuffs used by running script. Test plan: At the OPAC and the intranet interfaces: Open 2 different browser session to simulate several users - Clear the cookies of the browsers - User 1 (U1) an User 2 (U2) should be set to the default language (depending on the browser settings) - U1 chooses another language - U2 refreshes and the language used must be the default one - U2 chooses a third language - U1 refreshes and must be still using the one he has choosen. Try to use a language which is not defined: Add &language=es-ES (if es-ES is not translated) to the url, you should not see the Spanish interface. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jacek Ablewicz from comment #18)
That part of the code
if ( $cgi and not $cgi->param('language') ) { my $cached = $memory_cache->get_from_cache($cache_key);
still doesn't look quite right IMO; getlanguage() is almost never called with $cgi parameter, and the cached value will be hardly ever used.
Shoudn't it be something like that instead:
unless ($cgi && $cgi->param('language')) {
Indeed! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jacek Ablewicz <ablewicz@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jacek Ablewicz <ablewicz@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51405|0 |1 is obsolete| | --- Comment #21 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 51415 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51415&action=edit Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language The goal of this patch is to avoid unecessary flush of the L1 cache on creating a new CGI object each time C4::Languages::getlanguage is called without a CGI object. The new class Koha::Cache::Memory::Lite must be flushed by the CGI constructor overide done in the psgi file. This new class will ease caching of specific stuffs used by running script. Test plan: At the OPAC and the intranet interfaces: Open 2 different browser session to simulate several users - Clear the cookies of the browsers - User 1 (U1) an User 2 (U2) should be set to the default language (depending on the browser settings) - U1 chooses another language - U2 refreshes and the language used must be the default one - U2 chooses a third language - U1 refreshes and must be still using the one he has choosen. Try to use a language which is not defined: Add &language=es-ES (if es-ES is not translated) to the url, you should not see the Spanish interface. Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #22 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- Looks good, tests good and works really good - now there is only one CGI->new() call! Aparently get_template_and_user(), which usually triggers the 1st getlanguage() call is doing it properly with $cgi parameter included. Not sure if cache clean in setlanguagecookie() is strictly necessary, but it's not harming performance in any way, and this sub is used also in installer, better to be on the safe side. Koha::Cache::set_in_L1_cache_only() may need some kind of unit test, if it's intended to stay there. In t/Cache.t skip "Cache not enabled", 33 unless ( $cache->is_cache_active() && defined $cache ); number of tests to skip is out of sync, but it was out of sync before this patch as well. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51415|0 |1 is obsolete| | --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 51418 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51418&action=edit Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language The goal of this patch is to avoid unecessary flush of the L1 cache on creating a new CGI object each time C4::Languages::getlanguage is called without a CGI object. The new class Koha::Cache::Memory::Lite must be flushed by the CGI constructor overide done in the psgi file. This new class will ease caching of specific stuffs used by running script. Test plan: At the OPAC and the intranet interfaces: Open 2 different browser session to simulate several users - Clear the cookies of the browsers - User 1 (U1) an User 2 (U2) should be set to the default language (depending on the browser settings) - U1 chooses another language - U2 refreshes and the language used must be the default one - U2 chooses a third language - U1 refreshes and must be still using the one he has choosen. Try to use a language which is not defined: Add &language=es-ES (if es-ES is not translated) to the url, you should not see the Spanish interface. Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #24 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jacek Ablewicz from comment #22)
Looks good, tests good and works really good - now there is only one CGI->new() call! Aparently get_template_and_user(), which usually triggers the 1st getlanguage() call is doing it properly with $cgi parameter included.
Not sure if cache clean in setlanguagecookie() is strictly necessary, but it's not harming performance in any way, and this sub is used also in installer, better to be on the safe side.
What I thought too, I did not manage to know if it was useful but preferred to keep it.
Koha::Cache::set_in_L1_cache_only() may need some kind of unit test, if it's intended to stay there.
Erk, of course it is not intended, I have amended the patch using git reset HEAD~1 Koha/Cache.pm git checkout Koha/Cache.pm git commit --amend Nothing more
In t/Cache.t
skip "Cache not enabled", 33 unless ( $cache->is_cache_active() && defined $cache );
number of tests to skip is out of sync, but it was out of sync before this patch as well.
I will submit a patch for that -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 51420 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51420&action=edit Bug 16088: Fix the number of tests to skip -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jacek Ablewicz <ablewicz@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51420|0 |1 is obsolete| | --- Comment #26 from Jacek Ablewicz <ablewicz@gmail.com> --- Created attachment 51429 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51429&action=edit Bug 16088: Fix the number of tests to skip Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #27 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- Yikes, finally it would be possible to benchmark koha under plack and get the timings that actually make some sense ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #28 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jacek Ablewicz from comment #27)
Yikes, finally it would be possible to benchmark koha under plack and get the timings that actually make some sense ;)
Does that mean you have doubt about this patch? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #29 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jonathan Druart from comment #28)
(In reply to Jacek Ablewicz from comment #27)
Yikes, finally it would be possible to benchmark koha under plack and get the timings that actually make some sense ;)
Does that mean you have doubt about this patch?
No, not at all. I think this patch is pretty essential for all plack setups. What I meant was that some earlier benchmarks I did with plack (catalogue searches in particular) were 50%-100% slower in comparision with CGI, for no apparent reasons, and with this patch I expect this to be fixed. BTW, is 3.22 also affected by this? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #30 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jacek Ablewicz from comment #29)
BTW, is 3.22 also affected by this?
Yes but to a lesser extent, bug 16044 has not been backported (yet?): in 3.22, CGI->new is overridden to call clear_syspref_L1_cache. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #31 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jonathan Druart from comment #30)
(In reply to Jacek Ablewicz from comment #29)
BTW, is 3.22 also affected by this?
Yes but to a lesser extent, bug 16044 has not been backported (yet?): in 3.22, CGI->new is overridden to call clear_syspref_L1_cache.
AFAIRC feching a syspref from memcached takes 80-120 usec on average, in 3.22 there are ~1500 ->preference() calls per catalog search (for 20 results per page, ~3000 for 50 results per page). There is still some (hard to say how much) syspref caching going on.. Worst case scenario: 1500 * 0.0001 = 150 msec penalty. Unless someone is running 3.22 under plack without memcached (worst case: 1500 * 0.0014 = 2.1 seconds) - probably not such a big deal. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #32 from Jesse Weaver <jweaver@bywatersolutions.com> --- Question about naming/design; this thing you're making has less in common with Koha::Cache and more in common with a per-request C4::Context. Might it make more sense to tie it to the latter? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jesse Weaver from comment #32)
Question about naming/design; this thing you're making has less in common with Koha::Cache and more in common with a per-request C4::Context. Might it make more sense to tie it to the latter?
To me it makes sense to have it using the Koha::Cache namespace, it's cache-related, but I don't really mind about the naming :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jesse Weaver <jweaver@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jesse Weaver <jweaver@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51418|0 |1 is obsolete| | --- Comment #34 from Jesse Weaver <jweaver@bywatersolutions.com> --- Created attachment 52008 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52008&action=edit Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language The goal of this patch is to avoid unecessary flush of the L1 cache on creating a new CGI object each time C4::Languages::getlanguage is called without a CGI object. The new class Koha::Cache::Memory::Lite must be flushed by the CGI constructor overide done in the psgi file. This new class will ease caching of specific stuffs used by running script. Test plan: At the OPAC and the intranet interfaces: Open 2 different browser session to simulate several users - Clear the cookies of the browsers - User 1 (U1) an User 2 (U2) should be set to the default language (depending on the browser settings) - U1 chooses another language - U2 refreshes and the language used must be the default one - U2 chooses a third language - U1 refreshes and must be still using the one he has choosen. Try to use a language which is not defined: Add &language=es-ES (if es-ES is not translated) to the url, you should not see the Spanish interface. Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jesse Weaver <jweaver@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51429|0 |1 is obsolete| | --- Comment #35 from Jesse Weaver <jweaver@bywatersolutions.com> --- Created attachment 52009 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52009&action=edit Bug 16088: Fix the number of tests to skip Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl> Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Brendan Gallagher <brendan@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |brendan@bywatersolutions.co | |m --- Comment #36 from Brendan Gallagher <brendan@bywatersolutions.com> --- Works cool. Pushed to Master - Should be in the November 2016 release. Thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16675 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16675 [Bug 16675] fix breakage of t/Languages.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr --- Comment #37 from Frédéric Demians <frederic@tamil.fr> --- Am I wrong, or this patch is critic for 16.05 performances? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #38 from Frédéric Demians <frederic@tamil.fr> --- Pushed in 16.05. Will be in 16.05.01. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com Status|Pushed to Master |Pushed to Stable --- Comment #39 from Julian Maurice <julian.maurice@biblibre.com> --- Patch pushed to 3.22.x, will be in 3.22.8 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16088 --- Comment #40 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Frédéric Demians from comment #37)
Am I wrong, or this patch is critic for 16.05 performances?
No critic, but it's a good to have. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org