Bug 20116 - Improve performance by caching the language list
Summary: Improve performance by caching the language list
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 20115
Blocks: 7923
  Show dependency treegraph
 
Reported: 2018-01-31 18:20 UTC by Jonathan Druart
Modified: 2022-10-18 22:25 UTC (History)
11 users (show)

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


Attachments
Bug 20116: Prepare the ground (3.72 KB, patch)
2018-01-31 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.73 KB, patch)
2018-01-31 18:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.73 KB, patch)
2018-04-02 14:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.72 KB, patch)
2018-04-04 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.73 KB, patch)
2018-04-04 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.72 KB, patch)
2018-05-16 20:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.83 KB, patch)
2018-05-16 20:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Flush the caches (2.37 KB, patch)
2018-05-16 20:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.89 KB, patch)
2018-05-16 20:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.76 KB, patch)
2019-04-17 14:55 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20116: Flush the caches (2.42 KB, patch)
2019-04-17 14:55 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.93 KB, patch)
2019-04-17 14:55 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.76 KB, patch)
2019-11-04 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Flush the caches (2.42 KB, patch)
2019-11-04 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.93 KB, patch)
2019-11-04 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.81 KB, patch)
2020-02-21 20:00 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 20116: Flush the caches (2.46 KB, patch)
2020-02-21 20:00 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 20116: Cache language list (1.98 KB, patch)
2020-02-21 20:00 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 20116: Prepare the ground (3.91 KB, patch)
2020-04-15 14:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20116: Flush the caches (2.59 KB, patch)
2020-04-15 14:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20116: Cache language list (2.08 KB, patch)
2020-04-15 14:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20116: Fix package install path for clear_cache.pl (1019 bytes, patch)
2020-05-26 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-01-31 18:20:45 UTC
C4::Languages::getTranslatedLanguages calls _get_language_dirs that list directories to know the available themes.
It sounds like we could avoid that and cache it instead.
Comment 1 Jonathan Druart 2018-01-31 18:21:36 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2018-01-31 18:21:39 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2018-01-31 18:25:06 UTC
Do these patches make sense?

It seems that some call to getTranslatedLanguages are wrong:
1.
members/memberentry.pl:    my $translated_languages = C4::Languages::getTranslatedLanguages( 'opac', C4::Context->preference('template') );
# template is for the intranet, we should use opacthemes instead

2. tools/letter.pl
159         my $translated_languages =
160           C4::Languages::getTranslatedLanguages( 'opac', 
161             C4::Context->preference('template') );

Additional note: "template" and "opacthemes" are confusing, the same terms must be used
Comment 4 Jonathan Druart 2018-01-31 18:27:22 UTC
TODO: We should add a warning to koha-translate to restart memcached (or better, clear only what is needed)
Comment 5 Mark Tompsett 2018-04-02 02:18:42 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2018-04-02 14:09:42 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2018-04-02 14:09:59 UTC
(In reply to M. Tompsett from comment #5)
> Comment on attachment 71098 [details] [review] [review]
> Bug 20116: Cache language list
> 
> Review of attachment 71098 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Languages.pm
> @@ +122,3 @@
> >      if ($interface && $interface eq 'opac' ) {
> >          my $htdocs = C4::Context->config('opachtdocs');
> >          @languages = _get_opac_language_dirs( $htdocs, $theme );
> 
> Forgot to delete this line, I think.

Indeed, thanks for catching it!
Comment 8 Josef Moravec 2018-04-04 11:28:23 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2018-04-04 12:54:59 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2018-04-04 12:55:03 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2018-04-04 12:55:31 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2018-05-16 20:34:22 UTC
Created attachment 75382 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable
Comment 13 Jonathan Druart 2018-05-16 20:34:28 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2018-05-16 20:34:32 UTC
Created attachment 75384 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent
Comment 15 Jonathan Druart 2018-05-16 20:40:25 UTC
Created attachment 75385 [details] [review]
Bug 20116: Cache language list
Comment 16 Jonathan Druart 2018-05-16 20:58:27 UTC
Before the patches, C4::Languages::getTranslatedLanguages('intranet', 'prog') took between 0.002 and 0.003s

Now it's 0.0015s (and I guess the difference can be much better when hundred of queries are made at the same time).
Comment 17 Jonathan Druart 2018-05-16 21:00:20 UTC
(Of course first hit is worse)
Comment 18 Tomás Cohen Arazi 2018-08-20 12:40:28 UTC
Jonathan, it looks great. Will you provide a couple tests for this?
Comment 19 Jonathan Druart 2018-08-24 18:38:49 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> Jonathan, it looks great. Will you provide a couple tests for this?

I do not think it can be done easily, any ideas?
Comment 20 Liz Rea 2019-04-16 17:36:01 UTC
I don't know if this matters - I gave this a go in a devbox, copied all the files in where they needed to be etc etc, and got this on a koha-translate --install de-DE:


Connection to the memcached servers '__MEMCACHED_SERVERS__' failed. Are the unix socket permissions set properly? Is the host reachable?
If you ignore this warning, you will face performance issues

Connection to the memcached servers '__MEMCACHED_SERVERS__' failed. Are the unix socket permissions set properly? Is the host reachable?
If you ignore this warning, you will face performance issues 

Now, I realise that may be because this feature wasn't installed as part of the package and thusly some data might not have been subbed in like it was supposed to be and this will work fine when it's installed properly via apt. If you think this is OK, I'll sign off on it.
Comment 21 Liz Rea 2019-04-17 14:32:41 UTC
Had a talk with Tomas, he thinks this is fine so I"ll attach a signed off one for QA to have a look at.

Liz
Comment 22 Liz Rea 2019-04-17 14:55:15 UTC
Created attachment 88200 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 23 Liz Rea 2019-04-17 14:55:24 UTC
Created attachment 88201 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 24 Liz Rea 2019-04-17 14:55:27 UTC
Created attachment 88202 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 25 Marcel de Rooy 2019-04-26 08:55:39 UTC
+    my @enabled_languages =
+      ( $interface && $interface eq 'intranet' )
+      ? split ",", C4::Context->preference('language')
+      : split ",", C4::Context->preference('opaclanguage');

Seems to reverse the previous logic. If eq opac then opac else intranet.
Comment 26 Jonathan Druart 2019-04-26 22:14:02 UTC
(In reply to Marcel de Rooy from comment #25)
> +    my @enabled_languages =
> +      ( $interface && $interface eq 'intranet' )
> +      ? split ",", C4::Context->preference('language')
> +      : split ",", C4::Context->preference('opaclanguage');
> 
> Seems to reverse the previous logic. If eq opac then opac else intranet.

I do not think I get that one, please read again the whole diff:

before:
if $interface == opac:
  use opaclanguages
elsif $interface == intranet:
  use language
else:
  use opaclanguages

after:
if $interface == intranet:
  use language
else:
  use opaclanguages

Looks equal for me.
Comment 27 Marcel de Rooy 2019-05-03 09:44:20 UTC
(In reply to Jonathan Druart from comment #26)
> (In reply to Marcel de Rooy from comment #25)
> > +    my @enabled_languages =
> > +      ( $interface && $interface eq 'intranet' )
> > +      ? split ",", C4::Context->preference('language')
> > +      : split ",", C4::Context->preference('opaclanguage');
> > 
> > Seems to reverse the previous logic. If eq opac then opac else intranet.
> 
> I do not think I get that one, please read again the whole diff:
> 
> before:
> if $interface == opac:
>   use opaclanguages
> elsif $interface == intranet:
>   use language
> else:
>   use opaclanguages
> 
> after:
> if $interface == intranet:
>   use language
> else:
>   use opaclanguages
> 
> Looks equal for me.

Seems so. OK
Comment 28 Marcel de Rooy 2019-05-03 09:51:50 UTC
Hmm What about adding a few tests now? :)
Comment 29 Marcel de Rooy 2019-05-03 09:52:42 UTC
(In reply to Jonathan Druart from comment #19)
> (In reply to Tomás Cohen Arazi from comment #18)
> > Jonathan, it looks great. Will you provide a couple tests for this?
> 
> I do not think it can be done easily, any ideas?

iirc other subs also are tested with mocking cache
Comment 30 Jonathan Druart 2019-05-03 12:57:29 UTC
Hi Marcel,
I have still no idea how to write useful tests for this change, can you give me more details please?
Comment 31 Marcel de Rooy 2019-05-06 06:30:06 UTC
(In reply to Jonathan Druart from comment #30)
> Hi Marcel,
> I have still no idea how to write useful tests for this change, can you give
> me more details please?

Mock the directory structure too where Koha looks for languages. Remove entries from the cache, etc.
Comment 32 Jonathan Druart 2019-05-06 15:36:05 UTC
I do not see the point of those tests. The patches are just caching a value, there is nothing to prove.
Comment 33 Guillaume 2019-05-12 18:53:44 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2019-05-13 00:11:34 UTC Comment hidden (obsolete)
Comment 35 Guillaume 2019-05-13 18:11:05 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2019-05-13 23:40:38 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2019-06-11 16:44:31 UTC
Asking for another QA feedback.
Comment 38 Mark Tompsett 2019-06-17 16:10:35 UTC
Comment on attachment 88201 [details] [review]
Bug 20116: Flush the caches

Review of attachment 88201 [details] [review]:
-----------------------------------------------------------------

::: debian/scripts/koha-translate
@@ +233,5 @@
>  
> +flush_cache()
> +{
> +    if [ "$dev" = "" ]; then
> +        koha-foreach --enabled "$KOHA_HOME/misc/bin/clear_cache.pl"

Is $KOHA_HOME/misc/bin going to exist? And is clear_cache.pl going to get put there? Something is fishy for running this live.
Comment 39 Mark Tompsett 2019-06-17 16:11:53 UTC
In addition to the whole $KOHA_HOME path existing in live problem, perhaps the flushing could be split to a separate patch, so that bug 20884 could depend on that, and not this?
Comment 40 Jonathan Druart 2019-06-20 01:18:46 UTC
(In reply to M. Tompsett from comment #38)
> Comment on attachment 88201 [details] [review] [review]
> Bug 20116: Flush the caches
> 
> Review of attachment 88201 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: debian/scripts/koha-translate
> @@ +233,5 @@
> >  
> > +flush_cache()
> > +{
> > +    if [ "$dev" = "" ]; then
> > +        koha-foreach --enabled "$KOHA_HOME/misc/bin/clear_cache.pl"
> 
> Is $KOHA_HOME/misc/bin going to exist? And is clear_cache.pl going to get
> put there? Something is fishy for running this live.

How that? Did you `git grep KOHA_HOME`?
Comment 41 Jonathan Druart 2019-06-20 01:20:28 UTC
(In reply to M. Tompsett from comment #39)
> In addition to the whole $KOHA_HOME path existing in live problem, perhaps
> the flushing could be split to a separate patch, so that bug 20884 could
> depend on that, and not this?

Or we push this and bug 20884 will benefit from it? :)
Comment 42 Jonathan Druart 2019-10-14 13:23:46 UTC
Still waiting for another QA feedback, back to NQA.
Comment 43 Marcel de Rooy 2019-11-01 06:51:54 UTC
(In reply to Jonathan Druart from comment #42)
> Still waiting for another QA feedback, back to NQA.

I am still hoping for tests :)
Note even that this patch set breaks a test when these patches are applied and does not when not.

t/db_dependent/Languages.t (Wstat: 512 Tests: 18 Failed: 2)
  Failed tests:  11, 13
Comment 44 Jonathan Druart 2019-11-04 10:41:00 UTC
Created attachment 94995 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 45 Jonathan Druart 2019-11-04 10:41:04 UTC
Created attachment 94996 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 46 Jonathan Druart 2019-11-04 10:41:09 UTC
Created attachment 94997 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 47 Jonathan Druart 2019-11-04 10:43:27 UTC
(In reply to Marcel de Rooy from comment #43)
> (In reply to Jonathan Druart from comment #42)
> > Still waiting for another QA feedback, back to NQA.
> 
> I am still hoping for tests :)

Hi Marcel,
Yes I know, but I still not know how to write them usefully. I switched back to SO to get the point of view of an other QAer.
See comments 28 to 32.

> Note even that this patch set breaks a test when these patches are applied
> and does not when not.
> 
> t/db_dependent/Languages.t (Wstat: 512 Tests: 18 Failed: 2)
>   Failed tests:  11, 13

Thanks for catching that, fixed.
Comment 48 Katrin Fischer 2019-11-24 10:34:37 UTC
I tried testing this, not sure if what I saw is to be expected:

sudo koha-translate --update de-DE --dev kohadev

German is updated, activated via sysprefs

sudo koha-translate --remove de-DE --dev kohadev
sudo koha-translate --list --dev kohadev

Only shows inactive fr-FR.

But: In the OPAC German is still visible and only disappears when I run restart_all.

I reinstalled the language:
sudo koha-translate --install de-DE --dev kohadev

As an additional find, searching the language preferences from the German interface exploded in an error (search for langu):

Can't use an undefined value as an ARRAY reference at /usr/lib/x86_64-linux-gnu/perl5/5.24/YAML/Syck.pm line 76

Jonathan, can you have a look? Just switching to FQA temporarily.
Comment 49 Jonathan Druart 2019-11-25 11:16:00 UTC
Hi Katrin,
My bet is that you tested without the correct koha-translate file. Are you sure you copied the one patched with this change?
You can use reset_all to make sure it will be copied.

Tested right now and it works as expected.
Comment 50 Katrin Fischer 2019-11-25 11:20:56 UTC
That's correct, I didn't copy the file. From where to where do you have to copy it? A test plan would be helpful.
Comment 51 Jonathan Druart 2019-11-25 12:10:42 UTC
(In reply to Katrin Fischer from comment #50)
> That's correct, I didn't copy the file. From where to where do you have to
> copy it? A test plan would be helpful.

Koha scripts are in /usr/sbin/koha-translate (`which koha-translate` tell you where it is).

A test plan could be:
0/ Apply the patch and `reset_all`
1/ Install languages, enable them at staff and OPAC
2/ Confirm you have the languages listed at the bottom
3/ Uninstall one language
4/ Confirm that it does not longer appear in the language at the bottom of the pages and in the prefs

Also play with koha-translate --install, --remove and --list

Additionally, QA will make sure that the changes make sense.
Comment 52 Bouzid Fergani 2020-02-21 20:00:26 UTC
Created attachment 99431 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 53 Bouzid Fergani 2020-02-21 20:00:32 UTC
Created attachment 99432 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 54 Bouzid Fergani 2020-02-21 20:00:37 UTC
Created attachment 99433 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>
Comment 55 Marcel de Rooy 2020-04-15 12:35:22 UTC
QAing
Comment 56 Marcel de Rooy 2020-04-15 14:00:47 UTC
Created attachment 103007 [details] [review]
Bug 20116: Prepare the ground

Just refactoring to make the next patch more readable

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Marcel de Rooy 2020-04-15 14:00:52 UTC
Created attachment 103008 [details] [review]
Bug 20116: Flush the caches

When languages are added or removed when need to flush the caches to
keep the interface consistent

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Marcel de Rooy 2020-04-15 14:00:56 UTC
Created attachment 103009 [details] [review]
Bug 20116: Cache language list

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Bouzid Fergani <bouzid.fergani@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Martin Renvoize 2020-04-17 08:29:24 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 60 Joy Nelson 2020-05-05 23:06:59 UTC
enhancement not backported to 19.11.x
Comment 61 Jonathan Druart 2020-05-26 12:38:44 UTC
On a package install I get:

root@koha-debian10:~# koha-translate --install es-ES                                                                                                                                                              
/bin/bash: /usr/share/koha/misc/bin/clear_cache.pl: No such file or directory


The script is in /usr/share/koha/bin/clear_cache.pl
Comment 62 Jonathan Druart 2020-05-26 12:41:48 UTC
Created attachment 105359 [details] [review]
Bug 20116: Fix package install path for clear_cache.pl

root@koha-debian10:~# koha-translate --install es-ES
/bin/bash: /usr/share/koha/misc/bin/clear_cache.pl: No such file or directory

The script is in /usr/share/koha/bin/clear_cache.pl
Comment 63 Jonathan Druart 2020-05-26 12:45:14 UTC
Last patch pushed to master for 20.05