On Mon, 14 Oct 2024 18:17:15 GMT, Chen Liang <li...@openjdk.org> wrote:
>> Eirik Bjørsnøs has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use Arrays.sort instead of TreeSet
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 1831:
>
>> 1829: metaVersions = new HashMap<>();
>> 1830: for (var entry : metaVersionsMap.entrySet()) {
>> 1831: // Convert TreeSet<Integer> to int[] for
>> performance
>
> I think if you just require order upon final freezing, you can just use a
> HashSet or ArrayList, and perform the sorting and deduplication when you
> compile to an int array.
If we trust that versioned entries are rare, this should not matter much either
way. But I pushed a commit which uses HashSet and Arrays::sort on freezing
instead. WDYT?
Given that most versioned entries will only have a single version, we may save
some memory and speed by special-casing for sets with one or two versions:
switch (metaVersionsMap.get(name)) {
case null -> {
// Special-case for set of size 1
metaVersionsMap.put(name, Set.of(version));
}
case Set<Integer> versions when versions.size() == 1 -> {
// Special-case for set of size 2
metaVersionsMap.put(name, Set.of(version, versions.iterator().next()));
}
case Set<Integer> versions when versions.size() == 2 -> {
// Now we convert to HashSet
HashSet<Integer> set = new HashSet<>(versions);
set.add(version);
metaVersionsMap.put(name, set);
}
case HashSet<Integer> versions -> {
// Just add
versions.add(version);
}
default -> throw new AssertionError("Illegal state");
}
But again, versioned entries are rare, so perhaps better to keep it simple.
Does @cl4es have thoughts about this?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21489#discussion_r1800001845
