Bug 32464 - Koha::Item->as_marc_field obsolete option mss
Summary: Koha::Item->as_marc_field obsolete option mss
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 28445
Blocks:
  Show dependency treegraph
 
Reported: 2022-12-13 21:19 UTC by Fridolin Somers
Modified: 2023-12-28 20:47 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.01,22.11.07


Attachments
Bug 32464: Koha::Item->as_marc_field obsolete option mss (1.22 KB, patch)
2022-12-13 21:26 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 32464: Koha::Item->as_marc_field obsolete option mss (1.29 KB, patch)
2022-12-15 08:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32464: (follow-up) fix t/db_dependent/Koha/Item.t (1.77 KB, patch)
2022-12-15 09:11 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 32464: Koha::Item->as_marc_field obsolete option mss (1.35 KB, patch)
2023-05-22 18:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32464: (follow-up) fix t/db_dependent/Koha/Item.t (1.83 KB, patch)
2023-05-22 18:34 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2022-12-13 21:19:24 UTC
Since Bug 28445 the method Koha::Item->as_marc_field has the option mss not anymore.
It is still existing in Koha::Filter::MARC::EmbedItems
Comment 1 Fridolin Somers 2022-12-13 21:26:15 UTC
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
Comment 2 Jonathan Druart 2022-12-15 08:18:33 UTC
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>
Comment 3 Fridolin Somers 2022-12-15 09:11:05 UTC
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
Comment 4 Fridolin Somers 2022-12-15 09:12:04 UTC
Thanks for testing Joubu :D
I found there are useless unit tests with mss arg to remove
Comment 5 Nick Clemens 2022-12-15 13:06:01 UTC
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%        --
Comment 6 Fridolin Somers 2022-12-16 06:19:04 UTC
Mmmm
as_marc_field() calls C4::Biblio::GetMarcStructure() that should be using cache
Comment 7 Nick Clemens 2022-12-16 12:41:04 UTC
(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
Comment 8 Magnus Enger 2023-03-28 13:07:01 UTC
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?
Comment 9 Katrin Fischer 2023-04-16 10:29:15 UTC
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.
Comment 10 Katrin Fischer 2023-04-16 10:29:41 UTC
If we decide to bring it back... can it get a nicer/more telling name?
Comment 11 Tomás Cohen Arazi 2023-04-16 11:35:53 UTC
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.
Comment 12 Jonathan Druart 2023-05-02 09:31:21 UTC
(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).
Comment 13 Nick Clemens 2023-05-02 10:59:25 UTC
(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
Comment 14 Jonathan Druart 2023-05-02 11:45:26 UTC
(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).
Comment 15 Nick Clemens 2023-05-02 12:38:09 UTC
(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);
     }
Comment 16 Jonathan Druart 2023-05-02 13:33:54 UTC
(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.
Comment 17 Nick Clemens 2023-05-22 18:33:57 UTC
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>
Comment 18 Nick Clemens 2023-05-22 18:34:00 UTC
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>
Comment 19 Tomás Cohen Arazi 2023-06-06 18:39:58 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 20 Matt Blenkinsop 2023-06-12 11:43:00 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x