From 7bdc106c986fb2ca57426797b3f9b85d2abab4a3 Mon Sep 17 00:00:00 2001 From: Jacek Ablewicz Date: Wed, 27 Apr 2016 13:19:10 +0200 Subject: [PATCH] Bug 16365: Selectively 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 selectively (only in those places in the code where GetMarcStructure() is called repetitively). 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 Signed-off-by: Marcel de Rooy Amended the POD of GetMarcStructure, removing a TODO. NOTE: GetAuthorisedValueDesc, as called in C4::XSLT::transformMARCXML4XSLT and by GetISBDView, GetMarcAuthors in C4::Biblio, may autovivify some hash entries in tagslib. Same for Koha/Filter/MARC/ViewPolicy.pm, sub filter. No reason however to worry; our use of this structure in Koha does not depend on the existence of intermediate hash keys. (We seem to be safe as long as $tagslib->{$tag}->{$subfield}->{tab} and/or hidden are not filled.) Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall --- 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 fd8f58cac2..a1f70b8f3f 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -940,7 +940,7 @@ sub GetISBDView { my $framework = $params->{framework}; my $itemtype = $framework; 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; @@ -1117,22 +1117,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 +$params allows you to pass { unsafe => 1 } for better performance. + +Note: If you call GetMarcStructure with unsafe => 1, do not modify or +even autovivify its contents. It is a cached/shared data structure. Your +changes c/would be passed around in subsequent calls. =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::Caches->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; @@ -1957,10 +1963,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 0bca944efc..e700f6f714 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -2475,7 +2475,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 @@ -2859,6 +2859,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 1da6ef1644..534226c7d7 100644 --- a/C4/XSLT.pm +++ b/C4/XSLT.pm @@ -66,7 +66,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 69a144c26e..e69b7303b7 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(). -- 2.20.1