Parse each package part once, and bound the total - #44
Conversation
3d40e4d to
baead09
Compare
An EPUB whose reading-order entries all point at the package document parsed that document once per entry, and the document grows with the entry count: a 35 KB file took 27 seconds and a 69 KB one did not finish. Part bytes were cached but parsed trees were not, and the per-part node cap resets on every parse, so nothing bounded the product. Packages now cache parsed trees and hand callers a shared `Rc`, so a part referenced many times parses once. The cache holds up to `max_cached_xml_nodes` (~86 MiB at the measured DOM cost), which covers the parts documents actually repeat; a part too large for it is served uncached and still re-parses, so a running node total across the package bounds that case at `max_document_xml_nodes`. The 35 KB file now converts in 0.03s and the 69 KB one in 0.02s. Repeated slide references in a presentation take about half the time they did. Output is byte-identical across the fixture corpus.
baead09 to
6c958c2
Compare
|
@abimaelmartell I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/package/archive.rs">
<violation number="1" location="src/package/archive.rs:172">
P2: The parsed-tree cache fills monotonically and never evicts: `cached_nodes` only grows and `trees` is never cleared, so once the cache holds `MAX_CACHED_XML_NODES` (200k) nodes, every later part is permanently served uncached — even small parts that are repeated heavily. Combined with the never-decreasing `parsed_nodes` document total, a large-but-legitimate document whose repeated parts come after other parts filled the cache will re-parse each repeat and charge its full node count, and can hit a spurious `max_document_xml_nodes` ResourceLimit even though no part is individually abusive. The cache retains whichever parts parsed first (which may never be re-referenced) while failing to cache the parts that are actually repeated. Consider evicting cached entries (e.g., LRU or dropping the least-useful part when the budget is exceeded) so that repeated parts stay parse-once, or at least re-cache a part that is demonstrably referenced more than once.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| let before = self.parsed_nodes; | ||
| let tree = Rc::new(parse_xml_counted(bytes, &mut self.parsed_nodes)?); | ||
| let nodes = self.parsed_nodes - before; | ||
| if self.cached_nodes + nodes <= limits::MAX_CACHED_XML_NODES { |
There was a problem hiding this comment.
P2: The parsed-tree cache fills monotonically and never evicts: cached_nodes only grows and trees is never cleared, so once the cache holds MAX_CACHED_XML_NODES (200k) nodes, every later part is permanently served uncached — even small parts that are repeated heavily. Combined with the never-decreasing parsed_nodes document total, a large-but-legitimate document whose repeated parts come after other parts filled the cache will re-parse each repeat and charge its full node count, and can hit a spurious max_document_xml_nodes ResourceLimit even though no part is individually abusive. The cache retains whichever parts parsed first (which may never be re-referenced) while failing to cache the parts that are actually repeated. Consider evicting cached entries (e.g., LRU or dropping the least-useful part when the budget is exceeded) so that repeated parts stay parse-once, or at least re-cache a part that is demonstrably referenced more than once.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/package/archive.rs, line 172:
<comment>The parsed-tree cache fills monotonically and never evicts: `cached_nodes` only grows and `trees` is never cleared, so once the cache holds `MAX_CACHED_XML_NODES` (200k) nodes, every later part is permanently served uncached — even small parts that are repeated heavily. Combined with the never-decreasing `parsed_nodes` document total, a large-but-legitimate document whose repeated parts come after other parts filled the cache will re-parse each repeat and charge its full node count, and can hit a spurious `max_document_xml_nodes` ResourceLimit even though no part is individually abusive. The cache retains whichever parts parsed first (which may never be re-referenced) while failing to cache the parts that are actually repeated. Consider evicting cached entries (e.g., LRU or dropping the least-useful part when the budget is exceeded) so that repeated parts stay parse-once, or at least re-cache a part that is demonstrably referenced more than once.</comment>
<file context>
@@ -132,9 +152,28 @@ impl<'a> Package<'a> {
+ let before = self.parsed_nodes;
+ let tree = Rc::new(parse_xml_counted(bytes, &mut self.parsed_nodes)?);
+ let nodes = self.parsed_nodes - before;
+ if self.cached_nodes + nodes <= limits::MAX_CACHED_XML_NODES {
+ self.cached_nodes += nodes;
+ self.trees.insert(name.to_string(), Rc::clone(&tree));
</file context>
Packages now cache parsed part trees, so a part referenced many times parses once instead of once per reference, with a running node total bounding the parts too large to cache.
For issue #43.