Bug 7276 - member entry Performance improvement
Summary: member entry Performance improvement
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Stéphane Delaune
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-29 10:16 UTC by Stéphane Delaune
Modified: 2013-12-05 19:57 UTC (History)
4 users (show)

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


Attachments
the csv file containing anonymised borrowers to import (140.36 KB, application/x-gzip)
2011-11-29 10:18 UTC, Stéphane Delaune
Details
proposed patch (2.47 KB, patch)
2011-11-29 10:36 UTC, Stéphane Delaune
Details | Diff | Splinter Review
Bug 7276 : member entry Performance improvement (2.53 KB, patch)
2011-12-06 22:03 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7276 : Follow up, adding a sub to clear the cache (1.19 KB, patch)
2011-12-06 22:08 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stéphane Delaune 2011-11-29 10:16:38 UTC
Before this patch : 9619 borrowers added in 31 Minutes
After : 68 seconds.
Comment 1 Stéphane Delaune 2011-11-29 10:18:21 UTC
Created attachment 6453 [details]
the csv file containing anonymised borrowers to import
Comment 2 Stéphane Delaune 2011-11-29 10:36:10 UTC Comment hidden (obsolete)
Comment 3 Stéphane Delaune 2011-12-02 11:27:42 UTC
The fix change the addmember sub, add a borrower in koha to test it
Comment 4 Ian Walls 2011-12-02 14:21:30 UTC
resetting priority so Patch Status will show up.

Very encouraging looking patch.  Please test thoroughly in not only Member Entry, but also Member search and acquisitions, as this change may affect those areas as well.
Comment 5 Paul Poulain 2011-12-05 20:47:45 UTC
This patch show a big improvement in performance when we bulk load patrons, using a specific script we're using at BibLibre when doing our migrations. I think the improvement can be seen in tools > import patrons, when loading large sets of patrons.

Note this patch is a porting of what we've made in git.biblibre.com and use in productions for months (if not years)
Comment 6 Chris Cormack 2011-12-06 21:58:50 UTC
This patch looks good, but as we are moving to a persistent running environment for Koha, we need to make sure we have a way to clear the table structure from the variable. 

So I will submit a follow up to clear the variable, which can be called by the update scripts when it changes the table structure. Similar to clear_syspref_cache in C4::Context

This patch does give a huge improvement in patron import though, so I am going to sign off and send that follow up.
Comment 7 Chris Cormack 2011-12-06 22:03:01 UTC
Created attachment 6623 [details] [review]
Bug 7276 : member entry Performance improvement

Before this patch : 9619 borrowers added in 31 Minutes, After : 68 seconds.
This adds Hashref to table structure in C4::SQLHelper to speed up bulk edits.

Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 8 Chris Cormack 2011-12-06 22:08:07 UTC
Created attachment 6624 [details] [review]
Bug 7276 : Follow up, adding a sub to clear the cache
Comment 9 Chris Cormack 2011-12-06 22:08:33 UTC
Only the follow up needs sign off
Comment 10 Paul Poulain 2011-12-09 15:12:50 UTC
QA comment

about the follow-up = the initial patch caches the database structure for up to 10 minuts. It means the clearing sub is not necessary imo, even under a persistent env, that will work. It will just be needed to wait for up to 10 minuts to get a fresh env.

Chris, can you confirm you've seen the 10 minuts limits, and explain why it should not be enough ?

(follow-up OK and everything passed QA though)
Comment 11 Chris Cormack 2011-12-09 16:55:11 UTC
(In reply to comment #10)
> QA comment
> 
> about the follow-up = the initial patch caches the database structure for up to
> 10 minuts. It means the clearing sub is not necessary imo, even under a
> persistent env, that will work. It will just be needed to wait for up to 10
> minuts to get a fresh env.
> 
> Chris, can you confirm you've seen the 10 minuts limits, and explain why it
> should not be enough ?
> 
> (follow-up OK and everything passed QA though)

We are moving slowly to a persistent model, be it mod_perl, plack, fastcgi ..whatever. As such variables declared like this hash need to be able to be wiped. This has nothing to do with the memcached caching, but that we are declaring a variable, that when we are running in persistent mode will persist.

So, we should always clean up after ourselves or we will get inconsistent and wrong behaviour when running in a persistent mode.
Comment 12 Paul Poulain 2011-12-09 17:12:46 UTC
OK, I undestand what I missed: cleaning the variable is necessary, otherwise, it will stay forever in your variable, which is bad. It has nothing to do with memcache

PS: maybe we should write guidelines to manage persistent variables
PS2: with a persistent model, will memcache still be usefull ?

(i'll push the patch on monday)
Comment 13 Chris Cormack 2011-12-10 09:13:33 UTC
(In reply to comment #12)
> OK, I undestand what I missed: cleaning the variable is necessary, otherwise,
> it will stay forever in your variable, which is bad. It has nothing to do with
> memcache
> 
> PS: maybe we should write guidelines to manage persistent variables

Probably a good idea.


> PS2: with a persistent model, will memcache still be usefull ?
> 
Yes, even with persistence, threads die after a while and respawn. So it is still useful.
Comment 14 Chris Cormack 2012-01-17 19:10:23 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > OK, I undestand what I missed: cleaning the variable is necessary, otherwise,
> > it will stay forever in your variable, which is bad. It has nothing to do with
> > memcache
> > 
> > PS: maybe we should write guidelines to manage persistent variables
> 
> Probably a good idea.
> 
> 
> > PS2: with a persistent model, will memcache still be usefull ?
> > 
> Yes, even with persistence, threads die after a while and respawn. So it is
> still useful.

Following up, FWIW on some of huge traffic sites (not Koha sites but other sites) we run with persistance, and memcached. When you move to a cluster model this really becomes useful, but even single server, removing a lot of unnessecary reads from your db helps it do writes much faster.