Bug 19820 - Add unsafe param to GetMarcSubfieldStructure
Summary: Add unsafe param to GetMarcSubfieldStructure
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks: 7923
  Show dependency treegraph
 
Reported: 2017-12-15 13:18 UTC by David Gustafsson
Modified: 2018-06-27 09:33 UTC (History)
7 users (show)

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


Attachments
Bug 19820: Add unsafe param to GetMarcSubfieldStructure (2.89 KB, patch)
2017-12-15 13:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19820: Add unsafe param to GetMarcSubfieldStructure (2.96 KB, patch)
2018-02-13 20:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19820: Add unsafe param to GetMarcSubfieldStructure (3.98 KB, patch)
2018-04-12 12:55 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19820: Add unsafe param to GetMarcSubfieldStructure (4.03 KB, patch)
2018-05-11 06:39 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19820: (QA follow-up) Fix pod in C4::Biblio (587 bytes, patch)
2018-05-11 06:39 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2017-12-15 13:18:06 UTC
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.
Comment 1 David Gustafsson 2017-12-15 13:24:20 UTC
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
Comment 2 Josef Moravec 2017-12-17 11:36:20 UTC
Is this ready to test?

What is a test plan?
Comment 3 David Gustafsson 2017-12-17 17:33:57 UTC
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.
Comment 4 David Gustafsson 2017-12-18 15:25:52 UTC
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).
Comment 5 David Gustafsson 2017-12-18 15:58:20 UTC
My vm-disk was too small for the dump, so will not be able to run this today.
Comment 6 David Gustafsson 2017-12-19 16:06:08 UTC
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
Comment 7 Jonathan Druart 2018-02-13 20:08:15 UTC
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>
Comment 8 Julian Maurice 2018-04-10 06:11:09 UTC
The patch works as intended, but please add documentation for the new parameter.
Comment 9 Katrin Fischer 2018-04-12 10:50:26 UTC
Small follow-up required, please fix (approaching feature freeze fast now)!
Comment 10 David Gustafsson 2018-04-12 12:05:02 UTC
Ok, I will probably be able to fix this today, or at least no later than tomorrow.
Comment 11 Katrin Fischer 2018-04-12 12:07:45 UTC
Great!
Comment 12 David Gustafsson 2018-04-12 12:55:20 UTC
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>
Comment 13 Josef Moravec 2018-05-11 06:39:21 UTC
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>
Comment 14 Josef Moravec 2018-05-11 06:39:26 UTC
Created attachment 75252 [details] [review]
Bug 19820: (QA follow-up) Fix pod in C4::Biblio

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Nick Clemens 2018-06-22 18:11:33 UTC
Awesome work all!

Pushed to master for 18.11.x
Comment 16 Jonathan Druart 2018-06-22 18:23:25 UTC
Let's stay on the safe side and do not backport this one.
Comment 17 Martin Renvoize 2018-06-27 09:33:10 UTC
Agreed, lets not backport this one :)