[Bug 32464] New: Koha::Item->as_marc_field obsolete option mss
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Bug ID: 32464 Summary: Koha::Item->as_marc_field obsolete option mss Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: fridolin.somers@biblibre.com QA Contact: testopia@bugs.koha-community.org 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |fridolin.somers@biblibre.co |ity.org |m -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |28445 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28445 [Bug 28445] Use the task queue for the batch delete and update items tool -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #1 from Fridolin Somers <fridolin.somers@biblibre.com> --- Created attachment 144558 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144558&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=32463 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144558|0 |1 is obsolete| | --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 144604 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144604&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #3 from Fridolin Somers <fridolin.somers@biblibre.com> --- Created attachment 144608 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144608&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #4 from Fridolin Somers <fridolin.somers@biblibre.com> --- Thanks for testing Joubu :D I found there are useless unit tests with mss arg to remove -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- 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% -- -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #6 from Fridolin Somers <fridolin.somers@biblibre.com> --- Mmmm as_marc_field() calls C4::Biblio::GetMarcStructure() that should be using cache -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion CC| |magnus@libriotech.no --- Comment #8 from Magnus Enger <magnus@libriotech.no> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com, tomascohen@gmail.com --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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/b52c7d7b3664... 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- If we decide to bring it back... can it get a nicer/more telling name? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #11 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #12 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #14 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #15 from Nick Clemens <nick@bywatersolutions.com> --- (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); } -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #16 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144604|0 |1 is obsolete| | --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 151542 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151542&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144608|0 |1 is obsolete| | --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 151543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151543&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |matt.blenkinsop@ptfs-europe | |.com Status|Pushed to master |Pushed to stable Version(s)|23.11.00 |23.11.00,23.05.01 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|23.11.00,23.05.01 |23.11.00,23.05.01,22.11.07 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32464 --- Comment #20 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Nice work everyone! Pushed to oldstable for 22.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org