Bug 16365

Summary: Selectively introduce GetMarcStructure() "unsafe" variant for better performance
Product: Koha Reporter: Jacek Ablewicz <abl>
Component: Architecture, internals, and plumbingAssignee: Jacek Ablewicz <abl>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P2 CC: jonathan.druart, kyle, m.de.rooy, veron
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16140
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16431
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19820
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16166    
Bug Blocks:    
Attachments: Bug 16365: Selectively introduce GetMarcStructure() "unsafe" variant for better performance
Bug 16365: Selectivelly introduce GetMarcStructure() "unsafe" variant for better performance
Bug 16365 - GetMarcStructure() "unsafe" variant in PrepareItemrecordDisplay()
Bug 16365: Selectively introduce GetMarcStructure() "unsafe" variant for better performance
Bug 16365 - GetMarcStructure() "unsafe" variant in PrepareItemrecordDisplay()
Bug 16365: Adding additional unsafe call-candidates from Acquisition
Bug 16365: [QA Follow-up] Add some comment lines to Cache.pm
Bug 16365: Selectively introduce GetMarcStructure() "unsafe" variant for better performance
Bug 16365 - GetMarcStructure() "unsafe" variant in PrepareItemrecordDisplay()
Bug 16365: Adding additional unsafe call-candidates from Acquisition
Bug 16365: [QA Follow-up] Add some comment lines to Cache.pm

Description Jacek Ablewicz 2016-04-27 12:03:58 UTC
GetMarcStructure() currently uses Koha::Cache in the "safe" mode (returning deep copy of the result data structure by default), which causes numerous performance issues in many Koha scripts. Switching it to the "unsafe" mode globally (2nd patch from Bug 16140) resolves those issues, but ensuring that it is regression-free (and that it will stay that way in the future) is far from easy.

This patch proposes a bit more manageable solution, it introduces a possibility to use "unsafe" variant selectively (only in those places in the code where GetMarcStructure() is called repetively). That way, amount of the code that needs to be audited for possible problems gets vastly reduced, without any performance trade-offs.
Comment 1 Jacek Ablewicz 2016-04-27 12:07:34 UTC Comment hidden (obsolete)
Comment 2 Jacek Ablewicz 2016-06-14 18:56:41 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-07-25 12:34:20 UTC
--- a/C4/Items.pm
+++ b/C4/Items.pm

@@ -2815,6 +2815,10 @@ sub PrepareItemrecordDisplay {
     my $dbh = C4::Context->dbh;
     $frameworkcode = &GetFrameworkCode($bibnum) if $bibnum;
     my ( $itemtagfield, $itemtagsubfield ) = &GetMarcFromKohaField( "items.itemnumber", $frameworkcode );
+
+    # it would be perhaps beneficial (?) to call GetMarcStructure with 'unsafe' parameter
+    # for performance reasons, but $tagslib may be passed to $plugin->build(), and there
+    # is no way to ensure that this structure is not getting corrupted somewhere in there
     my $tagslib = &GetMarcStructure( 1, $frameworkcode );

Hm. I have the strong impression that the item plugins do not even read the tagslib structure. I kept the parameter in terms of backward compatibility for custom plugins that would need it. It might be an idea to let these custom (item) plugins call GetMarcStructure itself when they really need it.. But yes, that would be another report :)

The assumption that tagslib would be corrupted by a plugin is not very realistic imo; most do not even read, not to mention change. If the included plugins do not, how far should we go in protecting the author of a custom plugin from the potential threat he created himself? We can't stop him now from doing all kinds of things..

So I would not object to calling it unsafe here too. In my tests it makes a remarkable difference: 0.0015 versus 0.046 per GetMarcStructure call. Please note too that PrepareItemrecordDisplay is called twice in a for loop (in serials code: for each item or subscription).
Comment 4 Marcel de Rooy 2016-07-25 12:58:47 UTC
btw this would effectively make your patch a oneliner: just add unsafe in GetMarcStructure ?
Comment 5 Jacek Ablewicz 2016-07-26 09:31:59 UTC
(In reply to Marcel de Rooy from comment #4)
> btw this would effectively make your patch a oneliner: just add unsafe in
> GetMarcStructure ?

You mean that with this patch all GetMarcStructure() calls remaining in the code (including the call in PrepareItemrecordDisplay()) will be 'unsafe => 1' ones?

Hm, it's possible - it's been a while since I reviewed those calls, and I was only having a closer look at those of them that were called repeatedly. Also in the meantime there was hopefully some progress with eliminating unneeded / most offending ones in the other bug reports. I'll need to have a look..
Comment 6 Jacek Ablewicz 2016-07-27 09:34:16 UTC
(In reply to Marcel de Rooy from comment #3)
> --- a/C4/Items.pm
> +++ b/C4/Items.pm
> 
> @@ -2815,6 +2815,10 @@ sub PrepareItemrecordDisplay {
>      my $dbh = C4::Context->dbh;
>      $frameworkcode = &GetFrameworkCode($bibnum) if $bibnum;
>      my ( $itemtagfield, $itemtagsubfield ) = &GetMarcFromKohaField(
> "items.itemnumber", $frameworkcode );
> +
> +    # it would be perhaps beneficial (?) to call GetMarcStructure with
> 'unsafe' parameter
> +    # for performance reasons, but $tagslib may be passed to
> $plugin->build(), and there
> +    # is no way to ensure that this structure is not getting corrupted
> somewhere in there
>      my $tagslib = &GetMarcStructure( 1, $frameworkcode );
> 
> Hm. I have the strong impression that the item plugins do not even read the
> tagslib structure. I kept the parameter in terms of backward compatibility
> for custom plugins that would need it. It might be an idea to let these
> custom (item) plugins call GetMarcStructure itself when they really need
> it.. But yes, that would be another report :)
> 
> The assumption that tagslib would be corrupted by a plugin is not very
> realistic imo; most do not even read, not to mention change. If the included
> plugins do not, how far should we go in protecting the author of a custom
> plugin from the potential threat he created himself? We can't stop him now
> from doing all kinds of things..

+1, overcautiousness is not always a virtue ;). I'll post a follow-up.

> So I would not object to calling it unsafe here too. In my tests it makes a
> remarkable difference: 0.0015 versus 0.046 per GetMarcStructure call. Please
> note too that PrepareItemrecordDisplay is called twice in a for loop (in
> serials code: for each item or subscription).

Ouch, for serial subscriptions with 'serialsadditems' enabled, it might be called a lot of times for a single subscription. This is seriously unoptimized function btw, traverses a whole MARC structure while it only needs 33 or so entries, and also generates a lot of unneeded database traffic. But that's outside the scope of this report.
Comment 7 Jacek Ablewicz 2016-07-27 09:52:30 UTC Comment hidden (obsolete)
Comment 8 Jacek Ablewicz 2016-07-27 11:04:56 UTC
(In reply to Jacek Ablewicz from comment #5)
> (In reply to Marcel de Rooy from comment #4)
> > btw this would effectively make your patch a oneliner: just add unsafe in
> > GetMarcStructure ?
> 
> You mean that with this patch all GetMarcStructure() calls remaining in the
> code (including the call in PrepareItemrecordDisplay()) will be 'unsafe =>
> 1' ones?
> 
> Hm, it's possible - it's been a while since I reviewed those calls, and I
> was only having a closer look at those of them that were called repeatedly.
> Also in the meantime there was hopefully some progress with eliminating
> unneeded / most offending ones in the other bug reports. I'll need to have a
> look..

Hm, in a way - yes, with the PrepareItemrecordDisplay() sorted out, there are hardly any "safe" calls left in the modules (only one in C4/Acquisition.pm ?). There is a bunch of them left in the scripts though. I skipped "safety audit" for the scripts (only checked that there are no repeated direct calls in there).

Right now, if the given script calls GetMarcStructure() only once, and it's doing it directly, adding 'unsafe' flag globally (i.e. oneliner variant) would not benefit this script speed-wise (script starts with empty L1 cache and for the 1st call it needs to fetch a framework structure from L2 anyway).

If any of those scripts is fetching the same framework 2nd+ time indirectly (via some functions from the modules), with the oneliner there would be a small speed benefit (10-15 msec), but now there is a possibility that direct (unaudited) unsafe call will interfere with the further unsafe (audited) indirect call and vice versa. This is purely theoretical, I have no idea if there may really be such problems in some of those scripts or not - I didn't checked, remember that by design this patch is an limited, lazy approach ;)
Comment 9 Marcel de Rooy 2016-07-27 11:09:53 UTC
(In reply to Jacek Ablewicz from comment #8)
> I didn't
> checked, remember that by design this patch is an limited, lazy approach ;)

My lazy approach was: why not change only one line ;)
Will have a look soon.
Comment 10 Marcel de Rooy 2016-07-27 12:15:47 UTC
Just a side note:
dcloning a structure would be safe, but is slower than decode with Sereal:
 dclone 0.0350
 decode 0.0200
For a call to GetMarcStructure.
Comment 11 Marcel de Rooy 2016-07-27 14:55:41 UTC
Still working on this one. Coming back here tomorrow..
Comment 12 Marcel de Rooy 2016-07-28 09:41:42 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2016-07-28 09:41:46 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2016-07-28 09:41:50 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2016-07-28 09:41:54 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2016-07-28 09:43:17 UTC
Did not touch:
catalogue/MARCdetail.pl:my $tagslib = &GetMarcStructure(1,$frameworkcode);
catalogue/getitem-ajax.pl:    my $acq_fw = GetMarcStructure(1, 'ACQ');
catalogue/labeledMARCdetail.pl:my $tagslib = GetMarcStructure(1,$frameworkcode);
cataloguing/addbiblio.pl:$tagslib         = &GetMarcStructure( 1, $frameworkcode
cataloguing/additem.pl:my $tagslib = &GetMarcStructure(1,$frameworkcode);
cataloguing/merge.pl:        my $tagslib = GetMarcStructure(1, $framework);
cataloguing/merge_ajax.pl:my $tagslib = GetMarcStructure(1, $framework);
circ/branchoverdues.pl:my $tagslib = &GetMarcStructure(1,'');
misc/batchDeleteUnusedSubfields.pl:my $tags = GetMarcStructure(1);
opac/opac-MARCdetail.pl:my $tagslib      = &GetMarcStructure( 0, $itemtype );
svc/cataloguing/framework:my $tagslib = GetMarcStructure( 1, $frameworkcode );
t/db_dependent/Biblio.t:    my $tagslib = GetMarcStructure();
tools/batchMod.pl:our $tagslib = &GetMarcStructure(1);

For another report.
Comment 17 Jonathan Druart 2016-08-01 16:05:37 UTC
Created attachment 53847 [details] [review]
Bug 16365: Selectively introduce GetMarcStructure() "unsafe" variant for better performance

GetMarcStructure() currently uses Koha::Cache in the "safe" mode
(returning deep copy of the result data structure by default), which
causes numerous performance issues in many Koha scripts. Switching
it to the "unsafe" mode globally (2nd patch from Bug 16140) resolves
those issues, but ensuring that it is regression-free (and that it
will stay that way in the future) is far from easy. This patch
proposes a bit more manageable solution, it introduces
a possibility to use "unsafe" variant selectively (only in those
places in the code where GetMarcStructure() is called repetitively).
That way, amount of the code that needs to be audited for possible
problems gets vastly reduced, without any performance trade-offs.

Test plan:
1) Have a look at the code and audit the parts affected by this patch
for possible regressions

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended the POD of GetMarcStructure, removing a TODO.

NOTE: GetAuthorisedValueDesc, as called in C4::XSLT::transformMARCXML4XSLT
and by GetISBDView, GetMarcAuthors in C4::Biblio, may autovivify some hash
entries in tagslib.
Same for Koha/Filter/MARC/ViewPolicy.pm, sub filter.

No reason however to worry; our use of this structure in Koha does not
depend on the existence of intermediate hash keys. (We seem to be safe as
long as $tagslib->{$tag}->{$subfield}->{tab} and/or hidden are not filled.)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2016-08-01 16:05:41 UTC
Created attachment 53848 [details] [review]
Bug 16365 - GetMarcStructure() "unsafe" variant in PrepareItemrecordDisplay()

This sub is a good candidate for the "unsafe" treatment too, it doesn't
modify nor autovivify anything in the marc structure.

Added a warning in the code regarding the $tagslib usage by the
custom item plugins, plus a small change to prevent possible
"Use of uninitialized value" warnings in the future.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with neworderempty.pl and itemrecorddisplay.pl.
Amended slightly: Made warning less dramatic.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2016-08-01 16:05:46 UTC
Created attachment 53849 [details] [review]
Bug 16365: Adding additional unsafe call-candidates from Acquisition

[1] Candidate 1 is FillWithDefaultValues.
This routine in Acquisition.pm does not autovivify the tagslib
structure but first collects the keys at tag and subfield level.
So, unsafe can be safely added here :)

[2] Candidate 2 is script acqui/neworderempty.pl.
It only (!) uses GetMarcStructure to know if there is a ACQ framework.
There should be cheaper ways to do it, but when we use the fast cache,
it does not matter that much anymore.

[3] Candidate 3 is script acqui/orderreceive.pl. Same reason as [2].

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2016-08-01 16:05:50 UTC
Created attachment 53850 [details] [review]
Bug 16365: [QA Follow-up] Add some comment lines to Cache.pm

Resolve typo inifinite too.
Adds a few lines in order to stress that the thawed key of the L1
cache SHOULD ONLY be used for unsafe calls, and not be mixed with
regular (safe) calls.

Test plan:
Nothing to test, but verify the quality of the added comments.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Kyle M Hall 2016-09-09 11:48:44 UTC
Pushed to master for 16.11, thanks Jacek, Marcel!