https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=26641 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 --- Comment #1 from Andreas Roussos <a.roussos@dataly.gr> --- (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 ;-)] -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 Andreas Roussos <a.roussos@dataly.gr> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal Assignee|koha-bugs@lists.koha-commun |a.roussos@dataly.gr |ity.org | Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 --- Comment #3 from Andreas Roussos <a.roussos@dataly.gr> --- Created attachment 154131 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154131&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 --- Comment #4 from Andreas Roussos <a.roussos@dataly.gr> --- (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".

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154131|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 155176 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155176&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net --- Comment #7 from Victor Grousset/tuxayo <victor@tuxayo.net> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155176|0 |1 is obsolete| | --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 155792 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155792&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |34815 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34815 [Bug 34815] Improve efficiency of C4::Biblio::LinkBibHeadingsToAuthorities -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21828 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00 |23.11.00,23.05.04 released in| | Status|Pushed to master |Pushed to stable --- Comment #12 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.04 -- You are receiving this mail because: You are watching all bug changes.