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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
