The goal is to be able to build a database handler (dbh) and to execute queries without loading unnecessary stuff. This will be useful to reduce memory usage of daemons that need to check the database periodically
Created attachment 120752 [details] [review] Bug 28306: Allow to query database with minimal memory footprint The goal is to be able to build a database handler (dbh) and to execute queries without loading unnecessary stuff. This will be useful to reduce memory usage of daemons that need to check the database periodically The patch provides a new method Koha::Database::dbh which returns a database handler without loading the DBIx::Class schema. This method is also used by DBIx::Class, so whether you use DBI or DBIx::Class, the same method is used to initialize the connection. The patch also moves some code in order to avoid loading C4::Context: - C4::Context::timezone moves to Koha::Config - C4::Context::db_scheme2dbi moves to Koha::Database To measure memory usage I used the following commands: * before the patch: perl -MKoha::Database \ -E 'Koha::Database->schema->storage->dbh->do("select 1");' \ -E '$|=1; say $$; sleep 2' \ | while read pid; do ps -p $pid -o rss=; done * after the patch: perl -MKoha::Database \ -E 'Koha::Database->dbh->do("select 1");' \ -E '$|=1; say $$; sleep 2' \ | while read pid; do ps -p $pid -o rss=; done It will give you the RSS (Resident Set Size) of the perl process in kB What I get: * before the patch: between 96.9MB and 97.2MB * after the patch: between 17.8MB and 18.2MB Note that if a timezone is configured (either from $KOHA_CONF or TZ environment variable), Koha will load DateTime::Timezone to check if it's valid, and it increases RSS to 36MB Another interesting metric is the number of modules loaded: * before the patch: perl -MKoha::Database \ -E 'Koha::Database->schema->storage->dbh;' \ -E 'say scalar keys %INC' Result: 567 * after the patch: perl -MKoha::Database \ -E 'Koha::Database->dbh;' \ -E 'say scalar keys %INC' Result: 51 Test plan: 1. Apply the patch & restart starman 2. Make sure Koha is still ok (ie. can access the database, does not have encoding issues, ...) 3. Run the tests in t/Context.t, t/Koha/Config.t, t/db_dependent/Koha/Database.t, t/timezones.t
This looks very interesting, I'm loving how much code cleaning goes on and the results in my testing so far are very promising.. I'll continue to play a little and then SO, great work Julian
Created attachment 120794 [details] [review] Bug 28306: Allow to query database with minimal memory footprint The goal is to be able to build a database handler (dbh) and to execute queries without loading unnecessary stuff. This will be useful to reduce memory usage of daemons that need to check the database periodically The patch provides a new method Koha::Database::dbh which returns a database handler without loading the DBIx::Class schema. This method is also used by DBIx::Class, so whether you use DBI or DBIx::Class, the same method is used to initialize the connection. The patch also moves some code in order to avoid loading C4::Context: - C4::Context::timezone moves to Koha::Config - C4::Context::db_scheme2dbi moves to Koha::Database To measure memory usage I used the following commands: * before the patch: perl -MKoha::Database \ -E 'Koha::Database->schema->storage->dbh->do("select 1");' \ -E '$|=1; say $$; sleep 2' \ | while read pid; do ps -p $pid -o rss=; done * after the patch: perl -MKoha::Database \ -E 'Koha::Database->dbh->do("select 1");' \ -E '$|=1; say $$; sleep 2' \ | while read pid; do ps -p $pid -o rss=; done It will give you the RSS (Resident Set Size) of the perl process in kB What I get: * before the patch: between 96.9MB and 97.2MB * after the patch: between 17.8MB and 18.2MB Note that if a timezone is configured (either from $KOHA_CONF or TZ environment variable), Koha will load DateTime::Timezone to check if it's valid, and it increases RSS to 36MB Another interesting metric is the number of modules loaded: * before the patch: perl -MKoha::Database \ -E 'Koha::Database->schema->storage->dbh;' \ -E 'say scalar keys %INC' Result: 567 * after the patch: perl -MKoha::Database \ -E 'Koha::Database->dbh;' \ -E 'say scalar keys %INC' Result: 51 Test plan: 1. Apply the patch & restart starman 2. Make sure Koha is still ok (ie. can access the database, does not have encoding issues, ...) 3. Run the tests in t/Context.t, t/Koha/Config.t, t/db_dependent/Koha/Database.t, t/timezones.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All works nicely and as expected.. I think this could lead to some real improvements in some of our regular command-line scripts etc. Signing off
Sounds good to me. I would've liked this as a patch set to make it easier to read. It looks like there are quite a few different changes here, so I think QA will need to take time to unpick them to make sure everything is OK. Unfortunately, I don't have that time right now. But +1 to the idea.
Created attachment 123145 [details] [review] Bug 28306: Allow to query database with minimal memory footprint The goal is to be able to build a database handler (dbh) and to execute queries without loading unnecessary stuff. This will be useful to reduce memory usage of daemons that need to check the database periodically The patch provides a new method Koha::Database::dbh which returns a database handler without loading the DBIx::Class schema. This method is also used by DBIx::Class, so whether you use DBI or DBIx::Class, the same method is used to initialize the connection. The patch also moves some code in order to avoid loading C4::Context: - C4::Context::timezone moves to Koha::Config - C4::Context::db_scheme2dbi moves to Koha::Database To measure memory usage I used the following commands: * before the patch: perl -MKoha::Database \ -E 'Koha::Database->schema->storage->dbh->do("select 1");' \ -E '$|=1; say $$; sleep 2' \ | while read pid; do ps -p $pid -o rss=; done * after the patch: perl -MKoha::Database \ -E 'Koha::Database->dbh->do("select 1");' \ -E '$|=1; say $$; sleep 2' \ | while read pid; do ps -p $pid -o rss=; done It will give you the RSS (Resident Set Size) of the perl process in kB What I get: * before the patch: between 96.9MB and 97.2MB * after the patch: between 17.8MB and 18.2MB Note that if a timezone is configured (either from $KOHA_CONF or TZ environment variable), Koha will load DateTime::Timezone to check if it's valid, and it increases RSS to 36MB Another interesting metric is the number of modules loaded: * before the patch: perl -MKoha::Database \ -E 'Koha::Database->schema->storage->dbh;' \ -E 'say scalar keys %INC' Result: 567 * after the patch: perl -MKoha::Database \ -E 'Koha::Database->dbh;' \ -E 'say scalar keys %INC' Result: 51 Test plan: 1. Apply the patch & restart starman 2. Make sure Koha is still ok (ie. can access the database, does not have encoding issues, ...) 3. Run the tests in t/Context.t, t/Koha/Config.t, t/db_dependent/Koha/Database.t, t/timezones.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Adding my SO - there are POD complaints from QA tools - this has potential to break things so another pair of eyes for QA would be nice
0.11 t/timezones.t .. Can't locate object method "redefine" via package "Test::MockModule" at /kohadevbox/koha/t/lib/Mocks.pm line 56, <DATA> line 1. koha-testing-docker has Test::MockModule v0.11, but the method "redefine" was not added until v0.13
(In reply to Kyle M Hall from comment #8) > 0.11 > > t/timezones.t .. Can't locate object method "redefine" via package > "Test::MockModule" at /kohadevbox/koha/t/lib/Mocks.pm line 56, <DATA> line 1. > > koha-testing-docker has Test::MockModule v0.11, but the method "redefine" > was not added until v0.13 I ran into that recently. It is a pity, but we should use ->mock until all our supported distros ship the newer Test::MockModule.
Created attachment 123148 [details] [review] Bug 28306: Allow to query database with minimal memory footprint The goal is to be able to build a database handler (dbh) and to execute queries without loading unnecessary stuff. This will be useful to reduce memory usage of daemons that need to check the database periodically The patch provides a new method Koha::Database::dbh which returns a database handler without loading the DBIx::Class schema. This method is also used by DBIx::Class, so whether you use DBI or DBIx::Class, the same method is used to initialize the connection. The patch also moves some code in order to avoid loading C4::Context: - C4::Context::timezone moves to Koha::Config - C4::Context::db_scheme2dbi moves to Koha::Database To measure memory usage I used the following commands: * before the patch: perl -MKoha::Database \ -E 'Koha::Database->schema->storage->dbh->do("select 1");' \ -E '$|=1; say $$; sleep 2' \ | while read pid; do ps -p $pid -o rss=; done * after the patch: perl -MKoha::Database \ -E 'Koha::Database->dbh->do("select 1");' \ -E '$|=1; say $$; sleep 2' \ | while read pid; do ps -p $pid -o rss=; done It will give you the RSS (Resident Set Size) of the perl process in kB What I get: * before the patch: between 96.9MB and 97.2MB * after the patch: between 17.8MB and 18.2MB Note that if a timezone is configured (either from $KOHA_CONF or TZ environment variable), Koha will load DateTime::Timezone to check if it's valid, and it increases RSS to 36MB Another interesting metric is the number of modules loaded: * before the patch: perl -MKoha::Database \ -E 'Koha::Database->schema->storage->dbh;' \ -E 'say scalar keys %INC' Result: 567 * after the patch: perl -MKoha::Database \ -E 'Koha::Database->dbh;' \ -E 'say scalar keys %INC' Result: 51 Test plan: 1. Apply the patch & restart starman 2. Make sure Koha is still ok (ie. can access the database, does not have encoding issues, ...) 3. Run the tests in t/Context.t, t/Koha/Config.t, t/db_dependent/Koha/Database.t, t/timezones.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 123149 [details] [review] Bug 28306: Switch back to using 'mock' instead of 'redefine' Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 123150 [details] [review] Bug 28306: Fix POD Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 123151 [details] [review] Bug 28306: Fix POD Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 21.11, thanks to everybody involved!
A very weird side-effects caused by this patch, t/db_dependent/Koha/Objects.t is failing with: t/db_dependent/Koha/Objects.t .. 18/24 # Failed test at t/db_dependent/Koha/Objects.t line 1152. # Failed test at t/db_dependent/Koha/Objects.t line 1164. # Looks like you failed 2 tests of 13. # Failed test 'filter_by_last_update' # at t/db_dependent/Koha/Objects.t line 1172. If you inspect the exception it contains "The method 1970-12-31->dt_from_string is not covered by tests!" Could you have a look?
(Prefixing the dt_from_string calls in filter_by_last_update fixes the error, but I don't think it's the correct fix)
Created attachment 124274 [details] [review] Bug 28306: Fix t/db_dependent/Koha/Objects.t It looks like a circular dependency problem where methods are not exported correctly There is at least one circular dependency here: C4::Context -> Koha::Caches -> Koha::Cache -> C4::Context (-> means "uses") The tests pass if C4::Context is loaded early, so the patch does that. Circular dependencies should be fixed ideally, but it is a little more complicated, and I don't have the time right now...
Created attachment 124282 [details] [review] Bug 28306: Trying to understand... Use of uninitialized value $class in sprintf at /kohadevbox/koha/Koha/Objects.pm line 594. The method ->dt_from_string is not covered by tests! Trace begun at /kohadevbox/koha/Koha/Objects.pm line 594 Koha::Objects::AUTOLOAD at /kohadevbox/koha/Koha/Objects.pm line 280 Koha::Objects::filter_by_last_update('Koha::Patrons=HASH(0x55baa017c208)', 'HASH(0x55baa017c178)') called at test.pl line 5 We are hitting AUTOLOAD, however I am not expecting to reach AUTOLOAD as I am considering dt_from_string() defined in Koha::Objects. https://perldoc.perl.org/perlsub#Autoloading
As I didn't get additional help I've pushed Julian's patch.
(In reply to Jonathan Druart from comment #18) > Bug 28306: Trying to understand... Does this commit title meet our standards? :) What I would like to understand too, is what happens with Objects.t on this patch. At first glance looks like deleting lots of lines since we cannot resolve a problem with AUTOLOAD or so ?
(In reply to Marcel de Rooy from comment #20) > (In reply to Jonathan Druart from comment #18) > > Bug 28306: Trying to understand... > > Does this commit title meet our standards? :) > > What I would like to understand too, is what happens with Objects.t on this > patch. At first glance looks like deleting lots of lines since we cannot > resolve a problem with AUTOLOAD or so ? It was not meant to be pushed. I sent the following email to the QA team on August 31: """ Wanna have some fun? I am struggling with the following situation. Today I pushed bug 28306 and noticed a failure on Objects.t (see from coment 15 - https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28306#c15) It's absolutely not related with the original patches, but seems to be linked with 17600 (the EXPORT_OK funkiness). So there is an easy fix, two actually: * use C4::Context from the test file (even not used directly from there...) * Use full qualified namespace for dt_from_string That could work, but we don't know what's going on exactly. I've attached a patch to play (comment 18), you just need to run a 3 lines script to recreate the problem A simple call to dt_from_string generates a "The method ->dt_from_string is not covered by tests!" error. Because we are hitting AUTOLOAD (??). But the POD is clear, we shouldn't https://perldoc.perl.org/perlsub#Autoloading And, we are using dt_from_string that way from Koha::Object (singular) and it's working perfectly. So, what's going on? Maybe coming from Koha::DateUtils that is not using EXPORT_OK? I tried, see bug 28931. You can apply the patch from there and try again with the 3 lines script. Unfortunately, it's not fixed, but maybe part of the solution... I need more eyes one this one, please help! """ I didn't receive any feedback and decided to abandon the investigation and push the (meaningless) fix.
(In reply to Jonathan Druart from comment #21) > I sent the following email to the QA team on August 31: > > I didn't receive any feedback and decided to abandon the investigation and > push the (meaningless) fix. Yeah I saw it but the security stuff came after it right away and has put this one in the shadow.