perf(docx): resolve an exported image once instead of twice - #555
Conversation
writeImage needs the node's data twice over -- the bytes it writes and the intrinsic size it measures against, then the box the frame gets -- and asked for it twice. Resolving is not free: ImageSourceCache.fromBytes copies the array whole and takes a SHA-256 over it, so a 10 MB image paid two copies and two hashes on every export, and a path-sourced one announced its arrival twice in the log. resolveImageDimensions gains an overload taking the ImageData the caller already holds. The existing two-argument form resolves and delegates, so the layout path is untouched and nothing else has to change. Counted through the log rather than through a spy: DocumentImageData is final, so there is no seam to instrument, and the arrival message is the one thing the resolution leaves behind that a test can see from outside. Red before the fix with the message logged twice.
…zed it Removing the second resolve left the larger half in place: the bytes still came from their own path -- readAllBytes for a path source, a defensive copy for a byte source -- while the metadata came from the cache. That is a second read, and worse than a second read. The source cache keys on the path alone, with no regard for what the file has since become. So once a first render warmed it, a file rewritten underneath gave writeImage fresh bytes off disk and the previous version's metadata, and the document carried a picture embedded at another image's dimensions. Nothing reported it: both halves succeeded, they simply described different files. Bytes now come from the ImageData that sized the frame, so the two cannot disagree. readBytes goes with them. An unreadable source used to leave through that method's swallowed exception, silently. Resolving throws instead, so the catch is explicit and keeps the export alive -- one bad image should not cost the document -- but says what it dropped, as every other dropped node here does. The new case pins the divergence rather than the cost: render, rewrite the file with a differently-sized image, render again, and read the embedded picture back. It reports 90 wide against a frame built for 40 before the fix.
DemchaAV
commented
Aug 14, 2026
Confirmed, and it is worse than a second read — thank you for pushing on it. I fixed the second resolve and left the larger half standing. The divergence is real and silent. Fixed as you suggested — bytes come from the ImageDataresolved = NodeDefinitionSupport.toImageData(node.imageData());
byte[] bytes = resolved.getBytes();
On the silent skip: it was not a contract, it was a side effect — The new case pins the divergence, not the cost, which is the gap you identified in the first test: render, rewrite the file with a differently-sized image, render again, then read the embedded picture back out of the — a 90-wide picture in a frame built for a 40-wide one. The arrival-count test stays, since the two failures are different: one is a repeated resolution, the other is two sources disagreeing. Full reactor |
Catching around the resolver widened the silence rather than preserving it. The old readBytes swallowed one narrow case -- a path that would not read -- and even that was a side effect of its catch, not a contract. Wrapping the resolver put corrupt bytes, a format with no reader, a metadata decode that gives up and any defect in the cache on the same footing: a picture missing from the document, reported once per export and only for the first one. The catch is gone. A resolution failure now reaches the caller, which is what it did for every one of those cases before this branch touched the method. The case that changes is the unreadable path, and it changes for the better: an export that quietly returns one picture short is the worst of the available answers. Pinned by a test, so it is a decision rather than a side effect of removing code.
DemchaAV
commented
Aug 14, 2026
Right on both counts, and the second one is the part I got wrong: I read the old behaviour as "this backend skips unreadable images" and preserved it, when it was a side effect of The catch is gone. A resolution failure reaches the caller, which is exactly what it did for every one of those cases before this branch touched the method. The one case that changes is the unreadable path, and it changes for the better — a document that quietly comes back one picture short is the worst of the available answers. I pinned it with I have not introduced a typed source-read exception here. Agreed that it belongs in its own change with one policy across the backends, rather than being invented in a PR about resolving an image once. Full reactor |
isInstanceOf(Exception.class) passed for any failure at all, including one that had nothing to do with reading the source -- so the case would have gone on green through a change that broke the export for an entirely different reason. It names IllegalStateException and the phrase the source read throws, matched on the fragment rather than the whole message since the rest of it is a temp-directory path.
Closes the remaining half of #531.
Why
DocxSemanticBackend.writeImageneeds the node's data twice over — the bytes it writes and the intrinsic size it measures the fit against, then the box the frame gets — and asked for it twice:Resolving is not free.
ImageSourceCache.fromBytescopies the byte array whole (Arrays.copyOf) and takes a SHA-256 over it, so a 10 MB image paid two full copies and two hashes on every export. A path-sourced image also announced its arrival twice in the log, which is what made the duplication visible in the first place.The other half of #531 — a row cell dropping its paragraph alignment — was fixed in #547, where the cell walk started going through
applyParagraphProperties.What changed
resolveImageDimensionsgains a three-argument overload taking theImageDatathe caller already holds. The existing two-argument form resolves and delegates to it, so the layout path (ImageDefinition) is untouched and the DOCX backend is the only caller that changes.Verification
DocxImageResolutionTestexports a path-sourced image and counts the arrival messages: one image, one resolution. Red before the fix, with the message logged twice —Counted through the log rather than through a spy on purpose:
DocumentImageDataisfinal, so there is no seam to instrument, and instrumenting production code to make a cost measurable would put test-only machinery insrc/main. The arrival message is the one observable the resolution already leaves behind.What the test cannot see directly is the copy and the hash — those have no observable of their own. It counts the resolution, and the copy and hash are what a resolution costs.
./mvnw clean verifyover the full reactor — 13/13 modules,BUILD SUCCESS.