The routine iterates through all fields in a biblio and compares them to a list of acceptable tags that may link to an authority. It may be more efficient to do the opposite iteration, working through acceptable authority tags and looking for the existence of that field in the biblio. (Note.. I've not tested this hypothesis.. it depends heavily on how efficient it is to fetch a single MARC tag using Marc::Record)
(In reply to Martin Renvoize from comment #0) > The routine iterates through all fields in a biblio and compares them to a > list of acceptable tags that may link to an authority. It may be more > efficient to do the opposite iteration, working through acceptable authority > tags and looking for the existence of that field in the biblio. (Note.. I've > not tested this hypothesis.. it depends heavily on how efficient it is to > fetch a single MARC tag using Marc::Record) Hi Martin, TL;DR: In UNIMARC instances, link_bibs_to_authorities.pl run time can be reduced by 80% and the number of DBI calls can be reduced by 90% with a very simple fix that optimises the C4::Biblio::LinkBibHeadingsToAuthorities routine ;-) I had a go at optimising this routine recently, as I'm in the process of writing a Perl script that will perform some housekeeping tasks in our bibliographic records. [The script is meant to search for "TOPICAL SUBJECT" authority headings (UNIMARC field 250) using the text found in our existing "UNCONTROLLED SUBJECT TERMS" (biblio subfield 610$a) and, when a match is found, add a new "TOPICAL NAME USED AS SUBJECT" tag (field 606) to the bibliographic record, copy the authority record contents over, link it via subfield $9, then finally remove the (unwanted) uncontrolled subject term.] In developing this script, I investigated the inner workings of the routine in question, and I'm pleased to report that I've identified where the inefficiency in LinkBibHeadingsToAuthorities() actually lies. The patch for fixing this is trivial and backportable, too! ;-) > The routine iterates through all fields in a biblio and compares them to a > list of acceptable tags that may link to an authority. Indeed, that's exactly what happens in LinkBibHeadingsToAuthorities() with the crucial bit being line 641: 637 foreach my $field ( $bib->fields() ) { 638 if ( defined $tagtolink ) { 639 next unless $field->tag() == $tagtolink ; 640 } 641 my $heading = C4::Heading->new_from_field( $field, $frameworkcode ); 642 next unless defined $heading; [...] Then, in C4::Heading->new_from_field(): 61 sub new_from_field { 62 my $class = shift; 63 my $field = shift; 64 my $frameworkcode = shift; #FIXME this is not used? 65 my $auth = shift; 66 my $marcflavour = C4::Context->preference('marcflavour'); 67 my $marc_handler = _marc_format_handler($marcflavour); [...] 277 sub _marc_format_handler { 278 my $marcflavour = uc shift; 279 my $pname = "C4::Heading::$marcflavour"; 280 load $pname; 281 return $pname->new(); 282 } This is where things get interesting as the behaviour diverges a bit depending on the MARC flavour being used: in C4/Heading/MARC21.pm the object is constructed with: 255 sub new { 256 my $class = shift; 257 return bless {}, $class; 258 } ... and the $bib_heading_fields data structure/hash is statically set at the top of the module: 49 my $bib_heading_fields = { 50 '100' => { 51 auth_type => 'PERSO_NAME', 52 subfields => 'abcdfghjklmnopqrst', 53 main_entry => 1 54 }, 55 '110' => { 56 auth_type => 'CORPO_NAME', 57 subfields => 'abcdfghklmnoprst', 58 main_entry => 1 59 }, [...] Whilst in C4/Heading/UNIMARC.pm the object is constructed with: 68 sub new { 69 my $class = shift; 70 71 my $dbh = C4::Context->dbh; 72 my $sth = $dbh->prepare( 73 "SELECT tagfield, authtypecode 74 FROM marc_subfield_structure 75 WHERE frameworkcode = '' AND authtypecode <> ''" 76 ); 77 $sth->execute(); 78 $bib_heading_fields = {}; 79 while ( my ( $tag, $auth_type ) = $sth->fetchrow ) { 80 $bib_heading_fields->{$tag} = { 81 auth_type => $auth_type, 82 subfields => 'abcdefghjklmnopqrstvxyz', 83 }; 84 } 85 86 return bless {}, $class; 87 } ... thus resetting the $bib_heading_fields hash *in each invocation*, then populating it again with the results fetched from the database! Does this information really need to be re-calculated for each field of the record being saved/updated/linked? I think not, as: 1) Changes to the marc_subfield_structure table are not that frequent, and are unlikely to occur in the relatively short timeframe that a save/update/linking takes to complete. 2) As per the official Koha manual, you're not really meant to edit the Default framework as this will cause problems, but rather clone it to a different one which you'll use to catalogue your records. [C4::Heading->new_from_field() can actually be fed with a $frameworkcode parameter, but it doesn't currently use it. I'll leave that for a different bug report, though ;-)]
bulkmarcimport.pl seems to be the only script that is affected the most by this, as it can potentially make an excessive number of calls to LinkBibHeadingsToAuthorities(). Also, if the AutoLinkBiblios system preference is set to 'Do', then AddBiblio() and ModBiblio() both call BiblioAutoLink() which in turn calls LinkBibHeadingsToAuthorities() to do the heavy-lifting. So, I believe my patch might bring an improvement in the overall responsiveness of the Staff interface when saving new bibliographic records or modifying existing ones, especially for records with many many fields. However, the big win here is the significantly lower execution times for any helper scripts that call these functions. These benefits apply to UNIMARC only ;-) To put the above claim to the test, I profiled the DBI calls made while using link_bibs_to_authorities.pl and also timed how long it took to run the script. The exact command line used is shown below. My database had 8,241 authority and 23,142 biblio records. +---------------+------------+------------+ DBI calls made | without patch | with patch | difference | +---------------+------------+------------+ | 14,732,858 | 626,090 | -95.75% | +---------------+------------+------------+ +---------------+------------+------------+ execution time | without patch | with patch | difference | +---------------+------------+------------+ | 1,125 sec | 186 sec | -83.47% | +---------------+------------+------------+ Command line used: $ DBI_PROFILE=1 ./misc/link_bibs_to_authorities.pl -t I include the "raw" profiling data from my test runs below: Bib authority heading linking report ======================================================= Linker module: C4::Linker::Default Run started at: 08/01/23 20:10:39 Run ended at: 08/01/23 20:29:24 Total run time: 1124977 ms Number of bibs checked: 23142 Number of bibs modified: 0 Number of bibs with errors: 0 Number of headings linked: 32316 Number of headings unlinked: 15793 Number of headings fuzzily linked: 0 **** Ran in test mode only **** DBI::Profile: 880.455353s 78.05% (14732858 calls) link_bibs_to_authorities.pl @ 2023-08-01 20:29:24 Bib authority heading linking report ======================================================= Linker module: C4::Linker::Default Run started at: 08/01/23 20:40:06 Run ended at: 08/01/23 20:43:12 Total run time: 185965 ms Number of bibs checked: 23142 Number of bibs modified: 0 Number of bibs with errors: 0 Number of headings linked: 32316 Number of headings unlinked: 15793 Number of headings fuzzily linked: 0 **** Ran in test mode only **** DBI::Profile: 29.444538s 15.58% (626090 calls) link_bibs_to_authorities.pl @ 2023-08-01 20:43:12
Created attachment 154131 [details] [review] Bug 21828: build $bib_heading_fields only once per invocation In UNIMARC instances, the run time of link_bibs_to_authorities.pl can be reduced by up to 80% and the number of DBI calls can be reduced by up to 90% with a very simple fix that optimises the constructor of the C4::Heading::UNIMARC object. Currently, the constructor resets the $bib_heading_fields hash *in each invocation* (i.e. for every field the bibliographic record contains), then populating it again with the results fetched from the database! This is inefficient. The patch/fix is trivial: we take advantage of the fact that $bib_heading_fields is declared at the top of the C4::Heading::UNIMARC module and is thus a package variable that is in scope for the entire execution of the program (more info here: https://stackoverflow.com/q/75317862). Placing the section that generates the $bib_heading_fields hash inside a "unless ( defined $bib_heading_fields )" code block is enough to cause a significant reduction in the number of "expensive" SQL SELECT queries that must be run. Test plan: 0) Have a UNIMARC instance with some sample data (the KTD one will do just fine for this experiment). 1) Run the following commands: $ ktd --shell k$ DBI_PROFILE=1 ./misc/link_bibs_to_authorities.pl -t Observe the output from the script and the DBI profiling info. [You may want to play with different DBI_PROFILE levels (such as 2, 4, 6, 8, etc.) to see what's going on under the hood DBI-wise, for reference see: https://metacpan.org/pod/DBI::Profile] 2) Apply this patch. 3) Rerun the script from step 1), it should run a lot faster!
(In reply to Andreas Roussos from comment #2) > bulkmarcimport.pl seems to be the only script that is affected > the most by this, as it can potentially make an excessive number > of calls to LinkBibHeadingsToAuthorities(). Wrong script name -- I meant to write "link_bibs_to_authorities.pl".
Created attachment 155176 [details] [review] Bug 21828: build $bib_heading_fields only once per invocation In UNIMARC instances, the run time of link_bibs_to_authorities.pl can be reduced by up to 80% and the number of DBI calls can be reduced by up to 90% with a very simple fix that optimises the constructor of the C4::Heading::UNIMARC object. Currently, the constructor resets the $bib_heading_fields hash *in each invocation* (i.e. for every field the bibliographic record contains), then populating it again with the results fetched from the database! This is inefficient. The patch/fix is trivial: we take advantage of the fact that $bib_heading_fields is declared at the top of the C4::Heading::UNIMARC module and is thus a package variable that is in scope for the entire execution of the program (more info here: https://stackoverflow.com/q/75317862). Placing the section that generates the $bib_heading_fields hash inside a "unless ( defined $bib_heading_fields )" code block is enough to cause a significant reduction in the number of "expensive" SQL SELECT queries that must be run. Test plan: 0) Have a UNIMARC instance with some sample data (the KTD one will do just fine for this experiment). 1) Run the following commands: $ ktd --shell k$ DBI_PROFILE=1 ./misc/link_bibs_to_authorities.pl -t Observe the output from the script and the DBI profiling info. [You may want to play with different DBI_PROFILE levels (such as 2, 4, 6, 8, etc.) to see what's going on under the hood DBI-wise, for reference see: https://metacpan.org/pod/DBI::Profile] 2) Apply this patch. 3) Rerun the script from step 1), it should run a lot faster! Signed-off-by: David Nind <david@davidnind.com>
Test run results using KTD (UNIMARC instance with sample data). Before the patch was applied ---------------------------- DBI_PROFILE=1 ./misc/link_bibs_to_authorities.pl -t Bib authority heading linking report ======================================================= Linker module: C4::Linker::Default Run started at: 09/02/23 20:34:46 Run ended at: 09/02/23 20:35:47 Total run time: 61565 ms Number of bibs checked: 4849 Number of bibs modified: 0 Number of bibs with errors: 0 Number of headings linked: 10756 Number of headings unlinked: 555 Number of headings fuzzily linked: 0 **** Ran in test mode only **** DBI::Profile: 46.609051s 75.18% (3111830 calls) link_bibs_to_authorities.pl @ 2023-09-02 20:35:47 After the patch was applied --------------------------- DBI_PROFILE=1 ./misc/link_bibs_to_authorities.pl -t Bib authority heading linking report ======================================================= Linker module: C4::Linker::Default Run started at: 09/02/23 20:37:05 Run ended at: 09/02/23 20:37:14 Total run time: 9231 ms Number of bibs checked: 4849 Number of bibs modified: 0 Number of bibs with errors: 0 Number of headings linked: 10756 Number of headings unlinked: 555 Number of headings fuzzily linked: 0 **** Ran in test mode only **** DBI::Profile: 2.781512s 27.82% (105365 calls) link_bibs_to_authorities.pl @ 2023-09-02 20:37:14
Ok so this seem to implement the UNIMARC part of the initial request of the ticket. C4::Heading::MARC21->new() does nothing compared to C4::Heading::UNIMARC->new() I guess the equivalent code might be somewhere else, weird. Though what Andreas found doesn't seem to be the same as what Martin analyzed at the beginning. So maybe it two different optimizations that can be stacked. So anyway, this could keep moving as UNIMARC only and in the end another ticket can be made for if the same could be done for MARC21. And/or keeping the initial proposal of Martin. (not sure if I understood well what we have here and the initial idea) --- The relevant tests seem to be *at least*: prove t/db_dependent/Biblio.t t/db_dependent/Heading.t They fail when when I sabotage C4::Heading::UNIMARC->new() But I can't tell in the detail if they test enough regarding the changes of this patch. No idea about eventual side effects of skipping stuff when $bib_heading_fields if defined. It's likely 100% redundant but I don't have the insight to be sure.
Nice work Andreas.. I agree this patch looks great though isn't really testing what I described.. However, I think it's a well worthwhile improvement.. I'll QA your patch and migrate the original idea to a new bug.
Created attachment 155792 [details] [review] Bug 21828: build $bib_heading_fields only once per invocation In UNIMARC instances, the run time of link_bibs_to_authorities.pl can be reduced by up to 80% and the number of DBI calls can be reduced by up to 90% with a very simple fix that optimises the constructor of the C4::Heading::UNIMARC object. Currently, the constructor resets the $bib_heading_fields hash *in each invocation* (i.e. for every field the bibliographic record contains), then populating it again with the results fetched from the database! This is inefficient. The patch/fix is trivial: we take advantage of the fact that $bib_heading_fields is declared at the top of the C4::Heading::UNIMARC module and is thus a package variable that is in scope for the entire execution of the program (more info here: https://stackoverflow.com/q/75317862). Placing the section that generates the $bib_heading_fields hash inside a "unless ( defined $bib_heading_fields )" code block is enough to cause a significant reduction in the number of "expensive" SQL SELECT queries that must be run. Test plan: 0) Have a UNIMARC instance with some sample data (the KTD one will do just fine for this experiment). 1) Run the following commands: $ ktd --shell k$ DBI_PROFILE=1 ./misc/link_bibs_to_authorities.pl -t Observe the output from the script and the DBI profiling info. [You may want to play with different DBI_PROFILE levels (such as 2, 4, 6, 8, etc.) to see what's going on under the hood DBI-wise, for reference see: https://metacpan.org/pod/DBI::Profile] 2) Apply this patch. 3) Rerun the script from step 1), it should run a lot faster! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to master for 23.11. Nice work everyone, thanks!
Whaouuu :D This is also used in Koha/SearchEngine/Elasticsearch.pm: my $heading = C4::Heading->new_from_field( $field, undef, 1 ); #new auth heading So it may speedup authorities indexing :D
Pushed to 23.05.x for 23.05.04
Enhancement - not pushing to oldstable for 22.11.x