Bug 21828 - Improve efficiency of C4::Biblio::LinkBibHeadingsToAuthorities
Summary: Improve efficiency of C4::Biblio::LinkBibHeadingsToAuthorities
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Andreas Roussos
QA Contact: Martin Renvoize
URL: misc/link_bibs_to_authorities.pl
Keywords:
Depends on:
Blocks: 7923 34815
  Show dependency treegraph
 
Reported: 2018-11-13 17:16 UTC by Martin Renvoize
Modified: 2023-10-17 13:51 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04


Attachments
Bug 21828: build $bib_heading_fields only once per invocation (3.16 KB, patch)
2023-08-01 19:25 UTC, Andreas Roussos
Details | Diff | Splinter Review
Bug 21828: build $bib_heading_fields only once per invocation (3.20 KB, patch)
2023-09-02 20:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 21828: build $bib_heading_fields only once per invocation (3.27 KB, patch)
2023-09-18 10:33 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2018-11-13 17:16:01 UTC
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)
Comment 1 Andreas Roussos 2023-08-01 19:20:44 UTC
(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 ;-)]
Comment 2 Andreas Roussos 2023-08-01 19:23:37 UTC
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
Comment 3 Andreas Roussos 2023-08-01 19:25:11 UTC
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!
Comment 4 Andreas Roussos 2023-08-04 07:15:57 UTC
(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".
Comment 5 David Nind 2023-09-02 20:37:57 UTC
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>
Comment 6 David Nind 2023-09-02 20:42:35 UTC
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
Comment 7 Victor Grousset/tuxayo 2023-09-05 23:15:01 UTC
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.
Comment 8 Martin Renvoize 2023-09-18 10:32:50 UTC
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.
Comment 9 Martin Renvoize 2023-09-18 10:33:59 UTC
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>
Comment 10 Tomás Cohen Arazi 2023-09-22 15:58:08 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 11 Fridolin Somers 2023-09-22 20:47:09 UTC
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
Comment 12 Fridolin Somers 2023-09-22 20:47:39 UTC
Pushed to 23.05.x for 23.05.04
Comment 13 Jacob O'Mara 2023-10-17 13:51:30 UTC
Enhancement - not pushing to oldstable for 22.11.x