On Fri, 2 Jan 2026 00:57:05 GMT, Chen Liang <[email protected]> wrote:

>> Jeremy Wood has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8374377: Adding a JMH test for PNGImageDecoder
>>   
>>   This corroborates previous (more informal) tests that show approx a 10% 
>> improvement.
>>   
>>   Without this PR I observed this result on my Mac 26.2 laptop:
>>   
>>   ```
>>   Benchmark                                                 Mode  Cnt   
>> Score   Error  Units
>>   PNGImageDecoder_8bit_uninterlaced.measurePNGImageDecoder  avgt   15  
>> 25.296 ± 0.521  ms/op
>>   ```
>>   
>>   With this PR I observed this result:
>>   
>>   ```
>>   Benchmark                                                 Mode  Cnt   
>> Score   Error  Units
>>   PNGImageDecoder_8bit_uninterlaced.measurePNGImageDecoder  avgt   15  
>> 22.132 ± 0.378  ms/op
>>   ```
>
> With the patch, the code `case 8: bPixels[col+rowOffset] = 
> rowByteBuffer[spos++];` at old line 415/new line 427 is now dead. Should we 
> replace that site with an assertion, or just move the arraycopy code to there 
> instead?

@liach thanks for checking that far ahead.

I just confirmed that code is NOT dead, though. It should still be used for 
interlaced 8-bit PNGs. `spos` is incremented by 1, but `col` may be incremented 
by different amounts when we reach `col += colInc`.

I rewrote the unit test just now so that:
1. It only tests for correctness (it does NOT try to measure performance 
anymore)
2. It tests correctness of 8-bit non-interlaced PNGs (the crux of this bug) and 
8-bit interlaced PNGs.

I tried removing the line you mentioned (`bPixels[col+rowOffset] = 
rowByteBuffer[spos++];`), and confirmed that the 8-bit interlaced PNG fails 
without that line.

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

PR Comment: https://git.openjdk.org/jdk/pull/29004#issuecomment-3704352410

Reply via email to