From 5f2ca40a90dbb75e565fb842ae782b4507d09746 Mon Sep 17 00:00:00 2001 From: Jacek Ablewicz Date: Wed, 27 Apr 2016 13:19:10 +0200 Subject: [PATCH] Bug XXXXX: 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) What test plan? There is no test plan :) 2) 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 | 23 +++++++++++++++-------- C4/Items.pm | 7 +++++-- C4/XSLT.pm | 2 +- Koha/Filter/MARC/ViewPolicy.pm | 2 +- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index a59b1b7..5fe6022 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..'; @@ -3533,7 +3540,7 @@ sub get_biblio_authorised_values { my $authorised_values; - my $tagslib = GetMarcStructure( $forlibrarian, $frameworkcode ) + my $tagslib = GetMarcStructure( $forlibrarian, $frameworkcode, { unsafe => 1 }) or return $authorised_values; # assume that these entries in the authorised_value table are bibliolevel. diff --git a/C4/Items.pm b/C4/Items.pm index c4f239b..4fe2792 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -455,7 +455,6 @@ sub _build_default_values_for_mod_marc { my ($frameworkcode) = @_; return $default_values_for_mod_from_marc{$frameworkcode} if exists $default_values_for_mod_from_marc{$frameworkcode}; - my $marc_structure = C4::Biblio::GetMarcStructure( 1, $frameworkcode ); my $default_values = { barcode => undef, booksellerid => undef, @@ -2530,7 +2529,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 @@ -2914,6 +2913,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 dd13c50..d7c8f63 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