Bug 38646

Summary:	C4::Languages::getLanguages is very unreasonably slow (100+ ms)
Product:	Koha	Reporter:	Michał <schodkowy.omegi-0r>
Component:	Searching	Assignee:	Michał <schodkowy.omegi-0r>
Status:	Needs Signoff ---	QA Contact:	Testopia <testopia>
Severity:	normal
Priority:	P5 - low	CC:	david, fridolin.somers, januszop, jonathan.druart, roman.dolny, schodkowy.omegi-0r
Version:	Main
Hardware:	All
OS:	All
See Also:	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38630
Change sponsored?:	---	Patch complexity:	---
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:
Circulation function:
Attachments:	[PATCH] Bug 38646: rewrite C4::Languages::getLanguages to use a single cachable SQL query

Description Michał 2024-12-06 14:58:31 UTC

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.

Comment 1 Michał 2024-12-06 15:06:13 UTC

Created attachment 175262 [details] [review]
[PATCH] Bug 38646: rewrite C4::Languages::getLanguages to use a single cachable SQL query

Comment 2 Michał 2024-12-06 15:07:46 UTC

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?

Comment 3 David Nind 2024-12-06 22:36:53 UTC

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

Comment 4 Fridolin Somers 2024-12-09 11:58:18 UTC

Whooo good catch, I will try