Bug 19279 - Performance of linked items in search
Summary: Performance of linked items in search
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin SOMERS
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-08 15:12 UTC by Fridolin SOMERS
Modified: 2020-04-13 17:12 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05


Attachments
Bug 19279 - performance of linked items in search (2.44 KB, patch)
2017-09-11 08:58 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 19279 - performance of linked items in search (2.48 KB, patch)
2018-01-31 11:53 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 19279 - performance of linked items in search (2.54 KB, patch)
2018-02-16 14:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19279: performance of linked items in search (2.29 KB, patch)
2018-07-24 07:31 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 19279: performance of linked items in search (2.34 KB, patch)
2019-09-17 10:15 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 19279: performance of linked items in search (2.67 KB, patch)
2019-11-15 16:05 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 19279: performance of linked items in search (2.73 KB, patch)
2020-03-09 10:02 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19279: performance of linked items in search (2.80 KB, patch)
2020-03-18 11:10 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2017-09-08 15:12:21 UTC
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().
Comment 1 Fridolin SOMERS 2017-09-11 08:51:20 UTC
I have experimented an execution time 10 times faster, whouuu
Comment 2 Fridolin SOMERS 2017-09-11 08:58:33 UTC
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
Comment 3 Hugo Agud 2018-01-31 11:53:18 UTC
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>
Comment 4 Kyle M Hall 2018-02-16 14:18:25 UTC
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>
Comment 5 Jonathan Druart 2018-02-16 15:36:39 UTC
"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?
Comment 6 paxed 2018-02-19 10:25:18 UTC
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)
Comment 7 Katrin Fischer 2018-02-19 11:06:12 UTC
This is related to the EasyAnalytics feature that uses non-standard/user defined subfields.
Comment 8 Fridolin SOMERS 2018-02-21 09:40:13 UTC
It appended on a Koha 16.11 on an article record linked to a serial record with 1158 host items.
Comment 9 Fridolin SOMERS 2018-07-24 07:31:37 UTC
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>
Comment 10 Fridolin SOMERS 2018-07-24 07:32:12 UTC
Rebased in current master
Comment 11 Fridolin SOMERS 2019-01-16 10:54:34 UTC
How can we continue here ?
Can someone benchmark on a sandbox ?
Comment 12 Fridolin SOMERS 2019-01-16 11:03:53 UTC
Oh, there is also nearly the same code in additem.pl for @hostitemnumbers
Comment 13 Martin Renvoize 2019-01-23 11:20:58 UTC
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.
Comment 14 Fridolin SOMERS 2019-09-17 10:15:33 UTC
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>
Comment 15 Victor Grousset/tuxayo 2019-09-17 16:12:13 UTC
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?
Comment 16 Katrin Fischer 2019-09-21 20:10:33 UTC
(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.
Comment 17 Fridolin SOMERS 2019-11-15 16:05:46 UTC
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>
Comment 18 Fridolin SOMERS 2019-11-15 16:13:42 UTC
(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.
Comment 19 Martin Renvoize 2020-03-09 10:02:54 UTC
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>
Comment 20 Martin Renvoize 2020-03-09 10:04:34 UTC
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.
Comment 21 Jonathan Druart 2020-03-18 11:10:47 UTC
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>
Comment 22 Jonathan Druart 2020-03-18 11:12:03 UTC
Missing tests and benchmark, but I spent some time reading and stressing the code and it makes perfect sense to me.
Comment 23 Martin Renvoize 2020-03-18 17:52:53 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 24 Joy Nelson 2020-04-03 20:25:13 UTC
backported to 19.11.x for 19.11.05
Comment 25 Lucas Gass 2020-04-13 17:12:33 UTC
Fridolin,

If this is nesseary in 19.05.x can you provide a rebase?