Bug 32464

Created attachment 144558 [details] [review]
Bug 32464: Koha::Item->as_marc_field obsolete option mss

Since Bug 28445 the method Koha::Item->as_marc_field has the option mss not anymore.

This patch removes this in Koha::Filter::MARC::EmbedItems.

Test with :
prove t/db_dependent/Koha/Filter/EmbedItems.t

Created attachment 144604 [details] [review]
Bug 32464: Koha::Item->as_marc_field obsolete option mss

Since Bug 28445 the method Koha::Item->as_marc_field has the option mss not anymore.

This patch removes this in Koha::Filter::MARC::EmbedItems.

Test with :
prove t/db_dependent/Koha/Filter/EmbedItems.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 144608 [details] [review]
Bug 32464: (follow-up) fix t/db_dependent/Koha/Item.t

Remove useless unit tests with mss arg

Test with :
prove t/db_dependent/Koha/Filter/EmbedItems.t

Thanks for testing Joubu :D
I found there are useless unit tests with mss arg to remove

Why did we remove this option? Benchmarking, it seems much faster if we do pass the mss:

            Rate noPassMSS   passMSS
noPassMSS 40.8/s        --      -97%
passMSS   1552/s     3707%        --

Mmmm
as_marc_field() calls C4::Biblio::GetMarcStructure() that should be using cache

(In reply to Fridolin Somers from comment #6)
> Mmmm
> as_marc_field() calls C4::Biblio::GetMarcStructure() that should be using
> cache

It is, but that still has a cost, especially when called per item in a setting with many items per biblio

Sooo... I'm not sure what mss is or does, but should we continue to remove it, get it back or investigate why the new solutions seems to be slower than it should be?

It looks like the mss option was added 4 years ago by Tomas and removed 2 years ago by Joubu:

https://git.koha-community.org/Koha-community/Koha/blame/commit/b52c7d7b3664b81b41bade88cbb3491dcdb81693/Koha/Item.pm#L851

Nick, Tomas and Joubu - maybe you could resolve that between the three of you?

It looks as Frido's patch is correct, because passing mss won't have any effect in the current codebase.

If we decide to bring it back... can it get a nicer/more telling name?

I loved my first implementation performance. Don't remember why it was removed. I would have kept it, as it was an optional parameter, to be used where it makes sense.

mss stands for 'marc subfield structure'. It is named like that everywhere as far as I can tell.

(In reply to Tomás Cohen Arazi from comment #11)
> I loved my first implementation performance. Don't remember why it was
> removed. I would have kept it, as it was an optional parameter, to be used
> where it makes sense.
> 
> mss stands for 'marc subfield structure'. It is named like that everywhere
> as far as I can tell.

I am not expecting any significant performance gains passing a hashref that is actually retrieved from the cache (and 'unsafe' is used here).

(In reply to Jonathan Druart from comment #12)
> (In reply to Tomás Cohen Arazi from comment #11)
> > I loved my first implementation performance. Don't remember why it was
> > removed. I would have kept it, as it was an optional parameter, to be used
> > where it makes sense.
> > 
> > mss stands for 'marc subfield structure'. It is named like that everywhere
> > as far as I can tell.
> 
> I am not expecting any significant performance gains passing a hashref that
> is actually retrieved from the cache (and 'unsafe' is used here).

See comment 5, please test

(In reply to Nick Clemens from comment #13)
> See comment 5, please test

What/how did you test?

What I've done:

Add 1000 items to biblio 1

use t::lib::TestBuilder;
my $b = t::lib::TestBuilder->new;
for (1..1000){
    $b->build_sample_item({biblionumber => 1});
}

time that:
use Modern::Perl;
use Koha::Biblios;
use Koha::Patrons;
my $biblio = Koha::Biblios->find(1);
my $patron = Koha::Patrons->find(51);
my $items = $biblio->items->filter_by_visible_in_opac({ patron => $patron });
my $record = $biblio->metadata->record;
my $record_processor = Koha::RecordProcessor->new({
    filters => [ 'EmbedItems', 'ViewPolicy' ],
    options => { 
        interface     => 'opac',
        frameworkcode => $biblio->frameworkcode,
        items         => [ $items->as_list ],
    }    
});

With this change:
diff --git a/Koha/Item.pm b/Koha/Item.pm
index 7251a8a094f..c1e948f0795 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -939,16 +939,14 @@ with the current mappings configuration.
 =cut
 
 sub as_marc_field {
-    my ( $self ) = @_;
-
-    my ( $itemtag, $itemtagsubfield) = C4::Biblio::GetMarcFromKohaField( "items.itemnumber" );
+    my ( $self, $params ) = @_;
 
-    my $tagslib = C4::Biblio::GetMarcStructure( 1, $self->biblio->frameworkcode, { unsafe => 1 });
+    my $mss = $params->{mss} // C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );
+    #my $mss = C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );
+    my $itemtag = $mss->{'items.itemnumber'}[0]->{tagfield};
+    my $item_field = $mss->{$itemtag};
 
     my @subfields;
-
-    my $item_field = $tagslib->{$itemtag};
-
     my $more_subfields = $self->additional_attributes->to_hashref;
     foreach my $subfield (
         sort {

Switching the 'my $mss' line.

No difference noted (script takes ~4.5sec).

(In reply to Jonathan Druart from comment #14)
> (In reply to Nick Clemens from comment #13)
> > See comment 5, please test
> 
> What/how did you test?
> 

Fortunately, I still had the script I used, it seems we have improved things though, I get much smaller results:
             Rate noPassMSS   passMSS
noPassMSS 14088/s        --      -14%
passMSS   16373/s       16%        --

 
#!/usr/bin/env perl

use strict;
use warnings;

use Benchmark qw/cmpthese timethese/;
use Koha::Biblios;

my $biblio = Koha::Biblios->find( 25 );
my $record = $biblio->metadata->record;
my @items  = $biblio->items->as_list;

my $record_processor = Koha::RecordProcessor->new({
    filters => ['EmbedItems'],
    options => {
        interface => 'opac',
        items     => \@items
    }
});



cmpthese(
    -10,
    {
        noPassMSS => sub {
            $record_processor->process( $record, 0);
        },
        passMSS => sub {
            $record_processor->process( $record, 1);
        },
    }
);


Diff:
diff --git a/Koha/Filter/MARC/EmbedItems.pm b/Koha/Filter/MARC/EmbedItems.pm
index 2839694ff8..7401c3ebb9 100644
--- a/Koha/Filter/MARC/EmbedItems.pm
+++ b/Koha/Filter/MARC/EmbedItems.pm
@@ -67,6 +67,7 @@ Embed items into the MARC::Record object.
 sub filter {
     my $self   = shift;
     my $record = shift;
+    my $pass_mss = shift;
 
     return unless defined $record and ref($record) eq 'MARC::Record';
 
@@ -76,6 +77,8 @@ sub filter {
 
     my @item_fields;
 
+    $mss = undef unless $pass_mss;
+
     foreach my $item ( @{$items} ) {
         push @item_fields, $item->as_marc_field( { mss => $mss } );
     }
diff --git a/Koha/Item.pm b/Koha/Item.pm
index 7251a8a094..f4e701bd58 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -939,15 +939,15 @@ with the current mappings configuration.
 =cut
 
 sub as_marc_field {
-    my ( $self ) = @_;
+    my ( $self, $params ) = @_;
 
     my ( $itemtag, $itemtagsubfield) = C4::Biblio::GetMarcFromKohaField( "items.itemnumber" );
 
-    my $tagslib = C4::Biblio::GetMarcStructure( 1, $self->biblio->frameworkcode, { unsafe => 1 });
+    my $mss = $params->{mss} // C4::Biblio::GetMarcSubfieldStructure( '', { unsafe => 1 } );
 
     my @subfields;
 
-    my $item_field = $tagslib->{$itemtag};
+    my $item_field = $mss->{$itemtag};
 
     my $more_subfields = $self->additional_attributes->to_hashref;
     foreach my $subfield (
diff --git a/Koha/RecordProcessor.pm b/Koha/RecordProcessor.pm
index ec7cbea0ca..fff2e7810a 100644
--- a/Koha/RecordProcessor.pm
+++ b/Koha/RecordProcessor.pm
@@ -172,12 +172,13 @@ return value will be of the same type.
 sub process {
     my $self = shift;
     my $record = shift || $self->record;
+    my $pass_mss = shift;
 
     return unless defined $record;
 
     foreach my $filterobj (@{$self->filters}) {
         next unless $filterobj;
-        $filterobj->filter($record);
+        $filterobj->filter($record, $pass_mss);
     }

(In reply to Nick Clemens from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > (In reply to Nick Clemens from comment #13)
> > > See comment 5, please test
> > 
> > What/how did you test?
> > 
> 
> Fortunately, I still had the script I used, it seems we have improved things
> though, I get much smaller results:
>              Rate noPassMSS   passMSS
> noPassMSS 14088/s        --      -14%
> passMSS   16373/s       16%        --

Yes, I don't think 15% is significant here. I prefer to believe that retrieving from the cache is the best we can do (and keep the code tidy), instead of passing around something we have cached. From how many items this will be useful?
If you want to improve that (from 15 to 7%), you could use the memory cache that would use the regular cache to populate itself.

Bug if you are really chasing performance then help the big things (like 33568?) and you should see a real gain.

Created attachment 151542 [details] [review]
Bug 32464: Koha::Item->as_marc_field obsolete option mss

Since Bug 28445 the method Koha::Item->as_marc_field has the option mss not anymore.

This patch removes this in Koha::Filter::MARC::EmbedItems.

Test with :
prove t/db_dependent/Koha/Filter/EmbedItems.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 151543 [details] [review]
Bug 32464: (follow-up) fix t/db_dependent/Koha/Item.t

Remove useless unit tests with mss arg

Test with :
prove t/db_dependent/Koha/Filter/EmbedItems.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Pushed to master for 23.11.

Nice work everyone, thanks!

Nice work everyone!

Pushed to oldstable for 22.11.x