Bug 15970

Summary: Plack should not clear syspref cache
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: abl, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11998
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13815    
Bug Blocks: 15342    
Attachments: Bug 15970: Do not clear syspref cache in psgi file

Description Jonathan Druart 2016-03-03 13:25:24 UTC
Just played with nytprof and found something exciting:

If I remove the following line:
  C4::Context->clear_syspref_cache();
from the psgi file, the processing time is divided per 2 (!)

With the clear cache:
for 15.8s (of 18.6s), executing 2588556 statements and 903720 subroutine calls in 421 source files and 63 string evals.

Without the clear cache:
for 7.82s (of 8.69s), executing 904432 statements and 262539 subroutine calls in 421 source files and 63 string evals.

I have tried to update a pref (NoLoginInstructions): it's saved and displayed correctly.
Am I missing something?
Comment 1 Jacek Ablewicz 2016-03-03 14:48:24 UTC
(In reply to Jonathan Druart from comment #0)
> Just played with nytprof and found something exciting:
> 
> If I remove the following line:
>   C4::Context->clear_syspref_cache();
> from the psgi file, the processing time is divided per 2 (!)
> 
> With the clear cache:
> for 15.8s (of 18.6s), executing 2588556 statements and 903720 subroutine
> calls in 421 source files and 63 string evals.
> 
> Without the clear cache:
> for 7.82s (of 8.69s), executing 904432 statements and 262539 subroutine
> calls in 421 source files and 63 string evals.

Such a difference looks kind of weird - unless the script you are testing calls (for whatever reasons) CGI->new() repeatedly ?

BTW, how many times was C4::Context::preference() called in that test?

> Am I missing something?

With that line removed, each time when you change some syspref value, you'll need to restart starman server/workers manually for that change to take effect. However, clearing syspref cache inside CGI->new() override sub is probably not 100% flawless method anyway, so serwer restarts are still needed if one wants to be on the safe side ;)
Comment 2 Jonathan Druart 2016-03-03 16:49:20 UTC
(In reply to Jacek Ablewicz from comment #1)
> (In reply to Jonathan Druart from comment #0)
> > Just played with nytprof and found something exciting:
> > 
> > If I remove the following line:
> >   C4::Context->clear_syspref_cache();
> > from the psgi file, the processing time is divided per 2 (!)
> > 
> > With the clear cache:
> > for 15.8s (of 18.6s), executing 2588556 statements and 903720 subroutine
> > calls in 421 source files and 63 string evals.
> > 
> > Without the clear cache:
> > for 7.82s (of 8.69s), executing 904432 statements and 262539 subroutine
> > calls in 421 source files and 63 string evals.
> 
> Such a difference looks kind of weird - unless the script you are testing
> calls (for whatever reasons) CGI->new() repeatedly ?
> 
> BTW, how many times was C4::Context::preference() called in that test?

No idea, patch is coming.

> > Am I missing something?
> 
> With that line removed, each time when you change some syspref value, you'll
> need to restart starman server/workers manually for that change to take
> effect. However, clearing syspref cache inside CGI->new() override sub is
> probably not 100% flawless method anyway, so serwer restarts are still
> needed if one wants to be on the safe side ;)

Yes indeed, I have fixed bug 11998 to solve this constraint.
Comment 3 Jonathan Druart 2016-03-03 17:19:39 UTC
Created attachment 48641 [details] [review]
Bug 15970: Do not clear syspref cache in psgi file

This in only in koha.psgi, it has been introduced by bug 13815 but
should not have been added by this patch.

Removing it should not introduce any changes.
Not that it won't impact debian packages.
Comment 4 Tomás Cohen Arazi 2016-03-04 12:57:53 UTC
(In reply to Jonathan Druart from comment #3)
> Created attachment 48641 [details] [review] [review]
> Bug 15970: Do not clear syspref cache in psgi file
> 
> This in only in koha.psgi, it has been introduced by bug 13815 but
> should not have been added by this patch.
> 
> Removing it should not introduce any changes.
> Not that it won't impact debian packages.

Yeah. No one cares about misc/plack/koha.psgi. Why not patching debian/templates/plack.psgi instead?
Comment 5 Jonathan Druart 2016-03-04 13:51:25 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Created attachment 48641 [details] [review] [review] [review]
> > Bug 15970: Do not clear syspref cache in psgi file
> > 
> > This in only in koha.psgi, it has been introduced by bug 13815 but
> > should not have been added by this patch.
> > 
> > Removing it should not introduce any changes.
> > Not that it won't impact debian packages.
> 
> Yeah. No one cares about misc/plack/koha.psgi. Why not patching
> debian/templates/plack.psgi instead?

I do :)
Because it does not disable the syspref cache, contrary to my earlier belief.
Comment 6 Jonathan Druart 2016-03-10 10:15:00 UTC
Moved to bug 11998.

*** This bug has been marked as a duplicate of bug 11998 ***