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

[Jeremy Wood]

On Fri, 2 Jan 2026 13:56:54 GMT, Daniel Gredler <dgredler at openjdk.org> wrote:

>> 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.)
>
> 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`?

This is renamed

> 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.

This is updated

> 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

This is updated

> Am I correct in assuming that both models end up using PNGImageDecoder under the covers?

It'd make a lot of sense if they shared the same code, but no. It is my understanding we have two separate decoders. ImageIO uses the com.sun.imageio.plugins.png.PNGImageReader , and this PR modifies the sun.awt.image.PNGImageDecoder.

The fact that the PNGImageDecoder is a little slower than the ImageIO classes came to my attention because I was comparing the two decoding models, and the older sun.awt classes [appear to be faster](https://docs.google.com/spreadsheets/d/1SoiqnDPSVALb4xraq5hBIGAQLrOQTzA-XuVDohZbCJs/edit?usp=sharing) than the newer ImageIO classes -- except for this one case. This PR will fix this discrepancy.

(Separately: I've submitted a few other PRs that similarly focus on the older sun.awt decoding classes. See [JDK-8357034](https://github.com/openjdk/jdk/pull/25264) , [JDK-8356320](https://github.com/openjdk/jdk/pull/25076) , [JDK-8356137](https://github.com/openjdk/jdk/pull/25044) , [JDK-8351913](https://github.com/openjdk/jdk/pull/24271) . They generally compare the two models against each other for correctness. I also visually inspected the results at the time to triple-check, but in all of those cases ImageIO was already "doing the right thing". I guess I've made it my goal to bring the older sun.awt decoding classes up-to-date.)

> 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.

Sure. I updated this test so it avoids decoding with ImageIO.

> 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`?

ImageIO is a different rendering model; the BufferedImage needs to be created this way. (Or maybe (?) with a PixelGrabber or MediaTracker?)

> 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?

Yes, this is updated

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2658709767
PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2658709810
PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2658709798
PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2658709823
PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2658709730
PR Review Comment: https://git.openjdk.org/jdk/pull/29004#discussion_r2658709701