8375175: Improve lookup efficiency and maintainability in ImageReader #1896
Conversation
|
👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| public int getLocationIndex(String name) { | ||
| public int getLocationIndex(String name, int startIndex) { | ||
| int count = header.getTableLength(); | ||
| int index = redirect.get(ImageStringsReader.hashCode(name) % count); |
There was a problem hiding this comment.
The "index" variable is never actually an index. It's either the bit-flipped index, or a hash seed. So the name was never really correct.
| return attributes; | ||
| } | ||
|
|
||
| private static long readValue(int length, ByteBuffer buffer, int offset, int limit) { |
There was a problem hiding this comment.
This is just moved close to where it's used so it's no longer in-between two very similar verify() methods.
| strings); | ||
| } | ||
|
|
||
| static boolean verify(String module, String name, ByteBuffer locations, |
| moduleOffset, parentOffset, baseOffset, extOffset, strings); | ||
| } | ||
|
|
||
| private static long readValue(int length, ByteBuffer buffer, int offset, int limit) { |
| } | ||
| Objects.requireNonNull(path); | ||
|
|
||
| private static boolean verifyName(String module, String name, int index, int length, |
There was a problem hiding this comment.
Reduced the complexity and chance for accidental bad parameter passing by just passing in the attributes array reference. Less than half as many parameters now.
| return false; | ||
| } | ||
| } | ||
| private static boolean verifyPackagePath(String path, int index, long[] attributes, ImageStrings strings) { |
There was a problem hiding this comment.
The string being tested here is no longer a "name" string (it wasn't always a name string before either).
It's really the resource path (the trailing part of a jimage name).
| // Returns null for non-directory resources, since the jimage name does not | ||
| // start with "/modules" (e.g. "/java.base/java/lang/Object.class"). | ||
| ImageLocation loc = findLocation(name); | ||
| // Look for non-directory resources first as they are much more common. |
There was a problem hiding this comment.
This is the logic switch to try the resource name first (lookups for entries are most likely be for resources).
|
|
||
| public static int hashCode(String s) { | ||
| return hashCode(s, HASH_MULTIPLIER); | ||
| /** |
There was a problem hiding this comment.
Just simple plumbing and some better docs.
david-beaumont
changed the title
1 participant
The main aim of this change is to get an additional "int startIndex" parameter plumbed into the hashcode and verification methods of ImageReader and associated classes.
However, one of the issues with this is that now the
hashCode(String, int)method which existed before becomes ambiguous in its signature and prone to accidental misuse. There the "int" parameter is the explicitly specified seed, not the start index.To mitigate this, I split the methods into two groups, "defaultHashCode" and "seededHashCode". This helps disambiguate call sites and improves readability, but does mean a few additional classes are affected.
Also, in existing methods with a local variable called "index" which now also have a "startIndex", I renamed the existing variable to be move specific, to improve readability and reduce the risk of confusion (e.g. "index" -> "locIndex").
The main logic change regarding lookup ordering is in:
src/java.base/share/classes/jdk/internal/jimage/ImageReader.java
I also added documentation to many of the methods for future maintainers, and removed more dead code I found.
The code here is already very well tested by unit tests (ImageReaderTest and SystemImageTest are quite thorough).
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1896/head:pull/1896$ git checkout pull/1896Update a local copy of the PR:
$ git checkout pull/1896$ git pull https://git.openjdk.org/valhalla.git pull/1896/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1896View PR using the GUI difftool:
$ git pr show -t 1896Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1896.diff