You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pdfbox.apache.org by "tballison (via GitHub)" <gi...@apache.org> on 2026/09/16 13:01:33 UTC
[PR] PDFBOX-6268: avoid rebuilding key cache when possible [pdfbox]
tballison opened a new pull request, #533:
URL: https://github.com/apache/pdfbox/pull/533
This is 100% Fable :robot: . Handle carefully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] PDFBOX-6268: avoid rebuilding key cache when possible [pdfbox]
Posted by "tballison (via GitHub)" <gi...@apache.org>.
tballison commented on PR #533:
URL: https://github.com/apache/pdfbox/pull/533#issuecomment-5697892767
Notes from :robot:
```
● 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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] PDFBOX-6268: avoid rebuilding key cache when possible [pdfbox]
Posted by "tballison (via GitHub)" <gi...@apache.org>.
tballison commented on PR #533:
URL: https://github.com/apache/pdfbox/pull/533#issuecomment-5697905351
Perf tests as reported by :robot:
```
● 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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] PDFBOX-6268: avoid rebuilding key cache when possible [pdfbox]
Posted by "tballison (via GitHub)" <gi...@apache.org>.
tballison closed pull request #533: PDFBOX-6268: avoid rebuilding key cache when possible
URL: https://github.com/apache/pdfbox/pull/533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] PDFBOX-6268: avoid rebuilding key cache when possible [pdfbox]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr commented on PR #533:
URL: https://github.com/apache/pdfbox/pull/533#issuecomment-5890103600
@tballison thank you, please close this PR, as this was committed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] PDFBOX-6268: avoid rebuilding key cache when possible [pdfbox]
Posted by "tballison (via GitHub)" <gi...@apache.org>.
tballison commented on PR #533:
URL: https://github.com/apache/pdfbox/pull/533#issuecomment-5697949594
If this is something worth pursuing, we can backport to 3.x and run on the 190k pdfs in Tika's regression corpus.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] PDFBOX-6268: avoid rebuilding key cache when possible [pdfbox]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #533:
URL: https://github.com/apache/pdfbox/pull/533#discussion_r4028445983
##########
pdfbox/src/main/java/org/apache/pdfbox/cos/COSDocument.java:
##########
@@ -610,5 +626,115 @@
{
return documentState;
}
-
+
+ /**
+ * Xref table which also indexes its keys by internal hash, so the stored key instance (which carries the object
+ * stream index) can be looked up without scanning; HashMap has no API for that. Every method that can insert a
+ * key is overridden to index the inserted instance. Removals through views are not intercepted, so the index
+ * may hold keys no longer in the table; {@link #getKey(long, int)} checks for that.
+ */
+ private static final class XrefTable extends HashMap<COSObjectKey, Long>
Review Comment:
## CodeQL / No clone method
No clone method, yet implements Cloneable.
[Show more details](https://github.com/apache/pdfbox/security/code-scanning/619)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org