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.
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?
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?
> 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.
(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.
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.
(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.).
(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 :)
Is there a way to have Plack running without packages?
/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
Can't we use debian/templates/plack.psgi instead?
You can use the one you want. I have just shared the method I use for years now.
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.
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
(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.
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.
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
+ 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)
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?
Ok I think I got it: you want to serialise all prefs into the same structure.
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.
(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.
(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.
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...
(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?
(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.