RFR: 8374377: PNGImageDecoder Slow For 8-bit PNGs [v3]

[Daniel Gredler]

On Fri, 2 Jan 2026 02:34:55 GMT, Jeremy Wood <jwood at openjdk.org> 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