From 43aad885fd703337ffc7e7e7258301fa69d67343 Mon Sep 17 00:00:00 2001 From: Jacek Ablewicz Date: Wed, 27 Apr 2016 13:19:10 +0200 Subject: [PATCH] Bug 16365: Selectivelly introduce GetMarcStructure() "unsafe" variant for better performance GetMarcStructure() currently uses Koha::Cache in the "safe" mode (returning deep copy of the result data structure by default), which causes numerous performance issues in many Koha scripts. Switching it to the "unsafe" mode globally (2nd patch from Bug 16140) resolves those issues, but ensuring that it is regression-free (and that it will stay that way in the future) is far from easy. This patch proposes a bit more manageable solution, it introduces a possibility to use "unsafe" variant selectivelly (only in those places in the code where GetMarcStructure() is called repetively). That way, amount of the code that needs to be audited for possible problems gets vastly reduced, without any performance trade-offs. Test plan: 1) Have a look at the code and audit the parts affected by this patch for possible regressions https://bugs.koha-community.org/show_bug.cgi?id=16365 --- C4/Biblio.pm | 21 ++++++++++++++------- C4/Items.pm | 6 +++++- C4/XSLT.pm | 2 +- Koha/Filter/MARC/ViewPolicy.pm | 2 +- 4 files changed, 21 insertions(+), 10 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index ae1f2f3..168b2f0 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -932,7 +932,7 @@ sub GetISBDView { return unless defined $record; my $itemtype = &GetFrameworkCode($biblionumber); my ( $holdingbrtagf, $holdingbrtagsubf ) = &GetMarcFromKohaField( "items.holdingbranch", $itemtype ); - my $tagslib = &GetMarcStructure( 1, $itemtype ); + my $tagslib = &GetMarcStructure( 1, $itemtype, { unsafe => 1 } ); my $ISBD = C4::Context->preference($sysprefname); my $bloc = $ISBD; @@ -1109,22 +1109,28 @@ sub IsMarcStructureInternal { =head2 GetMarcStructure - $res = GetMarcStructure($forlibrarian,$frameworkcode); + $res = GetMarcStructure($forlibrarian, $frameworkcode, [ $params ]); Returns a reference to a big hash of hash, with the Marc structure for the given frameworkcode $forlibrarian :if set to 1, the MARC descriptions are the librarians ones, otherwise it's the public (OPAC) ones $frameworkcode : the framework code to read +TODO: add 'unsafe' parameter description and a warnings: what kinds of operations +on the directly returned data structure are allowed / forbidden +(autovivification of $tagslib->{$tag}->{$subfield} etc. are OK, +but adding/removing/modifying any scalars in the "leafs" are strictly forbidden) + =cut sub GetMarcStructure { - my ( $forlibrarian, $frameworkcode ) = @_; + my ( $forlibrarian, $frameworkcode, $params ) = @_; $frameworkcode = "" unless $frameworkcode; $forlibrarian = $forlibrarian ? 1 : 0; + my $unsafe = ($params && $params->{unsafe})? 1: 0; my $cache = Koha::Cache->get_instance(); my $cache_key = "MarcStructure-$forlibrarian-$frameworkcode"; - my $cached = $cache->get_from_cache($cache_key); + my $cached = $cache->get_from_cache($cache_key, { unsafe => $unsafe }); return $cached if $cached; my $dbh = C4::Context->dbh; @@ -1937,10 +1943,11 @@ sub GetMarcAuthors { } my ( $mintag, $maxtag, $fields_filter ); - # tagslib useful for UNIMARC author reponsabilities - my $tagslib = - &GetMarcStructure( 1, '' ); # FIXME : we don't have the framework available, we take the default framework. May be buggy on some setups, will be usually correct. + # tagslib useful only for UNIMARC author responsibilities + my $tagslib; if ( $marcflavour eq "UNIMARC" ) { + # FIXME : we don't have the framework available, we take the default framework. May be buggy on some setups, will be usually correct. + $tagslib = GetMarcStructure( 1, '', { unsafe => 1 }); $mintag = "700"; $maxtag = "712"; $fields_filter = '7..'; diff --git a/C4/Items.pm b/C4/Items.pm index 651e590..0d2aed2 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -2431,7 +2431,7 @@ sub _get_unlinked_item_subfields { my $original_item_marc = shift; my $frameworkcode = shift; - my $marcstructure = GetMarcStructure(1, $frameworkcode); + my $marcstructure = GetMarcStructure(1, $frameworkcode, { unsafe => 1 }); # assume that this record has only one field, and that that # field contains only the item information @@ -2815,6 +2815,10 @@ sub PrepareItemrecordDisplay { my $dbh = C4::Context->dbh; $frameworkcode = &GetFrameworkCode($bibnum) if $bibnum; my ( $itemtagfield, $itemtagsubfield ) = &GetMarcFromKohaField( "items.itemnumber", $frameworkcode ); + + # it would be perhaps beneficial (?) to call GetMarcStructure with 'unsafe' parameter + # for performance reasons, but $tagslib may be passed to $plugin->build(), and there + # is no way to ensure that this structure is not getting corrupted somewhere in there my $tagslib = &GetMarcStructure( 1, $frameworkcode ); # return nothing if we don't have found an existing framework. diff --git a/C4/XSLT.pm b/C4/XSLT.pm index a2bbaab..c32fc81 100644 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -68,7 +68,7 @@ Is only used in this module currently. sub transformMARCXML4XSLT { my ($biblionumber, $record) = @_; my $frameworkcode = GetFrameworkCode($biblionumber) || ''; - my $tagslib = &GetMarcStructure(1,$frameworkcode); + my $tagslib = &GetMarcStructure(1, $frameworkcode, { unsafe => 1 }); my @fields; # FIXME: wish there was a better way to handle exceptions eval { diff --git a/Koha/Filter/MARC/ViewPolicy.pm b/Koha/Filter/MARC/ViewPolicy.pm index 98df72d..cbf089e 100644 --- a/Koha/Filter/MARC/ViewPolicy.pm +++ b/Koha/Filter/MARC/ViewPolicy.pm @@ -84,7 +84,7 @@ sub filter { my $frameworkcode = $self->{options}->{frameworkcode} // q{}; my $hide = _should_hide_on_interface(); - my $marcsubfieldstructure = GetMarcStructure( 0, $frameworkcode ); + my $marcsubfieldstructure = GetMarcStructure( 0, $frameworkcode, { unsafe => 1 } ); #if ($marcsubfieldstructure->{'000'}->{'@'}->{hidden}>0) { # LDR field is excluded from $current_record->fields(). -- 1.7.10.4