On Fri, 2 Jan 2026 02:34:55 GMT, Jeremy Wood <[email protected]> wrote:
>> When decoding an uninterlaced 8-bit PNG image, the PNGImageDecoder is >> basically copying one byte at a time. >> >> This PR uses System.arraycopy instead, and it shows approx a 10% improvement. >> >> This graph shows the time it takes different decoders to convert a byte >> array into a BufferedImage as the size of the PNG image increases: >> >> <img width="596" height="366" alt="Screenshot 2025-12-27 at 9 14 19 PM" >> src="https://github.com/user-attachments/assets/73583cb2-eda0-47a8-b818-735a1835f1e8"; >> /> >> >> (This originally came to my attention when looking at an image in Java 1.8. >> There the ImageConsumer model took approx 400% longer than ImageIO. I was >> happy to see in recent JDKs that gap narrowed significantly, but there was >> still a noticeable 10% discrepancy.) >> >> I haven't tried submitting a performance enhancement PR before; I'm not sure >> if this issue meets this group's threshold for being worth addressing. And >> if it does: I'm not sure how to structure a unit test for it. > > Jeremy Wood has updated the pull request incrementally with one additional > commit since the last revision: > > 8374377: test correctness of non-interlaced PNGs too > > Also this removes the performance comparison. (As of this writing there's a > separate > `test/micro/org/openjdk/bench/sun/awt/image/PNGImageDecoder_8bit_uninterlaced.java` > file used to demonstrate that this change is more performant.) The change in `PNGImageDecoder` makes sense to me, although I haven't had a chance to run the JMH benchmark locally on my end. test/jdk/sun/awt/image/png/PNGImageDecoder_8bit_performance.java line 60: > 58: * This test has never failed. > 59: */ > 60: public class PNGImageDecoder_8bit_performance { Since this is now purely a regression test and the performance aspect has been removed, perhaps rename to e.g. `PngImageDecoder8BitTest`? test/jdk/sun/awt/image/png/PNGImageDecoder_8bit_performance.java line 174: > 172: BufferedImage actual = models[1].load(imageData); > 173: > 174: testCorrectness(expected, actual); Am I correct in assuming that both models end up using `PNGImageDecoder` under the covers? If so, won't `expected` and `actual` always match, even if there is a bug in `PNGImageDecoder`? I wonder if it would be better to keep the original `BufferedImage` around (the one we draw on), use it as `expected`, and compare it to the two model-generated images. test/jdk/sun/awt/image/png/PNGImageDecoder_8bit_performance.java line 233: > 231: BufferedImage actual) { > 232: if (expected.getWidth() != actual.getWidth()) { > 233: throw new Error(); Probably better to throw a `RuntimeException` instead of an `Error` here and below (at least that seems to be the convention that I've seen elsewhere). Also always best to include a short error message that helps zero in on the exact issue if it ever fails. test/jdk/sun/awt/image/png/PNGImageDecoder_8bit_performance.java line 243: > 241: int argb2 = actual.getRGB(x, y); > 242: if (argb1 != argb2) { > 243: throw new Error("x = " + x + ", y = " + y); `Error` -> `RuntimeException`, and I'd probably also include the two colors that didn't match in the message test/micro/org/openjdk/bench/sun/awt/image/PNGImageDecoder_8bit_uninterlaced.java line 76: > 74: @Benchmark > 75: public void measurePNGImageDecoder(Blackhole bh) throws Exception { > 76: Image img = Toolkit.getDefaultToolkit().createImage(pngImageData); Does the `BufferedImage` need to be created this way, or could it be simplified down to a simple `ImageIO.read()` with a `ByteArrayInputStream`? test/micro/org/openjdk/bench/sun/awt/image/PNGImageDecoder_8bit_uninterlaced.java line 172: > 170: * any accuracy. > 171: */ > 172: private static void testCorrectness(BufferedImage expected, Should the correctness check in the JMH benchmark be removed, since that's handled in the unit test? ------------- PR Review: https://git.openjdk.org/jdk/pull/29004#pullrequestreview-3623125208 PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2657759447 PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2657780639 PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2657765027 PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2657763413 PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2657722294 PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2657715697
