Bug 15970 - Plack should not clear syspref cache
Summary: Plack should not clear syspref cache
Status: RESOLVED DUPLICATE of bug 11998
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 13815
Blocks: 15342
  Show dependency treegraph
 
Reported: 2016-03-03 13:25 UTC by Jonathan Druart
Modified: 2016-03-10 10:15 UTC (History)
2 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 15970: Do not clear syspref cache in psgi file (1.30 KB, patch)
2016-03-03 17:19 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 ***