Bug 15341 - Performance - Retrieve all sysprefs at once
Summary: Performance - Retrieve all sysprefs at once
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 11998 13967
Blocks: 15342
  Show dependency treegraph
 
Reported: 2015-12-09 15:16 UTC by Jonathan Druart
Modified: 2023-08-30 12:20 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15341: Retrieve all sysprefs at once (2.07 KB, patch)
2015-12-09 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15341: Retrieve all sysprefs at once (3.29 KB, patch)
2016-10-04 09:53 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 2015-12-09 15:16:58 UTC
C4::Context->preference generates a call to Koha::Config::SysPrefs->find for all different sysprefs we are searching for.
It seems to be a performance killer and be better to retrieve them all together.
Comment 1 Jonathan Druart 2015-12-09 15:35:02 UTC
Created attachment 45535 [details] [review]
Bug 15341: Retrieve all sysprefs at once

C4::Context->preference generates a call to Koha::Config::SysPrefs->find
for all different sysprefs we are searching for.
It seems to be a performance killer and be better to retrieve them all
together.

I have enabled general_log in the MariaDB config file, the GET
circ/circulation.pl?borrowernumber=42
There are 293 queries of the following form:
  SELECT me.variable, me.value, me.options, me.explanation, me.type FROM
    systempreferences me WHERE ( me.variable = 'MY_SYSPREF' )

Then benchmarking before and after this patch (using plackup, and the
misc/plack/koha.psgi file with Profiler::NYTProf enabled):

Before:
Profile of /home/koha/perl5/bin/plackup for -2.07s (of 616ms), executing
158857 statements and 588479 subroutine calls in 427 source files and 73
string evals.
885 174ms 4.03s                    C4::Context::preference

After:
Profile of /home/koha/perl5/bin/plackup for -1.70s (of 349ms), executing
94273 statements and 397525 subroutine calls in 437 source files and 79
string evals.
1138  194ms 1.99s              C4::Context::preference
(calls) (exclusive time) (inclusive time)

Could someone confirm that?
Comment 2 Frédéric Demians 2015-12-09 17:26:00 UTC
On my box, without plack, execution time:

  - Without your patch: 1.70s
  - With your patch: 1.54s

I can see in NYTProf stats that 1/3 of execution time is spent in C4::Context::preference.

Isn't plack supposed to bring some persistency? And so couldn't we have preferences available somewhere from one page to the other?
Comment 3 Frédéric Demians 2015-12-09 18:11:32 UTC
> I can see in NYTProf stats that 1/3 of execution time is spent in
> C4::Context::preference.

But, this time is in fact spent in Koha::Database::schema, than DBIx::Class::Schema::load_namespaces.
Comment 4 Jonathan Druart 2015-12-10 09:48:52 UTC
(In reply to Frédéric Demians from comment #2)
> Isn't plack supposed to bring some persistency? And so couldn't we have
> preferences available somewhere from one page to the other?

Both psgi files (debian/templates/plack.psgi and misc/plack/koha.psgi) disable the syspref cache (C4::Context->disable_syspref_cache();), see bug 13805.
Comment 5 Frédéric Demians 2015-12-10 10:14:57 UTC
See comment 3. The main issue isn't in preferences by themselves, but in DBIx::Class startup. Let see that:

https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/Cookbook.pod#STARTUP-SPEED

Loading all Koha schema classes take more than 0.5 seconds on a recent server. With plack, schema shouldn't be reloaded for each page. That's where plack persistency will save time.
Comment 6 Jacek Ablewicz 2015-12-10 12:06:27 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Frédéric Demians from comment #2)
> > Isn't plack supposed to bring some persistency? And so couldn't we have
> > preferences available somewhere from one page to the other?
> 
> Both psgi files (debian/templates/plack.psgi and misc/plack/koha.psgi)
> disable the syspref cache (C4::Context->disable_syspref_cache();), see bug
> 13805.

Not good ;). But in my tests, with this patch applied, it is even worse - when syspref cache is disabled:

   #!/usr/bin/perl
   use Modern::Perl;
   use C4::Context;

   C4::Context->disable_syspref_cache();

   my @sysprefs = qw/
     UseTransportCostMatrix
     StaticHoldsQueueWeight
     RandomizeHoldsQueueWeight/;

   for (my $i = 0; $i < 1000; $i++) {
     for my $syspref_name (@sysprefs) {
       my $syspref_val = C4::Context->preference($syspref_name);
     }
   }

Runnig time - without patch: 4.4sec, with patch: 47.2 sec..

With syspref cache enabled, the difference is negglible (0.36 patched, 0.37 sec unpatched), but the patched version still seems to be a bit slower (but not much, <2 usec per call - hard to measure, in real life scenarios I guess it will also depend on the mysql query_cache_limit/size/type settings etc.).
Comment 7 Jonathan Druart 2015-12-10 12:18:15 UTC
(In reply to Jacek Ablewicz from comment #6)
> (In reply to Jonathan Druart from comment #4)
> > (In reply to Frédéric Demians from comment #2)
> > > Isn't plack supposed to bring some persistency? And so couldn't we have
> > > preferences available somewhere from one page to the other?
> > 
> > Both psgi files (debian/templates/plack.psgi and misc/plack/koha.psgi)
> > disable the syspref cache (C4::Context->disable_syspref_cache();), see bug
> > 13805.
> 
> Not good ;). But in my tests, with this patch applied, it is even worse -
> when syspref cache is disabled:
> 
>    #!/usr/bin/perl
>    use Modern::Perl;
>    use C4::Context;
> 
>    C4::Context->disable_syspref_cache();
> 
>    my @sysprefs = qw/
>      UseTransportCostMatrix
>      StaticHoldsQueueWeight
>      RandomizeHoldsQueueWeight/;
> 
>    for (my $i = 0; $i < 1000; $i++) {
>      for my $syspref_name (@sysprefs) {
>        my $syspref_val = C4::Context->preference($syspref_name);
>      }
>    }
> 
> Runnig time - without patch: 4.4sec, with patch: 47.2 sec..
> 
> With syspref cache enabled, the difference is negglible (0.36 patched, 0.37
> sec unpatched), but the patched version still seems to be a bit slower (but
> not much, <2 usec per call - hard to measure, in real life scenarios I guess
> it will also depend on the mysql query_cache_limit/size/type settings etc.).

Yes, I have tested *with* the syspref cache (I should have mentioned that in the commit msg!)
What you described is expected, you are not running under Plack and for each call all pref are retrieved from the DB :)
Comment 8 Frédéric Demians 2015-12-11 11:39:25 UTC
Is there a way to have Plack running without packages?
Comment 9 Jonathan Druart 2015-12-11 12:00:22 UTC
/etc/apache2/sites-available/koha.conf
    <Proxy>
      Order deny,allow
      Allow from 127.0.0.1,localhost
    </Proxy>
    <location /cgi-bin/koha/>
      ProxyPass http://localhost:5001/cgi-bin/koha/
    </location>

Then I use plackup with misc/plack/koha.psgi
Comment 10 Tomás Cohen Arazi 2015-12-11 13:58:14 UTC
Can't we use debian/templates/plack.psgi instead?
Comment 11 Jonathan Druart 2015-12-14 09:43:10 UTC
You can use the one you want. I have just shared the method I use for years now.
Comment 12 Jonathan Druart 2016-03-03 11:59:52 UTC
Retested today on opac-search:

Without the patch (i.e master):
17.0s (of 20.8s), executing 3550368 statements and 1053896 subroutine calls in 421 source files and 63 string evals.

With the patch:
Profile of /home/koha/perl5/bin/plackup for 95.8s (of 117s), executing 19107084 statements and 7240133 subroutine calls in 421 source files and 63 string evals.

With the patch *and* the syspref cache (i.e. remove C4::Context->disable_syspref_cache;)
15.8s (of 18.6s), executing 2588556 statements and 903720 subroutine calls in 421 source files and 63 string evals.
Comment 13 Jonathan Druart 2016-03-03 12:12:45 UTC
Something interesting:

on master:
# spent 35.1ms within DBI::common::DESTROY which was called 12520 times, avg 3µs/call

with path:
# spent 2.54s (697ms+1.85) within DBIx::Class::DESTROY which was called 118584 times, avg 21µs/call
Comment 14 Jonathan Druart 2016-03-03 17:27:05 UTC
(In reply to Jonathan Druart from comment #13)
> Something interesting:
> 
> on master:
> # spent 35.1ms within DBI::common::DESTROY which was called 12520 times, avg
> 3µs/call
> 
> with path:
> # spent 2.54s (697ms+1.85) within DBIx::Class::DESTROY which was called
> 118584 times, avg 21µs/call

Note that I don't get that without C4::Context->clear_syspref_cache, see bug 15970.
Comment 15 Jonathan Druart 2016-03-07 09:20:45 UTC
I have no idea how to write this patch on top of bug 11998.
We could call set_in_cache for all prefs, but it may spend too much time to process all of them.
Or we may not need it anymore. It will be good to benchmark the different possibilities.
I will continue to work on this one when bug 11998 will be pushed.
Comment 16 Jacek Ablewicz 2016-09-14 19:00:18 UTC
Jonathan,

I wonder what would you think about resurrecting this bug? Starts to look like a better and better idea to me, it would:

- allow to flush all L2 syspref cache entries selectively (storing them in separate [sub]namespace still isn't taking care of that, due to the memcached constraints in this matter)
- make the idea from Bug 16140 (aka intelligent L1 cache flush under plack) much more efficient
- be faster if done that way; fetching and deserialising a hash containing all 520+ system preferences takes ca 500-600 microseconds with sereal, which is an equivalent of fetching ~20 preferences separately (and an average Koha script is using 80+ preferences)
- there will be really just one accessor to such hash / structure (->preference() ), and it returns a scalar, so "unsafe" fetch could be used without introducing any risks for hard-to-catch kinds of regressions
Comment 17 Jacek Ablewicz 2016-09-14 19:06:58 UTC
+ retrieving all sysprefs at once directly from the database will be a lot faster too (especially in more recent DBIx version with hashref_inflator involved)
Comment 18 Jonathan Druart 2016-10-04 08:50:58 UTC
Jacek, sorry this has ran out of my radar.

I do not really undertand how this could be (really) useful, the syspref cache is populated when Koha needs a pref value for the first time, then the prefs are picked from the cache.
This patch will only improve the first hits and it will be insignificant compared to the whole pages load.

I may missed something here, could you please elaborate a bit more?
Comment 19 Jonathan Druart 2016-10-04 08:53:13 UTC
Ok I think I got it: you want to serialise all prefs into the same structure.
Comment 20 Jonathan Druart 2016-10-04 09:53:32 UTC
Created attachment 56012 [details] [review]
Bug 15341: Retrieve all sysprefs at once

C4::Context->preference generates a call to Koha::Config::SysPrefs->find
for all different sysprefs we are searching for.
It seems to be a performance killer and be better to retrieve them all
together and then cache them all.
Comment 21 Jonathan Druart 2016-10-04 09:54:42 UTC
(In reply to Jonathan Druart from comment #20)
> Created attachment 56012 [details] [review] [review]
> Bug 15341: Retrieve all sysprefs at once

Jacek, did you have something like that in mind?
This patch is not graceful and will need to be improved.
Comment 22 Jacek Ablewicz 2016-10-06 08:17:50 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Jonathan Druart from comment #20)
> > Created attachment 56012 [details] [review] [review] [review]
> > Bug 15341: Retrieve all sysprefs at once
> 
> Jacek, did you have something like that in mind?

Something like that, yes - i.e. that retrieving all sysprefs together from L2 cache, as a single data structure, should be (arguably) better then fetching 80-150 scalars individually per script run.

I expect there will be some speed improvements involved (nothing really major, maybe 5-10 msec on average), but the main benefit from such change would be the possibility to use the upstream (L2) cache more effectively. E.g.: for a start: no more need to call ->flush_all() in clear_syspref_cache(), ->clear_from_cache("sysprefs") will do the trick, without clearing all other existing L2 cache entries. If you are using the single memcached instance for multiple tenants (or worse: storing user sessions in the very same cache), avoiding ->flush_all() like a plague may be kind of important.
Comment 23 David Cook 2019-09-02 07:04:05 UTC
Looks like an old one, but I've often wondered why we haven't fetched all the system preferences at once to avoid subsequent calls to the database. 

I suppose it does introduce overhead though as you are getting everything when in practice you might only use a few preferences. And the more preferences you have, the slower that fetch will become. It wouldn't scale. 

I suppose the L1/L2 caching we have now should take care of this, although I'm trying to see where the L2 cache gets intialized...
Comment 24 Katrin Fischer 2023-08-28 20:08:48 UTC
(In reply to David Cook from comment #23)
> Looks like an old one, but I've often wondered why we haven't fetched all
> the system preferences at once to avoid subsequent calls to the database. 
> 
> I suppose it does introduce overhead though as you are getting everything
> when in practice you might only use a few preferences. And the more
> preferences you have, the slower that fetch will become. It wouldn't scale. 
> 
> I suppose the L1/L2 caching we have now should take care of this, although
> I'm trying to see where the L2 cache gets intialized...

Joubu, reading this, would you agree we can close this bug?
Comment 25 Jonathan Druart 2023-08-30 12:20:31 UTC
(In reply to Katrin Fischer from comment #24)
> (In reply to David Cook from comment #23)
> > Looks like an old one, but I've often wondered why we haven't fetched all
> > the system preferences at once to avoid subsequent calls to the database. 
> > 
> > I suppose it does introduce overhead though as you are getting everything
> > when in practice you might only use a few preferences. And the more
> > preferences you have, the slower that fetch will become. It wouldn't scale. 
> > 
> > I suppose the L1/L2 caching we have now should take care of this, although
> > I'm trying to see where the L2 cache gets intialized...
> 
> Joubu, reading this, would you agree we can close this bug?

No, I still think this is a good one.