At the moment, if there is no database upgrade to run, updatedatabase.pl can take over 1.1 seconds to run. If you chain together 60 runs of updatedatabase.pl via koha-upgrade-schema and "apt-get install koha-common", that takes well over 1 minute at best to check that nothing needs to be done. (In practice, I think it goes over 1 minute.) We should be short circuiting earlier because we have the code version at hand and it's easy to check the database version.
Created attachment 152543 [details] [review] Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately.
This isn't quite ready for NSO, as it needs to be polished, but I need to switch tasks right now, and I didn't want to forget to raise the ticket. This works very well. It takes about .06 seconds to run koha-upgrade-schema instead of 1.1 seconds. If you multiply that by 60, that is 3.6 seconds instead of 66 seconds. Huge difference!
(In reply to David Cook from comment #2) > This isn't quite ready for NSO, as it needs to be polished What I mean by this is just that other code in Koha should rely on Koha::Installer::TransformToNum. At the moment, Koha::Installer::TransformToNum is copy/pasted from C4::Installer, which is obviously not OK.
Created attachment 152995 [details] [review] Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if an update is needed (based off database version and code version). It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test C4::Installer, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed
This doesn't take into account "atomic updates" for developers though... But that should be easy enough to take into account...
(In reply to David Cook from comment #5) > This doesn't take into account "atomic updates" for developers though... > > But that should be easy enough to take into account... Hmm taking "atomic updates" into account makes it significantly slower as the "require C4::Context" alone adds about .25 seconds, which becomes significant when you multiply it against higher numbers... But "require Koha::Config" is very fast...
So I've just made a wild discovery... I'm adding a function to determine whether or not there are any atomic updates. The check is actually very quick, but when you return a non-null value - even if it's an empty arrayref, the time slows down by 1.152 seconds. It's bizarre...
(In reply to David Cook from comment #7) > So I've just made a wild discovery... > > I'm adding a function to determine whether or not there are any atomic > updates. The check is actually very quick, but when you return a non-null > value - even if it's an empty arrayref, the time slows down by 1.152 seconds. > > It's bizarre... Ah wait(In reply to David Cook from comment #7) > So I've just made a wild discovery... > > I'm adding a function to determine whether or not there are any atomic > updates. The check is actually very quick, but when you return a non-null > value - even if it's an empty arrayref, the time slows down by 1.152 seconds. > > It's bizarre... Ah wait it was obvious in the end. If something seems magical, you're clearly just missing something obvious hehe.
Created attachment 152996 [details] [review] Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if a db or atomic update is needed. It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. It also moves the logic for C4::Installer::get_atomic_updates to Koha::Installer::get_atomic_updates for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test db updates, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed 6. To test atomic updates: cp installer/data/mysql/atomicupdate/skeleton.pl \ installer/data/mysql/atomicupdate/bug_34088.pl 7. Run `time koha-upgrade-schema kohadev` 8. Note that it takes over 1 second to run and the atomic update is attempted
Created attachment 152997 [details] [review] Bug 34088: Fix qa script issues
Created attachment 152998 [details] [review] Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if a db or atomic update is needed. It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. It also moves the logic for C4::Installer::get_atomic_updates to Koha::Installer::get_atomic_updates for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test db updates, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed 6. To test atomic updates: cp installer/data/mysql/atomicupdate/skeleton.pl \ installer/data/mysql/atomicupdate/bug_34088.pl 7. Run `time koha-upgrade-schema kohadev` 8. Note that it takes over 1 second to run and the atomic update is attempted
Created attachment 154230 [details] [review] Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if a db or atomic update is needed. It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. It also moves the logic for C4::Installer::get_atomic_updates to Koha::Installer::get_atomic_updates for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test db updates, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed 6. To test atomic updates: cp installer/data/mysql/atomicupdate/skeleton.pl \ installer/data/mysql/atomicupdate/bug_34088.pl 7. Run `time koha-upgrade-schema kohadev` 8. Note that it takes over 1 second to run and the atomic update is attempted Signed-off-by: Sam Lau <samalau@gmail.com>
Thanks, Sam :)
This one is a nice performance boost. I'm tempted to implement it locally, but I'd love to upstream it first.
*** Bug 26596 has been marked as a duplicate of this bug. ***
(In reply to David Cook from comment #14) > This one is a nice performance boost. I'm tempted to implement it locally, > but I'd love to upstream it first. Maybe QA it? ;)
(In reply to Katrin Fischer from comment #16) > (In reply to David Cook from comment #14) > > This one is a nice performance boost. I'm tempted to implement it locally, > > but I'd love to upstream it first. > > Maybe QA it? ;) I wrote it so I can't QA it 😅
(In reply to David Cook from comment #17) > (In reply to Katrin Fischer from comment #16) > > (In reply to David Cook from comment #14) > > > This one is a nice performance boost. I'm tempted to implement it locally, > > > but I'd love to upstream it first. > > > > Maybe QA it? ;) > > I wrote it so I can't QA it 😅 Ok, I guess that's valid :D