JarFile.getVersionedEntry scalability with new release cadence

[Eirik Bjørsnøs]

I noticed that in JarFile.getEntry(name) , the first call to
getEntry0(name) call is thrown away for cases where getVersionedEntry
actually finds a versioned entry.

    public ZipEntry getEntry(String name) {
        JarFileEntry je = getEntry0(name);
        if (isMultiRelease()) {
            return getVersionedEntry(name, je);
        }
        return je;
    }

We could avoid this wasted lookup by letting getVersionedEntry perform the
default lookup instead. There are a couple of other callers
of getVersionedEntry with different fallbacks though, so not quite sure how
to deal with that. A Supplier<JarEntry> might work?

Eirik.


On Sun, Apr 12, 2020 at 9:06 PM Eirik Bjørsnøs <eirbjo at gmail.com> 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.
>