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 it's runtime is measured in microseconds instead of hundreds of miliseconds, making it easily a 1000x+ improvement. The nested CASE statements are a bit verbose, but not too bad, I figured it might be better to do that rather than select all the vars as temporary columns and later remove them or re-create the new object to return. Instead, no temporary variables that one has to remove will linger... Please note that I didn't test the actual code since I don't have a working ktd installation by hand, but I did test the SQL statement thoroughly.
Created attachment 175262 [details] [review] [PATCH] Bug 38646: rewrite C4::Languages::getLanguages to use a single cachable SQL query
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?
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
Whooo good catch, I will try