Skip to content

PDFBOX-6268: avoid rebuilding key cache when possible - #533

Open
tballison wants to merge 3 commits into
apache:trunkfrom
tballison:objstm-keycache
Open

tballison wants to merge 3 commits into
apache:trunkfrom
tballison:objstm-keycache

Conversation

@tballison

Copy link
Copy Markdown
Contributor

This is 100% Fable 🤖 . Handle carefully.

@tballison

Copy link
Copy Markdown
Contributor Author

Notes from 🤖

● Design choices to review

  1. Wrapper map vs. explicit mutators. The patch keeps COSDocument.getXrefTable() returning a live, mutable Map and maintains the
     hash→key index inside a wrapper, so the index can't go out of sync. The alternative is a plain HashMap plus explicit
     COSDocument.setXrefEntry(key, offset) / clearXrefTable() methods, with XrefParser, BruteForceParser and
     COSParser.getObjectOffset routed through them; that is less code but any direct mutation of the returned map would bypass the
     index. On trunk, a third option is to make getXrefTable() return an unmodifiable view and require the explicit API.
  2. Where the index lives. The patch puts it in COSDocument (document-scoped, survives the parser) because getObjectsByType and
     COSWriter also want the key stored in the xref table. The alternative is passing the parent parser's cache into
     PDFObjectStreamParser, which avoids new public surface on COSDocument but keeps the state parser-scoped.
  3. Replace semantics. put of an already-present key now stores the new key instance, so a new stream index becomes visible;
     HashMap keeps the first instance. This is never hit on the normal parse path (getObjectOffset only puts absent keys,
     brute-force recovery clears first) and matches HashMap value semantics. It makes the remove-then-put in
     PDFObjectStreamParserTest.testParseAllObjectsIndexed unnecessary. Could be reverted or just documented.
  4. Relation to the object pool overhaul. The removed comment noted the object pool should be overhauled or removed. The index
     exists only because COSObjectKey carries streamIndex and a HashMap can't return its stored key; moving the stream index out of
     the key (e.g. into the xref value) would remove the index entirely. This patch is intended as a contained fix for 3.0 and trunk
     rather than that refactoring.
  5. 3.0 backport. Same change applies to 3.0 with BaseParser.getObjectKey instead of COSParser.getObjectKey; Java 8 clean. Open
     question is whether a wrapper-map change behind a public getter is acceptable on the maintenance branch, or whether 3.0 should
     get the smaller explicit-mutator variant from item 1.
  6. API details. New public COSDocument.getObjectKey(long, int) (naming overlaps with the protected COSParser.getObjectKey).
     keySet().remove(x) on the xref table becomes O(n) via the view iterator, and AbstractMap's equals/hashCode/containsValue are
     O(n); none are used in pdfbox, tools, debugger, examples or Tika.
  7. Regression test for the performance issue. None added, since a timing assertion would be flaky; a JMH case in benchmark/ is
     possible if wanted.

@tballison

Copy link
Copy Markdown
Contributor Author

Perf tests as reported by 🤖

● Performance testing

  All runs on the same machine with two jars built from the same checkout: trunk before the patch (old) and after (new). Test files:
  G7BO7PNCCREVF2BCY5YSYOPYDLMBYASY (11.8 MB, 1,377,770 xref entries, 13,646 object streams) and RFFQRH5P3P4YLUL5MWWZIPDQRKAPES2H
  (11.0 MB, 474,562 entries, 4,699 object streams).

  1. Reproduction. COSDocument.getObjectsByType(COSName.FILESPEC) after Loader.loadPDF: old did not finish in 600 s on either file
     (killed). jstack samples of tika-app -t on G7 all sit in BaseParser.getObjectKey → HashMap.putIfAbsent, called from
     PDFObjectStreamParser.parseAllObjects; 7 CPU-minutes in 90 s wall.
  2. After the patch. Same call: G7 3.7 s, RF 2.3 s. PDFTextStripper.getText unchanged (G7 1.2 s, RF 3.6 s) since plain text
     extraction only touches a few object streams.
  3. Load-time regression check. Loader.loadPDF + getNumberOfPages, best of 15 iterations per JVM, 4 JVMs per jar, interleaved
     old/new, -XX:+UseParallelGC: G7 old 0.28–0.32 s vs new 0.22–0.33 s; RF old 0.19 s vs new 0.17 s. Parity. A first version of the
     patch was ~25% slower here because AbstractMap.putAll inserts one entry at a time and rehashes ~17 times on 1.38M entries;
     overriding putAll to bulk-insert into the presized HashMap and then index the keys removed that.
  4. Memory. Retained heap after load and System.gc(): identical old vs new (G7 261 MB, RF 104 MB). The previous per-parser cache
     was already built once in the top-level parser on the first getObjectKey call and lived as long as the document; the patch only
     removes the transient per-object-stream rebuilds (647 GCs / 18 s of pauses per 100 s in the original JFR).
  5. Differential correctness run (also a coarse perf check): 7,590 local PDFs, 5 parallel JVMs, 120 s per-file timeout, each pass
     ~2.5 min for both jars, no timeouts on either side.

@tballison

Copy link
Copy Markdown
Contributor Author

If this is something worth pursuing, we can backport to 3.x and run on the 190k pdfs in Tika's regression corpus.

Comment thread pdfbox/src/main/java/org/apache/pdfbox/cos/COSDocument.java Fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants