Bug 24715 - Cache repeatable subfield in TransformKohaToMarc
Summary: Cache repeatable subfield in TransformKohaToMarc
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 21800
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-24 07:32 UTC by Marcel de Rooy
Modified: 2020-05-07 20:40 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24715: Cache repeatable subfield in TransformKohaToMarc (3.43 KB, patch)
2020-02-24 13:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24715: Cache repeatable subfield in TransformKohaToMarc (3.43 KB, patch)
2020-02-24 13:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24715: Cache repeatable subfield in TransformKohaToMarc (3.49 KB, patch)
2020-02-24 13:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24715: Cache repeatable subfield in TransformKohaToMarc (3.50 KB, patch)
2020-02-27 11:38 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 24715: Cache repeatable subfield in TransformKohaToMarc (3.54 KB, patch)
2020-04-15 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24715: Improve code of _check_split (4.31 KB, patch)
2020-04-15 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24715: (QA follow-up) Remove $mss2 tric (2.79 KB, patch)
2020-04-15 13:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24715: (QA follow-up) Remove $mss2 tric (2.79 KB, patch)
2020-04-15 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24715: Cache repeatable subfield in TransformKohaToMarc (4.84 KB, patch)
2020-04-15 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2020-02-24 07:32:46 UTC
Follow-up of bug 21800
Comment 1 Marcel de Rooy 2020-02-24 13:10:13 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2020-02-24 13:11:00 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2020-02-24 13:14:09 UTC
Ere: Hopefully this proves to be faster. In a bad case we are reading and caching the whole framework instead of reading one record. But note that a few checks are done before wasting time on that..
Comment 4 Marcel de Rooy 2020-02-24 13:20:35 UTC
Created attachment 99516 [details] [review]
Bug 24715: Cache repeatable subfield in TransformKohaToMarc

Implemented by calling GetMarcSubfieldStructure.

Test plan:
Run t/db_dependent/Biblio/TransformKohaToMarc.t
Optionally, follow the test plan of bug 21800#comment7.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Ere Maijala 2020-02-27 11:11:06 UTC
I think that for this to help the worst case, e.g. export of large number of records with items that have repeated subfields, the framework data should be cached using Koha::Cache like e.g. C4::Biblio::GetMarcStructure does.
Comment 6 Marcel de Rooy 2020-02-27 11:13:55 UTC
(In reply to Ere Maijala from comment #5)
> I think that for this to help the worst case, e.g. export of large number of
> records with items that have repeated subfields, the framework data should
> be cached using Koha::Cache like e.g. C4::Biblio::GetMarcStructure does.

+    $mss2 = GetMarcSubfieldStructure($params->{framework}, { unsafe => 1 })
You mean this ? This comes from the cache.
Comment 7 Ere Maijala 2020-02-27 11:34:11 UTC
(In reply to Marcel de Rooy from comment #6)
> (In reply to Ere Maijala from comment #5)
> > I think that for this to help the worst case, e.g. export of large number of
> > records with items that have repeated subfields, the framework data should
> > be cached using Koha::Cache like e.g. C4::Biblio::GetMarcStructure does.
> 
> +    $mss2 = GetMarcSubfieldStructure($params->{framework}, { unsafe => 1 })
> You mean this ? This comes from the cache.

Oops, err. Right..
Comment 8 Ere Maijala 2020-02-27 11:38:12 UTC
Created attachment 99699 [details] [review]
Bug 24715: Cache repeatable subfield in TransformKohaToMarc

Implemented by calling GetMarcSubfieldStructure.

Test plan:
Run t/db_dependent/Biblio/TransformKohaToMarc.t
Optionally, follow the test plan of bug 21800#comment7.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 9 Marcel de Rooy 2020-02-27 12:15:21 UTC
(In reply to Ere Maijala from comment #8)
> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>

Great!
Comment 10 Jonathan Druart 2020-04-14 14:22:37 UTC
1994     my $mss2; # belongs to $params->{framework} only filled when needed 

I don't understand, can you explain? $mss2 in TransformKohaToMarc is never assigned.
Comment 11 Marcel de Rooy 2020-04-14 14:26:05 UTC
(In reply to Jonathan Druart from comment #10)
> 1994     my $mss2; # belongs to $params->{framework} only filled when needed 
> 
> I don't understand, can you explain? $mss2 in TransformKohaToMarc is never
> assigned.

Ahh you might be right. I am passing undef to the routine that assigns it. So that wont make sense.
If I would pass a reference, it should be fine I guess? See sub _check_split.
Not the most transparent code indeed.
Comment 12 Jonathan Druart 2020-04-14 14:51:45 UTC
Sorry but I do not understand exactly what you are trying to do, so it's hard to help :)
Comment 13 Marcel de Rooy 2020-04-15 12:23:11 UTC
Created attachment 102992 [details] [review]
Bug 24715: Cache repeatable subfield in TransformKohaToMarc

Implemented by calling GetMarcSubfieldStructure.

Test plan:
Run t/db_dependent/Biblio/TransformKohaToMarc.t
Optionally, follow the test plan of bug 21800#comment7.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 14 Marcel de Rooy 2020-04-15 12:23:15 UTC
Created attachment 102993 [details] [review]
Bug 24715: Improve code of _check_split

This routine may check repeatability in a specific framework, and
return the framework hash. The handling of $mss2 to store that
hash is improved here.

Adding a test in TransformKohaToMarc.t to see if we hit the cache.

Test plan:
Run t/db_dependent/Biblio/TransformKohaToMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2020-04-15 12:24:05 UTC
Back in the SO queue. Hopes this makes the code more clear, Jonathan.
Comment 16 Jonathan Druart 2020-04-15 13:14:25 UTC
I think I understand what you are trying to do but, did you benchmark it?

You a putting in a variable something that is L1 cached, then retrieved from there without deep copy.

I am really not sure it is needed.
Comment 17 Marcel de Rooy 2020-04-15 13:26:04 UTC
(In reply to Jonathan Druart from comment #16)
> I think I understand what you are trying to do but, did you benchmark it?
> 
> You a putting in a variable something that is L1 cached, then retrieved from
> there without deep copy.
> 
> I am really not sure it is needed.

Yeah, using the unsafe parameter as done elsewhere.
About cache/benchmarking, see bug 21800 comment 12.
I added it on request.
Note that I only check the framework if it is really needed. So when we actually get a repeated value in a field.
Comment 18 Jonathan Druart 2020-04-15 13:45:43 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > I think I understand what you are trying to do but, did you benchmark it?
> > 
> > You a putting in a variable something that is L1 cached, then retrieved from
> > there without deep copy.
> > 
> > I am really not sure it is needed.
> 
> Yeah, using the unsafe parameter as done elsewhere.
> About cache/benchmarking, see bug 21800 comment 12.
> I added it on request.
> Note that I only check the framework if it is really needed. So when we
> actually get a repeated value in a field.

I am not against the unsafe parameter.
My concern is more about the usefulness of the $mss2 trick.
I have the conviction that is not needed, retrieving $mss (framework specific in _check_split) from the L1 cache should be the only thing you need here.

Basically the patch could only be:
    # here we need to check the specific framework
    my $mss = GetMarcSubfieldStructure($params->{framework}, { unsafe => 1 });
    foreach my $fld2 ( @{ $mss->{ $fld->{kohafield} } } ) {
        next if $fld2->{tagfield} ne $fld->{tagfield};
        next if $fld2->{tagsubfield} ne $fld->{tagsubfield};
        return 1 if $fld2->{repeatable};
    }
Comment 19 Marcel de Rooy 2020-04-15 13:49:29 UTC
Created attachment 103005 [details] [review]
Bug 24715: (QA follow-up) Remove $mss2 tric

As requested by QA, removing the $mss2 variable. This may lead
to repeated calls to the cache.
Comment 20 Marcel de Rooy 2020-04-15 13:52:51 UTC
Created attachment 103006 [details] [review]
Bug 24715: (QA follow-up) Remove $mss2 tric

As requested by QA, removing the $mss2 variable. This may lead
to repeated calls to the cache.
Comment 21 Jonathan Druart 2020-04-15 14:02:18 UTC
Created attachment 103010 [details] [review]
Bug 24715: Cache repeatable subfield in TransformKohaToMarc

Implemented by calling GetMarcSubfieldStructure.

Test plan:
Run t/db_dependent/Biblio/TransformKohaToMarc.t
Optionally, follow the test plan of bug 21800#comment7.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>

Bug 24715: Improve code of _check_split

This routine may check repeatability in a specific framework, and
return the framework hash. The handling of $mss2 to store that
hash is improved here.

Adding a test in TransformKohaToMarc.t to see if we hit the cache.

Test plan:
Run t/db_dependent/Biblio/TransformKohaToMarc.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 24715: (QA follow-up) Remove $mss2 tric

As requested by QA, removing the $mss2 variable. This may lead
to repeated calls to the cache.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Patches have been squashed for readability.
I also re-add the existing syntax for the _check_split call
Comment 22 Jonathan Druart 2020-04-15 14:03:37 UTC
This patch could be slower in some cases, but will bring few DB hits in the majority of the cases I think. Especially if we assume that the framework is already cached in L1.
Comment 23 Jonathan Druart 2020-04-15 14:04:15 UTC
(In reply to Jonathan Druart from comment #22)
> This patch could be slower in some cases, but will bring few DB hits in the
> majority of the cases I think. Especially if we assume that the framework is
> already cached in L1.

s/bring/save
Comment 24 Martin Renvoize 2020-04-17 08:29:28 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 25 Joy Nelson 2020-05-07 20:40:41 UTC
missing dependencies - not backported to 19.11.x