JarFile.getVersionedEntry scalability with new release cadence

[Alan Bateman]

Would it be possible to send an updated webrev with the patch that is 
proposed for jdk/jdk? One thing that I'm concerned about is that 
META-INF/* is a JAR file concepts and I'd prefer not build up further 
shared secrets that operate on ZipFile (should be consistent JarFile and 
JarEntry when dealing with JAR files).

-Alan

On 12/04/2020 20:06, Eirik Bjørsnøs wrote:
> On Sun, Apr 12, 2020 at 4:18 PM Claes Redestad <claes.redestad at oracle.com>
> wrote:
>
>> I think this is shaping up nicely.
>>
>> If we ensure the array returned from getMetaInfVersions is already
>> sorted (by e.g. using a TreeMap during parse), we could remove the
>> stream expression. And if we check bounds in getVersionedEntry we don't
>> need the field in JarFile at all.
>>
> Yeah, that extra field is kind of redundant. I guess my thought was that
> ordering semantics of MRJ belongs in JarFile, but it's also great to reduce
> duplicated state. Perhaps we could update the ZipFile.metadataVersions
> field comment to say "ordered list"?
>
>
>> In initCEN I think you need to subtract 9 from nlen:
>>
>> getMetaVersion(cen, pos + CENHDR + 9, nlen - 9)
>>
> Nice catch. The constant 9 feels a bit magic here, so maybe we could add a
> comment saying // 9 is "META-INF/".length. (And / or extract a local
> variable)
>
>
>> And in getMetaVersion I think we can avoid creating Strings and parse
>> the version inline. By using 0 as the "not a version" value we can
>> simplify it further:
>>
>>               int version = 0;
>>               for (int i = vstart; i < nend; i++) {
>>                   final byte c = name[i];
>>                   if (c == '/') {
>>                       return version;
>>                   }
>>                   if (c < '0' || c > '9') {
>>                       return 0;
>>                   }
>>                   version = version * 10 + c - '0';
>>                   // Check for overflow
>>                   if (version < 0) {
>>                       return 0;
>>                   }
>>               }
>>
> This is brilliantly clever. I stopped trying to be clever at some point in
> order to maintain readability, but I think this code is both readable _and_
> clever.
>
> The javadocs for getMetaVersion should be updated to specify "Otherwise,
> return 0"
>
> Eirik.