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..

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>

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.

(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.

(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..

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>

(In reply to Ere Maijala from comment #8)
> Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>

Great!

1994     my $mss2; # belongs to $params->{framework} only filled when needed 

I don't understand, can you explain? $mss2 in TransformKohaToMarc is never assigned.

(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.

Sorry but I do not understand exactly what you are trying to do, so it's hard to help :)

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>

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>

Back in the SO queue. Hopes this makes the code more clear, Jonathan.

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.

(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.

(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};
    }

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.

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.

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

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.

(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

Nice work everyone!

Pushed to master for 20.05

missing dependencies - not backported to 19.11.x