[Bug 42880] New: Avoid the USMARC round-trip when building ES documents
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 Bug ID: 42880 Summary: Avoid the USMARC round-trip when building ES documents Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Searching - Elasticsearch Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org Target Milestone: --- For every record, marc_records_to_documents serializes the record to USMARC and then re-parses it with MARC::Record->new_from_usmarc, purely to "prove it will round-trip" before storing marc_data (Koha/SearchEngine/Elasticsearch.pm). Profiling shows this re-decode is roughly 20-25% of the per-record mapping CPU. The round-trip is a safety net to fall back to MARCXML storage when a record cannot be represented as USMARC (e.g. length > 99999). That check could run only when the encode actually fails, rather than decoding every record on the happy path. Test plan: 1. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t 2. Confirm marc_data is unchanged for normal records. 3. Confirm an over-long record still falls back to MARCXML storage. 4. Benchmark marc_records_to_documents before/after. -- 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=42880 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42881 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42881 [Bug 42881] Improve Elasticsearch Indexing performance -- 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=42880 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | -- 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=42880 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Assignee|nick@bywatersolutions.com |martin.renvoize@openfifth.c | |o.uk QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200647 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200647&action=edit Bug 42880: Avoid the USMARC round-trip when building ES documents For every record, marc_records_to_documents serialized it to USMARC and then re-parsed it with MARC::Record->new_from_usmarc, purely to "prove it will round-trip" before storing marc_data. Profiling shows that re-decode is ~20-25% of the per-record mapping CPU. The round-trip only needs to fire for records that cannot be represented in ISO 2709. Those cases are cheaply detectable from the record itself: - the total length overflows the 5-digit leader length (> 99999), - a single field overflows the 4-digit directory length (> 9999), or - field data contains raw ISO 2709 separators (0x1d/0x1e/0x1f). as_usmarc() carps only about the first. We now check all three and only run the full round-trip decode when one is hit; on the happy path it is skipped. The separator check makes the skip provably safe rather than relying on MARC::Record's tolerance of stray separators. Those characters are invalid in XML and are stripped on save by bug 35104, so in practice they never reach indexing -- this bug depends on 35104 and the check is belt-and-suspenders. The MARCXML fallback for non-representable records is unchanged. Test plan: 1. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t 2. Confirm a normal record is stored as base64ISO2709 and round-trips. 3. Confirm an over-long / over-long-field record falls back to MARCXML. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 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=42880 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #2 from David Cook <dcook@prosentient.com.au> --- Might need to fix bug 27365 first 😅 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- In the case of Elasticsearch, we should just switch away from USMARC https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38270 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- (In reply to David Cook from comment #2)
Might need to fix bug 27365 first 😅
Actually.. look at the code.. this is designed to work around that so they can go in either order -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize (ashimema) from comment #4)
(In reply to David Cook from comment #2)
Might need to fix bug 27365 first 😅
Actually.. look at the code.. this is designed to work around that so they can go in either order
Ahhh yes I see now. I just took the "Avoid the USMARC round-trip when building ES documents" at face value and assumed it meant in all cases haha. I'll add this one to the list... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=27365, | |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38270 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208524 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208524&action=edit Benchmark scripts for full-reindex / batch indexing (bug 42881 family) Clarifying the goal of this work (bug 42881 family): the target is infrastructure-facing - the time and CPU a FULL REINDEX (misc/search_tools/rebuild_elasticsearch.pl) and LARGE BATCH re-indexes (batch record modification, imports; these enqueue index jobs of up to chunk_size = 5000 records) consume. It is not about how quickly a cataloguer sees one edit: in KTD we measured the edit-to-searchable time through the stock es_indexer_daemon and it is dominated by job dispatch (roughly one job per second), so these patches do not change it. I am attaching a small benchmark package (bug42881-es-indexing-bench.tar.gz, see README.txt inside) so testers can reproduce the before/after numbers below and, importantly, verify that the generated Elasticsearch documents are byte-for-byte identical (SHA-256 over every document) with and without the patch. Setup, once, in a KTD with Elasticsearch (stock sample data works; 10,000 generated records give steadier numbers - gen_records.pl + bulkmarcimport.pl, see README): mkdir -p _bench && cp <extracted files> _bench/ Test plan for this bug 1. prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t (includes the round-trip / over-length fallback subtests) 2. Before/after, with this patch applied on a branch: _bench/ab_bench.sh main HEAD Expect: "output identical to baseline" = yes at every batch size. This is the important check for this patch. 3. Fallback still works: index a record with a single field longer than 9999 bytes (or a record over 99999 bytes in total); it must be stored as MARCXML (ElasticsearchMARCFormat fallback), while a normal record is stored as base64ISO2709. Both must be retrievable from search results. 4. Optional, full reindex wall-clock: bash _bench/bench_rebuild.sh before / after. Our results (10,436 generated records, KTD + ES 9, one core, best of 3 interleaved rounds, spread 0-4%), decode + mapping, ms per record: batch 1: 24.8 -> 24.5 (1.01x) batch 10: 6.6 -> 6.3 (1.05x) batch 500: 4.6 -> 4.3 (1.08x) full rebuild_elasticsearch.pl: 103.8 s -> 100.4 s (1.03x) Honest note: our earlier profile suggested the round trip was 20-25% of mapping CPU, but end to end we only measure a few percent on this data set. That may be down to our synthetic records (small, with few fields); records with many large fields may benefit more. Please check this one specifically, and please also note the dependency discussion: this patch's rationale mentions bug 35104 (separator stripping on store), of which only the Metadata::Error classes are currently on main; the patch applies and is safe without it because of the separator check it adds. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 --- Comment #7 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 208529 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208529&action=edit Bug 42880: Avoid the USMARC round-trip when building ES documents (rebased) Rebased on top of 42879, which this bug now depends on (it still depends on 35104 too). The module change is unchanged; the new unit test was re-placed and the Test::More plan count updated so the series applies in sequence. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200647|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |42879 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42879 [Bug 42879] Avoid per-record plugin lookup in ES marc_records_to_documents -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42880 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Correction to my earlier benchmark comment: I said that only the Metadata::Error classes of bug 35104 are currently on main. That was wrong. Bug 35104 is fully on main (store-time repair/validation included), so this bug's dependency on it is already satisfied and the rationale in the commit message (stray ISO 2709 separators being stripped or rejected on save) holds. The separator check added here remains a belt-and-braces guard. Sorry for the noise. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org