When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Look like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem().
I have experimented an execution time 10 times faster, whouuu
Created attachment 67036 [details] [review] Bug 19279 - performance of linked items in search When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Look like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem(). Test plan : - Get a record A with a lot of items - Create a new record B - Create a linked items from B to A with a field 773 (461 in UNIMARC) : biblionumber in $0 and itemnumber in $9 - Display B record details => Compare execution times with and without patch
Created attachment 71083 [details] [review] Bug 19279 - performance of linked items in search When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Look like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem(). Test plan : - Get a record A with a lot of items - Create a new record B - Create a linked items from B to A with a field 773 (461 in UNIMARC) : biblionumber in $0 and itemnumber in $9 - Display B record details => Compare execution times with and without patch Signed-off-by: Hugo Agud <hagud@orex.es>
Created attachment 71777 [details] [review] Bug 19279 - performance of linked items in search When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Look like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem(). Test plan : - Get a record A with a lot of items - Create a new record B - Create a linked items from B to A with a field 773 (461 in UNIMARC) : biblionumber in $0 and itemnumber in $9 - Display B record details => Compare execution times with and without patch Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
"Look like the code could be more performant", where are the benchmarks then? The code is not trivial, how are you sure you do not remove a hidden feature/introduce a regression?
This seems to be using 773$0 and 773$9... but I don't see either of those subfields defined in the MARC21 docs? (See: http://www.loc.gov/marc/bibliographic/bd773.html)
This is related to the EasyAnalytics feature that uses non-standard/user defined subfields.
It appended on a Koha 16.11 on an article record linked to a serial record with 1158 host items.
Created attachment 77204 [details] [review] Bug 19279: performance of linked items in search When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Look like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem(). Test plan : - Get a record A with a lot of items - Create a new record B - Create a linked items from B to A with a field 773 (461 in UNIMARC) : biblionumber in $0 and itemnumber in $9 - Display B record details => Compare execution times with and without patch Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Rebased in current master
How can we continue here ? Can someone benchmark on a sandbox ?
Oh, there is also nearly the same code in additem.pl for @hostitemnumbers
I get the feeling having some tests added here would greatly appease any worries of possible introducing regressions. It would also be nice to have a definitive performance comparison, but actually to me that less important than having tests for regressions as the code looks cleaner this way to me and doesn't give a noticeable degradation in performance in testing.
Created attachment 92869 [details] [review] Bug 19279: performance of linked items in search When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Look like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem(). Test plan : - Get a record A with a lot of items - Create a new record B - Create a linked items from B to A with a field 773 (461 in UNIMARC) : biblionumber in $0 and itemnumber in $9 - Display B record details => Compare execution times with and without patch Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Hi, I think it's a good idea to *always* instigate to add tests when fixing existing code. In this case the changes seems too complex to make this testable without messing with too much stuff. So in cases (like this) where the fix gets stuck for a long time due to requiring tests, if we are confident about the fix, can we allow it as it is?
(In reply to Victor Grousset/tuxayo from comment #15) > Hi, I think it's a good idea to *always* instigate to add tests when fixing > existing code. > In this case the changes seems too complex to make this testable without > messing with too much stuff. So in cases (like this) where the fix gets > stuck for a long time due to requiring tests, if we are confident about the > fix, can we allow it as it is? I think at the moment we can't be sure if this works correctly. I am also a bit doubtful about the test plan - it doesn't even mention the EasyAnalytics feature this seems to be used for? I'd at least like a real good test plan here.
Created attachment 95455 [details] [review] Bug 19279: performance of linked items in search When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Looks like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem(). Test plan : 1) Enable system preference EasyAnalyticalRecords 2) Get a record A with a lot of items, it will be the 'Host record' 3) Click on 'Analytics' on this record 4) On one of its items (say item A1) click on 'Create analytics' to create record B 5) Enter the same title as record A and all mandatory fields 6) Edit field 773 (461 in UNIMARC) 6) Check that $0 contains the biblionumber of record A 7) Check that $9 contains the itemnumber of item A1 8) Perform a search on record A title, be sure you see record B with other results => Compare execution times with and without patch Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Katrin Fischer from comment #16) > > I think at the moment we can't be sure if this works correctly. I am also a > bit doubtful about the test plan - it doesn't even mention the EasyAnalytics > feature this seems to be used for? > > I'd at least like a real good test plan here. Indeed. Here is a much better test plan.
Created attachment 100330 [details] [review] Bug 19279: performance of linked items in search When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Looks like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem(). Test plan : 1) Enable system preference EasyAnalyticalRecords 2) Get a record A with a lot of items, it will be the 'Host record' 3) Click on 'Analytics' on this record 4) On one of its items (say item A1) click on 'Create analytics' to create record B 5) Enter the same title as record A and all mandatory fields 6) Edit field 773 (461 in UNIMARC) 6) Check that $0 contains the biblionumber of record A 7) Check that $9 contains the itemnumber of item A1 8) Perform a search on record A title, be sure you see record B with other results => Compare execution times with and without patch Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Digging deeper into the code here I'm confident in this not creating regressions.. My fear was that it may produce regressions for non EasyAnalytics systems, but as the changed code in question is inside a block that is only triggered when EasyAnalytics is enabled we are safe on that front. Otherwise, the code is pretty clear and makes sense.. following the plan it all works as expected. Signing off.
Created attachment 100952 [details] [review] Bug 19279: performance of linked items in search When catalog has fields 773 (461 in UNIMARC), those linked items are fetched for display in search results. Looks like the code could be more performant by replacing item search by $9 with direct call with GetMarcItem(). Test plan : 1) Enable system preference EasyAnalyticalRecords 2) Get a record A with a lot of items, it will be the 'Host record' 3) Click on 'Analytics' on this record 4) On one of its items (say item A1) click on 'Create analytics' to create record B 5) Enter the same title as record A and all mandatory fields 6) Edit field 773 (461 in UNIMARC) 6) Check that $0 contains the biblionumber of record A 7) Check that $9 contains the itemnumber of item A1 8) Perform a search on record A title, be sure you see record B with other results => Compare execution times with and without patch Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Missing tests and benchmark, but I spent some time reading and stressing the code and it makes perfect sense to me.
Nice work everyone! Pushed to master for 20.05
backported to 19.11.x for 19.11.05
Fridolin, If this is nesseary in 19.05.x can you provide a rebase?