Bug 19819 - C4::Context->dbh is unreasonably slow
Summary: C4::Context->dbh is unreasonably slow
Status: RESOLVED DUPLICATE of bug 16156
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-15 10:57 UTC by David Gustafsson
Modified: 2019-11-27 10:37 UTC (History)
5 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 19819: C4::Context->dbh is unreasonably slow (943 bytes, patch)
2017-12-15 11:03 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19819: C4::Context->dbh is unreasonably slow (992 bytes, patch)
2018-01-03 14:58 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2017-12-15 10:57:45 UTC
I have been profiling Koha the last day or so to try to fix some bottlenecks increasing performance of bulk-processing of data. Discovered that C4::Context->dbh was unexpectedly slow (Koha::Database->schema->storage->dbh is the culprit). If it is safe to do so, performance can be greatly improved by locally caching $dbh in a state variable. Patch will be provided shortly.
Comment 1 David Gustafsson 2017-12-15 11:03:05 UTC
Created attachment 69822 [details] [review]
Bug 19819: C4::Context->dbh is unreasonably slow

Locally cache Koha::Database->schema->storage->dbh in state variable in
C4::Context->dbh to get rid of retrieval overhead
Comment 2 Josef Moravec 2017-12-17 11:35:50 UTC
Is this ready to test?

What should the test plan looks like?
Comment 3 David Gustafsson 2017-12-17 17:32:45 UTC
Yes, pretty hard to write a test plan for this. But I guess I can post a script on monday which can be run to illustrate the performance differance.
Comment 4 David Gustafsson 2017-12-18 12:55:38 UTC
Regarding how to test: time this script (for example) https://gist.github.com/gnucifer/8a9a26de02d2e344c99398a19fc8df71 in koha-shell wihtout the patch applied:

`koha-shell kohadev -c "time perl context_dbh_text.pl`

On my machine this takes about 17.2 seconds.

Apply patch, run again. For me with patch applied time decreases to about 10.2 seconds.
Comment 5 David Gustafsson 2017-12-18 12:57:30 UTC
Command should be:
`koha-shell <koha_instance_name> -c "time perl context_dbh_text.pl"`
Comment 6 David Gustafsson 2017-12-18 13:21:48 UTC
`koha-shell <koha_instance_name> -c "time perl context_dbh_test.pl"`
Comment 7 Josef Moravec 2017-12-19 08:18:55 UTC
My times:

Before:
real	0m16.295s
user	0m11.076s
sys	0m1.656s

After patch:
real	0m8.858s
user	0m6.044s
sys	0m0.824s
Comment 8 Josef Moravec 2018-01-03 14:58:16 UTC
Created attachment 70249 [details] [review]
Bug 19819: C4::Context->dbh is unreasonably slow

Locally cache Koha::Database->schema->storage->dbh in state variable in
C4::Context->dbh to get rid of retrieval overhead

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Jonathan Druart 2018-01-03 16:07:06 UTC
Did you try this patch in production? I am expecting regressions on the issues with had on bug 14778 (like "server has gone away").
Comment 10 David Gustafsson 2018-01-03 16:24:45 UTC
Good catch, I have only run it locally, but it will be included the next release for us. I'm not very knowledge about the inner working of Plack, but if the connection is persisted over time, and never closed, I understand how this can be an issue. I could try too look into this some more trying to figure out if the current fix if safe, and if not if there is some other way of going about this.
Comment 11 Julian Maurice 2018-03-09 11:12:47 UTC
I can confirm that with this patch, if I restart mysql server, koha is no longer able to connect to mysql until I restart (or reload) starman.

Looking at the test script and times, this seems to save ~40μs by call (~8s / 200000 calls). That's not much. Do we know how many times by page dbh get called ?

Changing status to Failed QA. The patch introduces a regression and I think we need more precise profiling data before changing the implementation of dbh.
Comment 12 David Gustafsson 2018-03-09 13:35:56 UTC
It actually has an significant impact on indexing (about 5 - 10% of execution time I think), but I have not had the time to think of a proper fix. I probably will some time in the near future, but I won't protest if someone wants to close the issue. I can open a new one when have a proper fix in that case.
Comment 13 Julian Maurice 2018-03-09 13:57:27 UTC
(In reply to David Gustafsson from comment #12)
> It actually has an significant impact on indexing (about 5 - 10% of
> execution time I think), but I have not had the time to think of a proper
> fix. I probably will some time in the near future, but I won't protest if
> someone wants to close the issue. I can open a new one when have a proper
> fix in that case.

No one is going to close the bug report, but the current patch cannot be accepted as it is. The bug report will stay open until you (or someone else) can provide a proper fix.
Comment 14 David Cook 2019-09-02 06:47:47 UTC
(In reply to Jonathan Druart from comment #9)
> Did you try this patch in production? I am expecting regressions on the
> issues with had on bug 14778 (like "server has gone away").

This was exactly what I was going to say. Database connection caching is a nice idea but not something you want to implement naively. 

I'd suggest not trying to re-invent the wheel on this one, and look somewhere else for database connection pooling. 

And actually looking at C4::Context::dbh... I see that Koha::Database->schema->storage->dbh... which does cache the $schema object in a package level variable.

This bug report was opened nearly 2 years ago, so maybe it is no longer relevant.
Comment 15 Jonathan Druart 2019-10-14 08:06:28 UTC
Agreed.
The cache is done already, I am not sure we can do better here.
I am closing it for now, but feel free to reopen if you have something to suggest.
Comment 16 Jonathan Druart 2019-11-27 10:37:15 UTC
I forgot about bug 16156, it has a patch that was waiting for feedback.
The idea was similar, but $dbh cache would be flushed when needed.

*** This bug has been marked as a duplicate of bug 16156 ***