8342040: Further improve entry lookup performance for multi-release JARs #21489
Conversation
|
👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into |
|
@eirbjo This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 149 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
added
security
|
Performance results: Baseline: PR: |
Webrevs
|
| if (metaVersionsMap != null) { | ||
| metaVersions = new HashMap<>(); | ||
| for (var entry : metaVersionsMap.entrySet()) { | ||
| // Convert TreeSet<Integer> to int[] for performance |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I like simple.
Mixing Set.of and HashSet means a lot of semantic fu-fu (throwing/not throwing on nulls) and risks being suboptimal due making some call sites megamorphic.
There was a problem hiding this comment.
Yep, looks good. And I agree with Claes' evaluation that we should not complicate this construction further, especially that the result is quickly garbage collected when we compile into the string to int array map.
There was a problem hiding this comment.
We could also just use Map<String, int[]>. This would allow us to skip the whole Set<Integer> to int[] transition step, and the temporary HashMap is no longer needed.
Deduplication could occur during registration:
metaVersions.merge(name, new int[] {version}, (current, val) -> {
// Check duplicates
for (int v : current) {
if (v == version) {
return current;
}
}
// Add version
int[] merged = Arrays.copyOf(current, current.length + 1);
merged[merged.length - 1] = version;
return merged;
});while the postprocessing step would simply sort each array:
for (int[] versions : metaVersions.values()) {
// JarFile::getVersionedEntry expects sorted versions
Arrays.sort(versions);
}This performs ~10% better on a version-heavy ZipFileOpen, and I think the code is reasonably simple compared to the current state of the PR. Thoughts?
There was a problem hiding this comment.
I suspect remaining regression is mostly from the added
Stringcreation/processing ininitCEN.
Yes, there is a cost with the String construction when parsing the name out of META-INF/versions/{name}
There was a problem hiding this comment.
There are a few possible strategies for avoiding that additional parse, since the effect we're getting at here is to have a quick filter to avoid pointless lookups and not necessarily an exact mapping.
One is to store the checkedHash rather than the full String. This gets openCloseZipFile down to ~910000 ns/op. (checkedHash very hot in profiles). There's still a chance for redundant lookups on hash collisions, but this should be rare.
Another is to store a BitSet per name length. This gets ZipFileOpen down to baseline level (~670000 ns/op), but increases chance of having to do redundant lookups a lot.
Both also improves footprint (not keeping each versioned entry String in memory would be nice).
There was a problem hiding this comment.
One is to store the
checkedHashrather than the fullString. This getsopenCloseZipFiledown to ~910000 ns/op. (checkedHashvery hot in profiles). There's still a chance for redundant lookups on hash collisions, but this should be rare.
Seems like a resonable trade-off. Could you take a look at the latest 771488e and see if that represents your suggestion here?
There was a problem hiding this comment.
Yes, 771488e matches my quick experiment (sans some cleanups you've made). I agree this variant makes for a reasonable trade-off.
There was a problem hiding this comment.
Maybe there's some benefit in detecting "META-INF/versions/" in the checkAndAddEntry method and then calculate the two different hash values back to back. Hash calculations are fast on modern hardware with vector support, but the memory accesses might not be so we might get a general win from re-arranging some of this stuff. I think this is more suitable for a follow-up experiment, though.
|
Marking this PR as draft while investigating alternatives as proposed by @cl4es |
Use BitSet to streamline construction
|
|
…oids String allocation and storage
|
/contributor add cl4es |
|
@eirbjo Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
/contributor add @cl4es |
|
@eirbjo |
|
Bringing this back from draft mode after some feedback and exploration with @cl4es The current version of this PR now uses a BitSet to represent versions. This speeds up version tracking in To summarize the performance benefits of this PR over baseline:
Claes has approved this PR already, but since he contributed code and ideas to this PR, I'll let this linger until early next week to allow non-Scandinavian reviewers to have a look :-) |
| // Register version for this hash code | ||
| if (metaVersions == null) | ||
| metaVersions = new HashMap<>(); | ||
| metaVersions.computeIfAbsent(hashCode, _ -> new BitSet()).set(version); |
There was a problem hiding this comment.
Does this hashCode ever collide? Tried reading ZipCoder and can't rule that out. If yes, we need a fallback mechanism if this computeIfAbsent encountered an existing entry.
There was a problem hiding this comment.
Yes it may collide. This is an overapproximation of the version set where a hash collision just results in an extra lookup being performed for a version entry which does not exist for that name. See the comment on the metaVersions field declaration.
This is a trade-off against the initial alternative proposed in this PR which was to map using the entry name string. That would minimize lookups, but it would also require allocating the String name and storing those strings in the map. So we decided a few extra lookups on hash collisions was an okay trade-off for faster parsing and less memory used.
In fact, if we a use constant hash code like '1', such that all hash codes collide, we get the behavior of the current mainline, where a search for any name will result in searches being performed for each version discovered in the ZIP.
Please review this PR which speeds up
JarFile::getEntrylookup significantly for multi-release JAR files.The changes in this PR are motivated by the following insights:
META-INF/versions/is sparsely populated.module-info.classfrom Java 8.Instead of performing one lookup for every version identified in the JAR, this PR narrows the version search down to an approximation of the number of versions found for the entry being looked up, which will often be zero. This speeds up lookup for non-versioned entries, and provides a more targeted search for versioned entries.
An alternative approach could be to normalize the hash code to use the none-versioned name such that versioned and non-versioned names would be resolved in the same lookup. This was quickly abandoned since the code changes were intrusive and mixed too many JAR specific concerns into
ZipFile.Testing: The existing
JarFileGetEntrybenchmark is updated to optionally test a multi-release JAR file with one versioned entry formodule-info.classplus two other versioned class files for two distinct versions. Performance results in first comment.Running
ZipFileOpenon a multi-release JAR did not show a significat difference between this PR and mainline.The JAR and ZIP tests are run locally. GHA results green. The
noreg-perflabel is added in JBS.Progress
Issue
Reviewers
Contributors
<redestad@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21489/head:pull/21489$ git checkout pull/21489Update a local copy of the PR:
$ git checkout pull/21489$ git pull https://git.openjdk.org/jdk.git pull/21489/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21489View PR using the GUI difftool:
$ git pr show -t 21489Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21489.diff
Webrev
Link to Webrev Comment