GetMarcStructure has an "unsafe" param for faster cache retrieval, I have looked through all the instances where GetMarcSubfieldStructure is called, and seems to me it's safe to call get_from_cache with the "unsafe" option also in those cases. While profiling Koha I found that quite some time was actually spent deserializing MarcSubfieldStructure-hashes (that are usually pretty big), actually around 7% of total time while indexing biblios for example.
Created attachment 69823 [details] [review] Bug 19820: Add unsafe param to GetMarcSubfieldStructure Add unsafe param to GetMarcSubfieldStructure and use this options where it's safe to do so to increase performance
Is this ready to test? What is a test plan?
I don't know if there is a test plan that can be fit into a commit-message. But I can post a script tomorrow that can be used to illustrate the performance differance.
It's seems it's a little bit to hard to test on a koha-instance with only a few biblios. I tried reloading the same biblio x number of times, but there only was minor difference in performance. Obviously some kind of caching-mechanism kicks in when reloading the same one, the difference is much more obvious when loading x different biblios, so requires a database with a lot of biblios to test. Anyway, this script can be run to demonstrate the impact of this change on the GetMarcBiblio-function, which is where much of the work takes place when loading a biblio with items: https://gist.github.com/gnucifer/1bc2fa8b3f9899358074d3f6a3554948 `koha-shell <koha_instance_name> -c "time perl unsafe_test.pl"` Will get back with timings with and without patch as I need to import a larger database into my local development instance (which takes a while).
My vm-disk was too small for the dump, so will not be able to run this today.
Since I was unable to find a representative benchmark that can be run with only a few bib-posts, I created a more artificial one instead, just to illustrate the slowness of the deserialization. In a real life example the impact is not huge, but think it was about 5% of time spent during indexing for example (and most accumulated time of all functions), so not insignificant. https://gist.github.com/gnucifer/5b8c2cf0a63362d95322c6f91dd7411e Without patch: real 0m14.540s user 0m13.144s sys 0m0.168s With patch: real 0m0.689s user 0m0.576s sys 0m0.084s
Created attachment 71573 [details] [review] Bug 19820: Add unsafe param to GetMarcSubfieldStructure Add unsafe param to GetMarcSubfieldStructure and use this options where it's safe to do so to increase performance Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
The patch works as intended, but please add documentation for the new parameter.
Small follow-up required, please fix (approaching feature freeze fast now)!
Ok, I will probably be able to fix this today, or at least no later than tomorrow.
Great!
Created attachment 74089 [details] [review] Bug 19820: Add unsafe param to GetMarcSubfieldStructure Add unsafe param to GetMarcSubfieldStructure and use this options where it's safe to do so to increase performance Sponsored-by: Gothenburg University Library Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 75251 [details] [review] Bug 19820: Add unsafe param to GetMarcSubfieldStructure Add unsafe param to GetMarcSubfieldStructure and use this options where it's safe to do so to increase performance Sponsored-by: Gothenburg University Library Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 75252 [details] [review] Bug 19820: (QA follow-up) Fix pod in C4::Biblio Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Awesome work all! Pushed to master for 18.11.x
Let's stay on the safe side and do not backport this one.
Agreed, lets not backport this one :)