Summary: | Improve performance of AddBiblio when importing records with many items | ||
---|---|---|---|
Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
Component: | MARC Bibliographic record staging/import | Assignee: | Nick Clemens (kidclamp) <nick> |
Status: | CLOSED FIXED | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | dcook, jonathan.druart, lucas, m.de.rooy, martin.renvoize |
Version: | Main | ||
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
|
|
Circulation function: | |||
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 (kidclamp)
2022-05-17 12:29:38 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 I wouldn't pass the mss, we are already retrieving it from cache using unsafe. 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 Adding this one to my to test list as I'd love this... 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> 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. Taking a short look here 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? (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 (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 :) (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? (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 ? Pushed to master for 22.11. Nice work everyone, thanks! Enhancement will not be backported to 22.05.x Performance bugs should be considered bug, not enhancement, remarking. Please consider for backport Backported to 22.05.x for 22.05.03 |