[Bug 42873] New: Add a faster MARCXML decoder (MARC::File::XML::Fast) for reading records
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 Bug ID: 42873 Summary: Add a faster MARCXML decoder (MARC::File::XML::Fast) for reading records Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org Target Milestone: --- This adds a faster path for decoding the MARCXML stored in biblio_metadata into MARC::Record objects, used when reading bibliographic records. Koha re-parses stored MARCXML on every read via MARC::Record::new_from_xml, which builds a full DOM and runs MARC::Field->new validation for every field. For records that were already validated when stored (bug 35104), that validation is redundant. A new module, MARC::File::XML::Fast, decodes MARCXML with a streaming XML::LibXML::Reader and builds MARC::Field objects by direct bless, skipping the per-field validation. Output is byte-identical to new_from_xml for well-formed UTF-8 MARCXML; malformed input falls back to new_from_xml so a record can never become un-decodable. The module is bundled under lib/ (like WebService::ILS) and is maintained as a standalone CPAN distribution. Koha::Biblio::Metadata->record uses it when the new UseFastMARCDecode system preference is enabled (default on). Depends on bug 35104, which guarantees stored metadata is valid MARCXML. Test plan: 1. Apply the patches, run updatedatabase and restart_all. 2. prove t/MARC/File/XML/Fast.t 3. prove t/db_dependent/Koha/Biblio/Metadata.t 4. Administration > System preferences: confirm UseFastMARCDecode exists under Cataloging > Record structure and defaults to "Use". 5. Browse several biblio detail pages; records display correctly. 6. Set UseFastMARCDecode to "Don't use"; confirm pages render identically. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200619 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200619&action=edit Bug 42873: Add UseFastMARCDecode system preference Kill-switch controlling whether Koha uses the faster MARCXML parser (MARC::File::XML::Fast) when reading bibliographic records from the database. Defaults to ON. - installer/data/mysql/mandatory/sysprefs.sql: seed value '1' - installer/data/mysql/atomicupdate/bug_42873.pl: upgrade - admin/preferences/cataloguing.pref: Cataloging > Record structure Test plan: 1. Apply patch, run updatedatabase 2. Administration > System preferences: confirm 'UseFastMARCDecode' (Cataloging
Record structure) exists and defaults to "Use"
-- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200621 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200621&action=edit Bug 42873: Use MARC::File::XML::Fast in Koha::Biblio::Metadata->record When UseFastMARCDecode is enabled (default), Koha::Biblio::Metadata->record decodes stored MARCXML with MARC::File::XML::Fast instead of MARC::Record::new_from_xml, falling back to new_from_xml on any failure so a record can never become un-decodable. This is safe because store() (bug 35104) guarantees stored metadata is valid MARCXML, making the read-time MARC::Field->new validation redundant. Test plan: 1. prove t/db_dependent/Koha/Biblio/Metadata.t 2. With UseFastMARCDecode on, browse biblio detail pages; records render. 3. Toggle UseFastMARCDecode off; pages render identically. -- 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=42873 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- 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=42873 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |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=42873 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |35104 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 [Bug 35104] We should warn when attempting to save MARC records that contain characters invalid in XML -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #4 from Jonathan Druart <jonathan.druart@gmail.com> --- 1. I don't understand where is coming from MARC::File::XML::Fast 2. If it's faster, why do we put it behind a syspref? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- 1. It's a new module I wrote after profiling.. it does two things under the hood.. * Changes how we parse the XML from DOM to Reader * Skips validation on MARC::Field->new which is called in a loop in MARC::File::XML from MARC::Record.. it's safe to do so in our case after bug 35104 as we should be strongly blocking bad data getting into the database.. however we still catch just in case and fall back to the older method of ->from_marcxml which includes the validation. I'm just opening a discussion with Galen to look at upstreaming it into Marc::Record itself or releasing a new compatible distro on cpan under the same namespace. - https://github.com/mrenvoize/MARC-File-XML-Fast For this bug, I've actually included it as a vendored version under /lib so we don't have to wait for the results of my upstream discussions or packaging efforts. 2. I'm just in the habbit of putting things behind preferences.. your right, it's probably completely superflous. As a side note.. I didn't get quiet as much performance boost as I wanted yet.. iniital intention was to consider an XS or Platypus rust module which gives bigger wins but really we need some of these bits first before the parser side makes much impact. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 200619 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200619 Bug 42873: Add UseFastMARCDecode system preference Review of attachment 200619: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=42873&attachment=200619) ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/bug_42873.pl @@ +9,5 @@
+ my ( $dbh, $out ) = @$args{qw(dbh out)}; + + $dbh->do( + q{ + INSERT IGNORE INTO systempreferences (variable, value, options, explanation, type)
Do we want to default to 1? Also, what's our stance on providing "options, explanation, type" columns on patches for main? ::: installer/data/mysql/mandatory/sysprefs.sql @@ +887,4 @@
('UseCirculationDesks','0'), ('UseControlNumber','0'), ('UseCourseReserves','0'), +('UseFastMARCDecode','1'),
Isn't this one in the wrong spot? I think sysprefs.sql is supposed to be sorted alphabetically? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #7 from David Cook <dcook@prosentient.com.au> --- Have some questions which probably need answering/addressing first -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42873 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- Ohhh it uses XML::LibXML::Reader. I *love* XML::LibXML::Reader. It's so good. I don't know what I think about skipping the MARC::Record->new constructor though. That seems hacky? I notice in the docs it says that the XML is pre-validated as it was validated on write, but that's not necessarily true. If people ingested records via an alternative method that didn't use Koha's internal APIs, that assumption would be false. -- As per my inline comments, wouldn't it make more sense to default to off for a behaviour change like this? -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org