[Koha-patches] [PATCH] Performance enhancemnent.
Joe Atzberger
joe.atzberger at liblime.com
Sat Aug 8 08:47:03 CEST 2009
Yes, though this convention is established already in the module with
$marc_structure_cache. You could convert both to Memoize.
--Joe
On Fri, Aug 7, 2009 at 10:28 PM, Chris Cormack <chrisc at catalyst.net.nz>wrote:
> Hi Joe
>
> This looks good, but couldn't we use memoize on
> _get_bibliolevel_authorised_values () and
> get_authorised_value_images to achieve the caching benefit without needing
> the global variable?
>
> Chris
>
> * Joe Atzberger (joe.atzberger at liblime.com) wrote:
> > get_biblio_authorised_values was identified by NYTprof as the most
> expensive
> > operation during a common catalog search. This adds some caching, and
> > restructures the logic to be smarter to improve performance.
> Specifically, we
> > can check for the existence of the field/subfield in the record *before*
> we get
> > into the loop going through the entire MARC structure for matches.
> >
> > The entire design of this approach is still wasteful, however, since
> really all
> > searches care about is authorised values for images. We should have a
> slim
> > MARC strcuture cached for just auth'd image values. But this should help
> somewhat for now.
> > ---
> > C4/Biblio.pm | 60
> +++++++++++++++++++++++++++++++--------------------------
> > C4/Items.pm | 21 +++++++++----------
> > 2 files changed, 43 insertions(+), 38 deletions(-)
> >
> > diff --git a/C4/Biblio.pm b/C4/Biblio.pm
> > index a7c66f9..038c67e 100644
> > --- a/C4/Biblio.pm
> > +++ b/C4/Biblio.pm
> > @@ -37,7 +37,7 @@ require C4::Serials;
> > use vars qw($VERSION @ISA @EXPORT);
> >
> > BEGIN {
> > - $VERSION = 1.00;
> > + $VERSION = 1.01;
> >
> > require Exporter;
> > @ISA = qw( Exporter );
> > @@ -3422,38 +3422,27 @@ sub set_service_options {
> > =cut
> >
> > sub get_biblio_authorised_values {
> > - my $biblionumber = shift;
> > - my $record = shift;
> > -
> > + my $biblionumber = shift or return;
> > + my $record = shift or return;
> > my $forlibrarian = 1; # are we in staff or opac?
> > - my $frameworkcode = GetFrameworkCode( $biblionumber );
> > -
> > - my $authorised_values;
> >
> > - my $tagslib = GetMarcStructure( $forlibrarian, $frameworkcode )
> > - or return $authorised_values;
> > -
> > - # assume that these entries in the authorised_value table are
> bibliolevel.
> > - # ones that start with 'item%' are item level.
> > - my $query = q(SELECT distinct authorised_value, kohafield
> > - FROM marc_subfield_structure
> > - WHERE authorised_value !=''
> > - AND (kohafield like 'biblio%'
> > - OR kohafield like '') );
> > - my $bibliolevel_authorised_values =
> C4::Context->dbh->selectall_hashref( $query, 'authorised_value' );
> > + my $frameworkcode = GetFrameworkCode( $biblionumber );
> > + my $tagslib = GetMarcStructure($forlibrarian, $frameworkcode)
> or return;
> > + my $bib_auth_vals = _get_bibliolevel_authorised_values();
> >
> > + my $authorised_values;
> > foreach my $tag ( keys( %$tagslib ) ) {
> > + defined $record->field($tag) or next; # FIXME: reorient the
> loop to iterate on just the fields present in the record
> > foreach my $subfield ( keys( %{$tagslib->{ $tag }} ) ) {
> > # warn "checking $subfield. type is: " . ref $tagslib->{
> $tag }{ $subfield };
> > - if ( 'HASH' eq ref $tagslib->{ $tag }{ $subfield } ) {
> > - if ( defined $tagslib->{ $tag }{ $subfield
> }{'authorised_value'} && exists $bibliolevel_authorised_values->{
> $tagslib->{ $tag }{ $subfield }{'authorised_value'} } ) {
> > - if ( defined $record->field( $tag ) ) {
> > - my $this_subfield_value = $record->field( $tag
> )->subfield( $subfield );
> > - if ( defined $this_subfield_value ) {
> > - $authorised_values->{ $tagslib->{ $tag }{
> $subfield }{'authorised_value'} } = $this_subfield_value;
> > - }
> > - }
> > - }
> > + # we check some criteria before proceeding
> > + 'HASH' eq ref $tagslib->{$tag}{$subfield} or next;
> > + my $authval =
> $tagslib->{$tag}->{$subfield}->{authorised_value};
> > + defined $authval or next;
> > + exists $bib_auth_vals->{$authval} or next;
> > + my $this_subfield_value = $record->field( $tag )->subfield(
> $subfield );
> > + if ( defined $this_subfield_value ) {
> > + $authorised_values->{$authval} = $this_subfield_value;
> > }
> > }
> > }
> > @@ -3461,6 +3450,23 @@ sub get_biblio_authorised_values {
> > return $authorised_values;
> > }
> >
> > +our $bibliolevel_authorised_values; # caching a static function
> > +
> > +sub _get_bibliolevel_authorised_values () {
> > + return $bibliolevel_authorised_values if
> $bibliolevel_authorised_values;
> > + # This query produces a static result fit for caching
> > + # assume that these entries in the authorised_value table are
> bibliolevel.
> > + # ones that start with 'item%' are item level.
> > + my $query = q#
> > + SELECT distinct authorised_value, kohafield
> > + FROM marc_subfield_structure
> > + WHERE authorised_value != ''
> > + AND (kohafield like 'biblio%'
> > + OR kohafield like '')
> > + #;
> > + $bibliolevel_authorised_values =
> C4::Context->dbh->selectall_hashref( $query, 'authorised_value' );
> > + return $bibliolevel_authorised_values;
> > +}
> >
> > 1;
> >
> > diff --git a/C4/Items.pm b/C4/Items.pm
> > index 9133756..9979957 100644
> > --- a/C4/Items.pm
> > +++ b/C4/Items.pm
> > @@ -1493,23 +1493,22 @@ sub get_item_authorised_values {
> > =cut
> >
> > sub get_authorised_value_images {
> > - my $authorised_values = shift;
> > + my $authorised_values = shift or return [];
> >
> > my @imagelist;
> >
> > my $authorised_value_list = GetAuthorisedValues();
> > # warn ( Data::Dumper->Dump( [ $authorised_value_list ], [
> 'authorised_value_list' ] ) );
> > foreach my $this_authorised_value ( @$authorised_value_list ) {
> > - if ( exists $authorised_values->{
> $this_authorised_value->{'category'} }
> > - && $authorised_values->{
> $this_authorised_value->{'category'} } eq
> $this_authorised_value->{'authorised_value'} ) {
> > - # warn ( Data::Dumper->Dump( [ $this_authorised_value ], [
> 'this_authorised_value' ] ) );
> > - if ( defined $this_authorised_value->{'imageurl'} ) {
> > - push @imagelist, { imageurl =>
> C4::Koha::getitemtypeimagelocation( 'intranet',
> $this_authorised_value->{'imageurl'} ),
> > - label =>
> $this_authorised_value->{'lib'},
> > - category =>
> $this_authorised_value->{'category'},
> > - value =>
> $this_authorised_value->{'authorised_value'}, };
> > - }
> > - }
> > + # check some required criteria
> > + defined $this_authorised_value->{'imageurl'} or next;
> > + exists $authorised_values->{
> $this_authorised_value->{'category'} } or next;
> > + ($authorised_values->{ $this_authorised_value->{'category'} } eq
> $this_authorised_value->{'authorised_value'}) or next;
> > + # warn ( Data::Dumper->Dump( [ $this_authorised_value ], [
> 'this_authorised_value' ] ) );
> > + push @imagelist, { imageurl =>
> C4::Koha::getitemtypeimagelocation( 'intranet',
> $this_authorised_value->{'imageurl'} ),
> > + label => $this_authorised_value->{'lib'},
> > + category =>
> $this_authorised_value->{'category'},
> > + value =>
> $this_authorised_value->{'authorised_value'}, };
> > }
> >
> > # warn ( Data::Dumper->Dump( [ \@imagelist ], [ 'imagelist' ] ) );
> > --
> > 1.5.6.5
> >
> > _______________________________________________
> > Koha-patches mailing list
> > Koha-patches at lists.koha.org
> > http://lists.koha.org/mailman/listinfo/koha-patches
>
> --
> Chris Cormack
> Catalyst IT Ltd.
> +64 4 803 2238
> PO Box 11-053, Manners St, Wellington 6142, New Zealand
>
--
Joe Atzberger
LibLime - Open Source Library Solutions
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-patches/attachments/20090808/80181a87/attachment-0002.htm>
More information about the Koha-patches
mailing list