https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #2 from Michał <schodkowy.omegi-0r@icloud.com> --- Also I was thinking of alternative approach to handle AdvancedSearchLanguages, but it seemed more sketchy and with ~200 langs performance of FIND_IN_SET shouldn't be any problem whatsoever. The alternative idea was: my @search_langs; my $language_list_cond = ''; my $language_list = $isFiltered ? C4::Context->preference("AdvancedSearchLanguages") : undef; if ($language_list) { @search_langs = split(/,|\|/, $language_list); my $params = join ', ' => ('?') x @search_langs; $language_list_cond = 'AND language_rfc4646_to_iso639.iso639_2_code IN ($params)'; } #... -- Grab ISO code for language INNER JOIN language_rfc4646_to_iso639 ON language_rfc4646_to_iso639.rfc4646_subtag = language_subtag_registry.subtag AND language_rfc4646_to_iso639.iso639_2_code IS NOT NULL ${language_list_cond} #... if ($language_list) { $sth->execute(@search_langs, $current_language); } else { $sth->execute($current_language); } ^Unless someone believes the above is better? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #3 from David Nind <david@davidnind.com> --- Hi Michal, Thanks for working on improving the performance, and submitting a patch! I can't comment on the code and approach, but I'm sure others will! I mainly help with testing bugs and signing them off before they go to QA. If you could add a test plan, that would be great. This should ideally set out how to replicate the issue, apply the patch, and then show how it fixes or improves things. See the commit message guidelines for what to include https://wiki.koha-community.org/wiki/Commit_messages Feel free to ask on the bug if you need any help, or on the Koha Community's Development chat channel https://chat.koha-community.org/ Some other links: - Most Koha developers use koha-testing-docker (KTD) for their development environment - it gives you a complete Koha environment with all the required tools including a working instance and sample data, see https://gitlab.com/koha-community/koha-testing-docker - This page about the development workflow may be useful if you are unfamiliar with how Koha does things https://wiki.koha-community.org/wiki/Development_workflow David Nind -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Needs Signoff |Failed QA --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- I'm afraid we have a test failure here: C4::Languages::getLanguages(): DBI Exception: DBD::mysql::st execute failed: Non-grouping field 'iso639_2_code' is used in HAVING clause at t/db_dependent/Languages.t line 46 Remember we have ONLY_FULL_GROUP_BY enabled on SQL connections for the test suite.. hopefully it's a simple fix without degrading the performance gain to much. I'd love to see a benchmark too.. failing QA for now for the failing test. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176207 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176207&action=edit Bug 38646: rewrite C4::Languages::getLanguages to use a single cachable SQL query My profiling with NYTProf has found that the call to C4::Languages::getLanguages is unreasonably slow, taking over 100 ms (and that's just on a single call of it apparently!!!). Indeed, the way it's coded, it'd run over 540 SQL queries for no good reason, slowing the already slow catalog search page loading further down. This patch is gonna replace the inefficient and complicated function logic with proper SQL statement that joins the relevant functions. It also fixes the bug where user's native language would be displayed with native language in the parenthesis as well, such as "Polski (Polski)" instead of just "Polski". The whole query is pretty fast and it gets cached by MySQL server, meaning its runtime is measured in microseconds instead of hundreds of miliseconds, making it easily a 1000x+ improvement. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #9 from Michał <schodkowy.omegi-0r@icloud.com> --- Awesome, thank you. I regret a bit not doing comparison benchmarks right away with the improved functions or not writing down how I got NYTProf working with Plack right away, while now I don't have access to the dev machine with the setup for some time. As for the tests, since you mention you want to expand them, a bit of a joke but a bit of actually serious idea: there could be some test that'd measure how many SQL queries some given function resulted in being made, or how long in total they took. That could have the potential to catch some regressions. Of course a simple example is this very function where a test could say it expects no more than say 5 sql queries to be run under it, rather than few hundred. But it could expand to some other more advanced code paths and call chains, where a regression suddenly causing something to be way less efficient could otherwise be much harder to catch (if some unrelated function suddenly calls something that would do many more queries down the call chain after a change, where at the time of coding the other function things were okay there). Would that make any sense? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- Another thing to note... getLanguages() is called both when loading the advanced search and when running a search and getting the results, but the output of this function is only ever used by the advanced search. We should move this function / wrap it in a $template_type check like we do with other functions that are only used in 1 of the 2 modes. It would actually be interesting to do a thoroughly analysis of the search.pl and opac-search.pl to see how much code could be moved into these types of conditions to further increase performance. I'll raise a separate ticket for that, since it's not intrinsically related to this one... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #13 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #12)

(In reply to Martin Renvoize (ashimema) from comment #5)

...

I'd love to see a benchmark too.. failing QA for now for the failing test.

So I just did a patch... and the performance difference when running catalogue/search.pl with getLanguages() and without getLanguages() is negligible in koha-testing-docker.

I had runs where it was slower running without getLanguages(), so I can't attribute the presence/absence of getLanguages() to any speed differences.

That said... I think it's a good idea to reduce our volume of DB calls wherever possible, and maybe with a Koha DB under load you'd see a noticeable speed difference.

See also bug 16734. (It looks like that patch was mistakenly never added to Koha despite being marked as pushed, but it's good to see another mention about the performance difference and utility of the change) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize (ashimema) from comment #8)

Working on a straight QA here whilst my head is in it... TDQA (Test Driven QA ;P - I.e I'm currently ensuring we have more comprehensive tests here.

Good call. I took a look at t/db_dependent/Languages.t and felt like we weren't really testing getLanguages() in a thorough way -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176207|0 |1 is obsolete| | --- Comment #16 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176228 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176228&action=edit Bug 38646: rewrite C4::Languages::getLanguages to use a single cachable SQL query My profiling with NYTProf has found that the call to C4::Languages::getLanguages is unreasonably slow, taking over 100 ms (and that's just on a single call of it apparently!!!). Indeed, the way it's coded, it'd run over 540 SQL queries for no good reason, slowing the already slow catalog search page loading further down. This patch is gonna replace the inefficient and complicated function logic with proper SQL statement that joins the relevant functions. It also fixes the bug where user's native language would be displayed with native language in the parenthesis as well, such as "Polski (Polski)" instead of just "Polski". The whole query is pretty fast and it gets cached by MySQL server, meaning its runtime is measured in microseconds instead of hundreds of miliseconds, making it easily a 1000x+ improvement. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #18 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Right.. we now have proper coverage or the expected return. You mentioned you'd changed the return value so the test fails after applying your patches as expected. I'm not working through whether I agree with the change as I have a nagging feeling there's a reason for including the additional information consistently. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176228|0 |1 is obsolete| | --- Comment #20 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176232 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176232&action=edit Bug 38646: rewrite C4::Languages::getLanguages to use a single cachable SQL query My profiling with NYTProf has found that the call to C4::Languages::getLanguages is unreasonably slow, taking over 100 ms (and that's just on a single call of it apparently!!!). Indeed, the way it's coded, it'd run over 540 SQL queries for no good reason, slowing the already slow catalog search page loading further down. This patch is gonna replace the inefficient and complicated function logic with proper SQL statement that joins the relevant functions. It also fixes the bug where user's native language would be displayed with native language in the parenthesis as well, such as "Polski (Polski)" instead of just "Polski". The whole query is pretty fast and it gets cached by MySQL server, meaning its runtime is measured in microseconds instead of hundreds of miliseconds, making it easily a 1000x+ improvement. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #22 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176234 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176234&action=edit Bug 38646: Update unit test for change This drops the bracketed translation in the tests in language_description where the bracketed language name matches the preceeding one. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #23 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 25.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #25 from Michał <schodkowy.omegi-0r@icloud.com> --- That's really weird in principle since these tests were added by Martin together with the change in this exact bug, I assume he tested them to pass before attaching. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #27 from Michał <schodkowy.omegi-0r@icloud.com> --- Okay then my guess right now for most likely cause is that `cmp_deeply` expects the array elements to be in the same order in $languages as $expected. And order of elements inside of actual objects doesn't matter, so it's possible `description` just happens to be first. I don't see any other logical explanation. This is what I expected, but wasn't sure why, MariaDB version upgrade sounds like very plausible cause. If you or someone would happen to have environment set up, they should either inspect the output of getLanguages with these sample data entered, or just actually view the tables and/or run the query from getLanguages. My hunch is that iirc especially since there's no numerical id in these columns, that the order of elements being returned is an explicit implementation detail of the DB server which shouldn't matter. So a fix could be to find or create a test function that'd compare equality of arrays without regard to the order of elements. And I think neither the new or old getLanguages had anything that'd reorder languages from what the DB returned. Which suggests that it'd have regressed regardless of whether my patch was implemented, it's just that the new tests from Martin caught that. So if that's the cause, then we could either: - make tests not care about the order - make getLanguages query explicitly sort by modifying the SQL query And actually the latter might be more preferable now that I think about it, since the order could mess up how languages are shown in Koha interfaces on language lists!! (In one of the libraries I've heard some complaints about some language lists being out of order, on older Koha version, but I wasn't personally looking into the causes, interesting to note though) And indeed I just checked on koha-staging and also on 24.11 (so before and after patch!), on /cgi-bin/koha/opac-search.pl, language list is definitely wrong and not ordered by anything logical!!! (and the order is different on different servers) So we should probably make a patch to sort it actually and keep the deep comparison func. Now backporting a fix would be annoying, since this bug is classified as enhancement. So we either backport this + a fix, or we make two fixes targetting separately main and stable branches... (there's behavior change for displaying "English" instead of "English (English)", so it's not really a pure bug fix, if we consider bad performance a bug) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #28 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I am sorry, pretty busy so can't really dig in here. What are we missing for a patch? Could we patch the current failing test on main? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #30 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 181063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=181063&action=edit Bug 38646: (follow-up) Ensure consistent ordering of languages We add an `ORDER BY` clause to the SQL statement to ensure we consistently order accross databases by the iso code. This fixes the failing test on MariaDB as it's default sort order appears to differ from MySQL for this. This also ensure we are consistently ordering which will lead to more consistent and clear results for end users. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|25.05.00 |25.05.00,24.11.04 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paul.derscheid@lmscloud.de --- Comment #33 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Why do we still have additional_work_needed here? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 --- Comment #35 from Michał <schodkowy.omegi-0r@icloud.com> --- There was also Bug 39001 where we couldn't decide whether to order languages by their code, English name or effective localized name. Technically that bug would now be resolved as far as the undefined sorting order behavior goes, but I might rename it and keep it open, unless someone decides to close, because there might still be some merit to considering what to sort by. Technically the language code is the most stable (no way for unexpected ordering conflict depending on current session state). But then for places where actual users sees a list of languages, they might really expect to see them ordered alphabetically by how they're displayed (but we might want to handle this directly at the places that display such lists and not at getLanguages func level). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38646 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Status|Pushed to stable |Needs documenting --- Comment #36 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11.x as it wasn't backported to 24.05.x -- You are receiving this mail because: You are watching all bug changes.