Repository navigation
ROB: Scan the file once when resolving undefined references - #4431
SudiptoKumar wants to merge 1 commit into
Conversation
Resolving an indirect reference to an object number that is absent from the xref table fell back to a full-file rescan for every distinct undefined reference, giving O(N x file size) behaviour. Build a recovery index of all 'N G obj' markers on the first such lookup and consult it for later ones, so the file is scanned once per reader. The index keys keep the digit strings as found in the file, so a lookup matches exactly what a per-object re.search would find. Fixes py-pdf#4428
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4431 +/- ##
=======================================
Coverage 98.87% 98.87%
=======================================
Files 59 59
Lines 12299 12309 +10
Branches 2342 2344 +2
=======================================
+ Hits 12161 12171 +10
Misses 68 68
Partials 70 70 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stefan6419846
left a comment
There was a problem hiding this comment.
Besides the code-level comments: How does this affect performance in the different cases, for example when only looking for a few objects, but having to parse a huge string?
| #: digit strings from the file — to the (start, end) offsets of | ||
| #: the first "N G obj" marker, so resolving many undefined | ||
| #: references does not rescan the whole file per lookup. | ||
| self._recovery_index: dict[tuple[bytes, bytes], tuple[int, int]] | None = None |
There was a problem hiding this comment.
Cannot we use a more specific name? Recovery index is rather generic and I would not know at first sight what it refers to.
| self.xref = {} | ||
| self.xref_free_entry: dict[int, dict[Any, Any]] = {} | ||
| self.xref_objStm: dict[int, tuple[Any, Any]] = {} | ||
| #: Index of indirect-object headers, built lazily on the first |
There was a problem hiding this comment.
| #: Index of indirect-object headers, built lazily on the first | |
| #: Index of indirect object headers, built lazily on the first |
| self.xref_free_entry: dict[int, dict[Any, Any]] = {} | ||
| self.xref_objStm: dict[int, tuple[Any, Any]] = {} | ||
| #: Index of indirect-object headers, built lazily on the first | ||
| #: undefined-reference lookup; maps (generation, idnum) — as the |
There was a problem hiding this comment.
| #: undefined-reference lookup; maps (generation, idnum) — as the | |
| #: undefined reference lookup; maps (generation, idnum) - as the |
| self.xref_objStm: dict[int, tuple[Any, Any]] = {} | ||
| #: Index of indirect-object headers, built lazily on the first | ||
| #: undefined-reference lookup; maps (generation, idnum) — as the | ||
| #: digit strings from the file — to the (start, end) offsets of |
There was a problem hiding this comment.
| #: digit strings from the file — to the (start, end) offsets of | |
| #: digit strings from the file - to the (start, end) offsets of |
| finally: | ||
| self._object_stream_resolution_stack.pop() | ||
|
|
||
| def _build_recovery_index(self) -> dict[tuple[bytes, bytes], tuple[int, int]]: |
There was a problem hiding this comment.
Please rename accordingly as well.
| m.start(0) + 1 | ||
| ) | ||
| self.stream.seek(m.end(0) + 1) | ||
| self.xref[indirect_reference.generation][indirect_reference.idnum] = offsets[0] + 1 |
There was a problem hiding this comment.
Consider unpacking offsets before using them for more clarity on what these values refer to. Alternatively, we might even consider named tuples or similar here, although this might be overkill.
|
|
||
| def test_get_object__speed__distinct_invalid_lookups_scan_file_once() -> None: | ||
| # https://github.com/py-pdf/pypdf/issues/4428 | ||
| # Each distinct undefined reference used to trigger its own full-file |
There was a problem hiding this comment.
We are only interested in the correct behavior, not the wrong one.
|
|
||
|
|
||
| def test_get_object__speed__distinct_invalid_lookups_scan_file_once() -> None: | ||
| # https://github.com/py-pdf/pypdf/issues/4428 |
There was a problem hiding this comment.
Please align with other tests - we usually do not use the full issue link here. Additionally, we prefer docstrings here over such comments, if required.
|
|
||
|
|
||
| def test_get_object__recovery_index_finds_object_missing_from_xref() -> None: | ||
| # https://github.com/py-pdf/pypdf/issues/4428 |
| # A second lookup comes from the cache, without another scan. | ||
| with mock.patch("pypdf._reader.get_stream_data", wraps=get_stream_data) as get_stream_data_mock: | ||
| assert reader.get_object(IndirectObject(4, 0, reader)) == 42 | ||
| assert get_stream_data_mock.call_count == 0 |
There was a problem hiding this comment.
| assert get_stream_data_mock.call_count == 0 | |
| get_stream_data_mock.assert_not_called() |
Bug
When
PdfReaderresolves an indirect reference to an object number that is not present in the cross-reference table, it falls back to a full-file regex rescan to try to locate a matchingN G objpattern. Failed lookups are cached per(generation, idnum), so repeated lookups of the same undefined object are cheap — but each distinct undefined reference is a fresh cache miss that triggers its own independent full-file materialization and regex scan. A PDF with N distinct undefined references at a fixed file size S therefore forces N full-file scans: O(N x S).Root cause
PdfReader.get_object()inpypdf/_reader.pycalledget_stream_data(self.stream)followed by are.searchfor that one object on every undefined-reference lookup, with no reuse across lookups.Fix
Build a recovery index once, on the first undefined-reference lookup: a single regex pass over the file records the first
"N G obj"marker for every object, and later lookups consult the index instead of rescanning. The index keys keep the digit strings as found in the file, so a lookup matches exactly what the old per-objectre.searchwould find (e.g."007 0 obj"does not match a lookup for7 0 R). Behaviour is otherwise unchanged: objects present in the file body but missing from the xref table are still recovered the same way, and strict mode still raisesPdfReadErrorfor objects that are not defined.Tests
get_stream_datacalls before the fix, 1 after it.test_get_object__speed__distinct_invalid_lookups_scan_file_once(fails on unmodified main, passes with the fix).test_get_object__recovery_index_finds_object_missing_from_xreflocks in that an object present in the body but absent from the xref table is still recovered.pytest tests/test_reader.py: 252 passed, 2 skipped; the 11 failures fail identically on unmodified main (missing sample files / modules in this environment).ruff checkandmypyclean; changed lines areruff formatclean.Fixes #4428