Bug 16088 - Excessive CGI->new() calls hurting cache performace under plack
Summary: Excessive CGI->new() calls hurting cache performace under plack
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 11998
Blocks: 16675
  Show dependency treegraph
 
Reported: 2016-03-17 13:04 UTC by Jacek Ablewicz
Modified: 2017-12-07 22:16 UTC (History)
6 users (show)

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


Attachments
Bug 16088: [POC] Koha::CGI - Do no create a new CGI object to access the language (2.22 KB, patch)
2016-03-18 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: [POC] Koha::CGI - Do no create a new CGI object to access the language (2.77 KB, patch)
2016-03-23 22:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: [POC] Koha::CGI - Do no create a new CGI object to access the language (2.76 KB, patch)
2016-03-25 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: Use Koha::Cache to cache the language (3.88 KB, patch)
2016-05-09 16:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language (10.10 KB, patch)
2016-05-10 17:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language (10.11 KB, patch)
2016-05-11 08:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language (10.11 KB, patch)
2016-05-11 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language (10.17 KB, patch)
2016-05-11 11:36 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language (9.77 KB, patch)
2016-05-11 12:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: Fix the number of tests to skip (718 bytes, patch)
2016-05-11 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16088: Fix the number of tests to skip (777 bytes, patch)
2016-05-11 13:43 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 16088: Introduce Koha::Cache::Memory::Lite to cache the language (9.89 KB, patch)
2016-06-03 13:56 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 16088: Fix the number of tests to skip (831 bytes, patch)
2016-06-03 13:56 UTC, Jesse Weaver
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2016-03-17 13:04:52 UTC
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.
Comment 1 Jonathan Druart 2016-03-18 15:01:34 UTC
Bug 15263 should help a bit
Comment 2 Jonathan Druart 2016-03-18 15:55:07 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-03-18 15:57:32 UTC
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?)
Comment 4 Jonathan Druart 2016-03-18 16:26:26 UTC
Jacek, btw it's an excellent catch, thanks for that!
Comment 5 Jonathan Druart 2016-03-23 22:59:43 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-03-25 13:22:33 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2016-03-25 13:22:51 UTC
Just a rebase.
Comment 8 Jonathan Druart 2016-03-25 16:30:09 UTC
See benchmarks on bug 16140 for more details on this patch.
Comment 9 Jacek Ablewicz 2016-05-09 10:50:40 UTC
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..
Comment 10 Jacek Ablewicz 2016-05-09 11:58:17 UTC
(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.
Comment 11 Jonathan Druart 2016-05-09 14:27:37 UTC
(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 :)
Comment 12 Jonathan Druart 2016-05-09 14:57:55 UTC
Jacek, what about caching the language (as usual) using the sessionID in the cache key (with a limited TTL)?
Comment 13 Jonathan Druart 2016-05-09 15:41:32 UTC
(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...
Comment 14 Jonathan Druart 2016-05-09 16:34:25 UTC Comment hidden (obsolete)
Comment 15 Jacek Ablewicz 2016-05-10 09:38:00 UTC
(In reply to Jonathan Druart from comment #14)
> Created attachment 51335 [details] [review] [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 ;).
Comment 16 Jonathan Druart 2016-05-10 17:09:19 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-05-11 08:59:22 UTC Comment hidden (obsolete)
Comment 18 Jacek Ablewicz 2016-05-11 09:46:28 UTC
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')) {
Comment 19 Jonathan Druart 2016-05-11 10:07:33 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2016-05-11 10:07:52 UTC
(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!
Comment 21 Jacek Ablewicz 2016-05-11 11:36:03 UTC Comment hidden (obsolete)
Comment 22 Jacek Ablewicz 2016-05-11 11:47:02 UTC
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.
Comment 23 Jonathan Druart 2016-05-11 12:10:08 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2016-05-11 12:12:14 UTC
(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
Comment 25 Jonathan Druart 2016-05-11 12:14:12 UTC Comment hidden (obsolete)
Comment 26 Jacek Ablewicz 2016-05-11 13:43:49 UTC Comment hidden (obsolete)
Comment 27 Jacek Ablewicz 2016-05-11 13:48:22 UTC
Yikes, finally it would be possible to benchmark koha under plack and get the timings that actually make some sense ;)
Comment 28 Jonathan Druart 2016-05-11 13:54:48 UTC
(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?
Comment 29 Jacek Ablewicz 2016-05-11 14:15:37 UTC
(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?
Comment 30 Jonathan Druart 2016-05-11 14:39:54 UTC
(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.
Comment 31 Jacek Ablewicz 2016-05-11 15:32:29 UTC
(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.
Comment 32 Jesse Weaver 2016-05-11 15:33:56 UTC
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?
Comment 33 Jonathan Druart 2016-05-13 14:08:54 UTC
(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 :)
Comment 34 Jesse Weaver 2016-06-03 13:56:02 UTC
Created attachment 52008 [details] [review]
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>
Comment 35 Jesse Weaver 2016-06-03 13:56:11 UTC
Created attachment 52009 [details] [review]
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>
Comment 36 Brendan Gallagher 2016-06-04 07:45:08 UTC
Works cool.  Pushed to Master - Should be in the November 2016 release.  Thanks!
Comment 37 Frédéric Demians 2016-06-15 06:01:47 UTC
Am I wrong, or this patch is critic for 16.05 performances?
Comment 38 Frédéric Demians 2016-06-16 10:52:18 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 39 Julian Maurice 2016-06-16 12:02:23 UTC
Patch pushed to 3.22.x, will be in 3.22.8
Comment 40 Jonathan Druart 2016-06-18 13:03:04 UTC
(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.