Bug 30789

Summary: Improve performance of AddBiblio when importing records with many items
Product: Koha Reporter: Nick Clemens <nick>
Component: MARC Bibliographic record staging/importAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: dcook, jonathan.druart, lucas, m.de.rooy, martin.renvoize
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.03
Bug Depends on:    
Bug Blocks: 30813    
Attachments: Bug 30789: Improve performance of AddBiblio
Bug 30789: Improve performance of AddBiblio
Bug 30789: Improve performance of AddBiblio

Description Nick Clemens 2022-05-17 12:29:38 UTC

    
Comment 1 Nick Clemens 2022-05-17 12:41:25 UTC
Created attachment 135071 [details] [review]
Bug 30789: Improve performance of AddBiblio

AddBiblio calls TransformMarcRecordToKoha. We are only interested in getting
biblio and biblioitems info, so we can pass 'no_items' to save some field lookups

Additionally we can pass the subfield structure we have already fetched into the subroutine
TransformMarcToKohaOneField

Testing with NYTProf I saw a reduction of ~6 seconds when importing ~400 bibs and ~1000 items
i.e. I exported and imported the sample data (after removing duplicate 010 from record 72)

To test:
1 - Stage and import some records
2 - Confirm it works the same before and after patch
Comment 2 Jonathan Druart 2022-05-18 08:14:27 UTC
I wouldn't pass the mss, we are already retrieving it from cache using unsafe.
Comment 3 Nick Clemens 2022-05-19 19:25:41 UTC
Created attachment 135216 [details] [review]
Bug 30789: Improve performance of AddBiblio

AddBiblio calls TransformMarcRecordToKoha. We are only interested in getting biblio and biblioitems info, so we can pass 'no_items' to save some field lookups

Benchmarking saw a ~75% increase in the performance of this call

To test:
1 - Stage and import some records
2 - Confirm it works the same before and after patch
3 - Add a biblio via cataloguing, confirm it works
Comment 4 David Cook 2022-05-20 02:02:16 UTC
Adding this one to my to test list as I'd love this...
Comment 5 Martin Renvoize 2022-05-20 10:56:29 UTC
Created attachment 135238 [details] [review]
Bug 30789: Improve performance of AddBiblio

AddBiblio calls TransformMarcRecordToKoha. We are only interested in getting biblio and biblioitems info, so we can pass 'no_items' to save some field lookups

Benchmarking saw a ~75% increase in the performance of this call

To test:
1 - Stage and import some records
2 - Confirm it works the same before and after patch
3 - Add a biblio via cataloguing, confirm it works

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-05-20 10:59:53 UTC
Trivial and clear code improvement.. we don't handle item table rows as part of AddBiblio so there's really no reason to process them.

Unit tests continue to all pass as expected, no regressions found and the QA script is happy.  I can confirm the performance improvement too alongside the cleaner feeling code.

Think I'm going to go straight to QA for this one.

Passed.
Comment 7 Marcel de Rooy 2022-06-03 09:29:46 UTC
Taking a short look here
Comment 8 Marcel de Rooy 2022-06-03 09:43:42 UTC
I agree with this patch, no problem about that.
But the performance gain should be close to zero.

Running Biblio.t (calling AddBIblio) twice with and without patch:

WITH PATCH
real    0m3.228s
user    0m2.505s
sys     0m0.371s
real    0m2.786s
user    0m2.176s
sys     0m0.370s

WITHOUT PATCH
real    0m3.162s
user    0m2.456s
sys     0m0.374s
real    0m2.728s
user    0m2.104s
sys     0m0.403s

The funny thing here is that it was faster without the patch. Which does not tell me anything. Just incidental.
If we cached the marc structure, the other calls is pure memory stuff on a few fields. This really costs nothing.

The title of this report promises way too much. No gain expected.

RM: I would suggest to revise the title unless we have clear feedback to prove the statement. Keeping the release notes a bit more realistic?
Comment 9 Nick Clemens 2022-06-03 10:36:14 UTC
(In reply to Marcel de Rooy from comment #8)
> I agree with this patch, no problem about that.
> But the performance gain should be close to zero.
> Running Biblio.t (calling AddBIblio) twice with and without patch:
> RM: I would suggest to revise the title unless we have clear feedback to
> prove the statement. Keeping the release notes a bit more realistic?

The gain is only going to come when you have records that contain items. 

Testing with NYTProf against an import with record containing items, there was a clear improvement

Biblio.t does not add items to most records, so the savings will be minimal if present
Comment 10 Marcel de Rooy 2022-06-07 11:42:15 UTC
(In reply to Nick Clemens from comment #9)
> (In reply to Marcel de Rooy from comment #8)
> > I agree with this patch, no problem about that.
> > But the performance gain should be close to zero.
> > Running Biblio.t (calling AddBIblio) twice with and without patch:
> > RM: I would suggest to revise the title unless we have clear feedback to
> > prove the statement. Keeping the release notes a bit more realistic?
> 
> The gain is only going to come when you have records that contain items. 
> 
> Testing with NYTProf against an import with record containing items, there
> was a clear improvement
> 
> Biblio.t does not add items to most records, so the savings will be minimal
> if present

When you look at the AddBiblio code, you will be surprised to see that this routine does not add items at all.
During import you need something as BatchCommitItems to add items.

So the title of this report still needs attention :)
Comment 11 Nick Clemens 2022-06-07 11:52:48 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Nick Clemens from comment #9)
> > (In reply to Marcel de Rooy from comment #8)
> > > I agree with this patch, no problem about that.
> > > But the performance gain should be close to zero.
> > > Running Biblio.t (calling AddBIblio) twice with and without patch:
> > > RM: I would suggest to revise the title unless we have clear feedback to
> > > prove the statement. Keeping the release notes a bit more realistic?
> > 
> > The gain is only going to come when you have records that contain items. 
> > 
> > Testing with NYTProf against an import with record containing items, there
> > was a clear improvement
> > 
> > Biblio.t does not add items to most records, so the savings will be minimal
> > if present
> 
> When you look at the AddBiblio code, you will be surprised to see that this
> routine does not add items at all.
> During import you need something as BatchCommitItems to add items.
> 
> So the title of this report still needs attention :)

That's exactly the issue - we aren't adding items, but we are processing item fields - what would you suggest for the bug title?
Comment 12 Marcel de Rooy 2022-06-07 12:06:57 UTC
(In reply to Nick Clemens from comment #11)
> 
> That's exactly the issue - we aren't adding items, but we are processing
> item fields - what would you suggest for the bug title?

Well, lets finish this here. What about: Improve performance of AddBiblio when importing records with many items ?
Comment 13 Tomás Cohen Arazi 2022-06-08 14:50:48 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 14 Lucas Gass 2022-07-12 16:58:04 UTC
Enhancement will not be backported to 22.05.x
Comment 15 Nick Clemens 2022-07-12 17:25:38 UTC
Performance bugs should be considered bug, not enhancement, remarking.

Please consider for backport
Comment 16 Lucas Gass 2022-07-12 17:36:40 UTC
Backported to 22.05.x for 22.05.03