RFR: 8341594: Use Unsafe to coalesce reads in java.util.zip.ZipUtils [v2]
Claes Redestad
redestad at openjdk.org
Sun Oct 6 16:15:36 UTC 2024
On Sun, 6 Oct 2024 14:56:27 GMT, Eirik Bjørsnøs <eirbjo at openjdk.org> wrote:
>> Claes Redestad has updated the pull request incrementally with one additional commit since the last revision:
>>
>> copyright
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1644:
>
>> 1642: // Now scan the block backwards for END header signature
>> 1643: for (int i = buf.length - ENDHDR; i >= 0; i--) {
>> 1644: if (get32(buf, i) == ENDSIG) {
>
> Maybe a matter of personal preference, but I think `GETSIG(buf, i) == ENDSIG` reads better than `get32(buf, i) == ENDSIG`.
>
> The fact that it's 32 bits is kind of a detail and it doesn't reveal as well that we intend to read a signature.
>
> So could we keep GETSIG, but add an index? There are places in `ZipInputStream` as well which could make use of that for signature checking. (But maybe not for this PR)
>
> Alternatively, `ENDSIG(buf, i) == ENDSIG` would be consistent with `CENSIG(buf, i)` uses.
>
> Same applies to the other GETSIG replacements in this file.
I think all the `GETSIG(byte[])` methods are quite nasty, and it's all used very inconsistently. I wouldn't mind going the other way and removing _all_ the `CENSIG`, `LOCNAM` etc methods and just call `get16/32/32S/64(buf, <field name>)` as appropriate.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21377#discussion_r1789151311
More information about the core-libs-dev
mailing list