C4::Letters::getletter use a package variable (%letter) to cache letter returned by the subroutine. I have not found any direct issues caused by that but it is safer to remove it. It won't be a big deal to hit the DBMS to get a valid letter when needed.
Created attachment 51218 [details] [review] Bug 16441: Do not use a package variable to cache C4::Letters::getletter C4::Letters::getletter use a package variable (%letter) to cache letter returned by the subroutine. I have not found any direct issues caused by that but it is safer to remove it. It won't be a big deal to hit the DBMS to get a valid letter when needed. No test plan here, just confirm that the changes make sense.
Created attachment 51703 [details] [review] [SIGNED-OFF] Bug 16441: Do not use a package variable to cache C4::Letters::getletter C4::Letters::getletter use a package variable (%letter) to cache letter returned by the subroutine. I have not found any direct issues caused by that but it is safer to remove it. It won't be a big deal to hit the DBMS to get a valid letter when needed. No test plan here, just confirm that the changes make sense. Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
(In reply to Jonathan Druart from comment #0) > It won't be a big deal to hit the DBMS to get a valid letter when needed. Hm. Not sure. getletter is called by GetPreparedLetter, SendCirculationAlert, etc. In loops ? -# FIXME: using our here means that a Plack server will need to be -# restarted fairly regularly when working with this routine. -# A better option would be to use Koha::Cache and use a cache -# that actually works in a persistent environment, but as a -# short-term fix, our will work. Why not use this opportunity to add caching?
(In reply to Marcel de Rooy from comment #3) > (In reply to Jonathan Druart from comment #0) > > It won't be a big deal to hit the DBMS to get a valid letter when needed. > > Hm. Not sure. getletter is called by GetPreparedLetter, > SendCirculationAlert, etc. In loops ? > > -# FIXME: using our here means that a Plack server will need to be > -# restarted fairly regularly when working with this routine. > -# A better option would be to use Koha::Cache and use a cache > -# that actually works in a persistent environment, but as a > -# short-term fix, our will work. > > Why not use this opportunity to add caching? Because I have not found anywhere it's called many times. As I wrote previously, I am not sure it'd be interesting to cache it, but I may be wrong. If we decide to cache it, we need to make sure the values in cache are deleted when letters are updated/deleted. Anyway it will be a much bigger patch and more an enhancement than a bugfix.
(In reply to Jonathan Druart from comment #4) > Because I have not found anywhere it's called many times. As I wrote > previously, I am not sure it'd be interesting to cache it, but I may be > wrong. > If we decide to cache it, we need to make sure the values in cache are > deleted when letters are updated/deleted. > Anyway it will be a much bigger patch and more an enhancement than a bugfix. Yes, it would be more work. But this patch affects performance. Note that I am not sure if this is actually a bugfix. The comment about regularly restarting plack is probably outdated. If we let each worker process only handle 50 requests, you do not need to restart plack very often. Since a new worker starts with an empty 'cache', you will not long see the old letter contents anymore.
(In reply to Marcel de Rooy from comment #5) > The comment about regularly restarting plack is probably outdated. If we let > each worker process only handle 50 requests, you do not need to restart > plack very often. --max-requests Number of the requests to process per one worker process. Defaults to 1000. git grep max-requests: debian/scripts/koha-plack: STARMANOPTS="-M FindBin --max-requests 50 --workers 2 \ misc/plack/plackup.sh:# --max-requests 50 decreased from 1000 to keep memory usage sane misc/plack/plackup.sh:opt="$opt --server Starman -M FindBin --max-requests 50 --workers 4"
Created attachment 52120 [details] [review] Bug 16441: Fix Letters.t These tests were wrong, but the package variable used to cache the letters hid the problem.
Working on tests for bug 16624, I have found that the tests were wrong, but we were not aware of that. Another point to remove this cache :)
Created attachment 52130 [details] [review] Bug 16441: Fix Letters.t These tests were wrong, but the package variable used to cache the letters hid the problem. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Planning to come back here soon :)
Created attachment 52217 [details] [review] Bug 16441: Do not use a package variable to cache C4::Letters::getletter C4::Letters::getletter use a package variable (%letter) to cache letter returned by the subroutine. I have not found any direct issues caused by that but it is safer to remove it. It won't be a big deal to hit the DBMS to get a valid letter when needed. No test plan here, just confirm that the changes make sense. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Confirm that performance loss is just a millisecond or so per subsequent call of getletter.
Created attachment 52218 [details] [review] Bug 16441: Fix Letters.t These tests were wrong, but the package variable used to cache the letters hid the problem. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 16.11, thanks Jonathan!
Pushed in 16.05. Will be in 16.05.01.
Patches pushed to 3.22.x, will be in 3.22.8