Hi,
Review comments for the jimage code in java.base, mostly cleanup but
perhaps a bug or two.
General: inconsistent style and not following guidelines, copyright
dates may need an update.
Use of assert for error checking is not going to catch or help isolate
errors in production builds.
dir: jdk/src/java.base/share/classes/jdk/internal/jimage/
BasicImageReader:
- readHeader() exception message should say not a jimage or wrong version
Other IOExceptions can occur if the file is corrupted or is too small,
array index out of bounds, etc.
- findLocation() should use a more explicit computation for the redirection
than rehashing; this would allow the hash seed to be encapsulated.
- slice() synchronizes on the ByteBuffer; does every other access?
- getAllLocations() is unused and does not respect its sorted argument;
Best to remove the method for now.
- getResourceBuffer(): unused - remove
- getResourceStream(): unused - remove
ImageStringsReader:
- Is Modified UTF-8 really needed here; using normal UTF-8 would allow
re-use and better performance. Alternatively, Modifed UTF-8 should be
added as a supported encoding.
- The ImageStringsReader.hashCode() should be defined so it can take
advantage of 32 or 64 bit accesses; since it is used frequently. And it
needs to be stable across revisions that may be accessing jimages from
different JDK versions.
- stringFromMUTF8(byte[]) - there may be some optimization to avoid
expanding ascii to chars and then re-encoding in String.
- charsFromByteBufferLength/charsFromByteBuffer should throw an
exception if no terminating null, not just when assertions are enabled
- inconsistent assert errors vs exceptions for malformed mutf8.
Use of assert will not detect errors in production builds
- mutf8FromChars allocates a new working buffer (byte[8]) potentially
for every UNICODE char.
- mutf8FromString: should take advantage of new String(bytes, offset,
length, charsetName) to avoid extra allocations.
(would need a Modified utf-8 charset).
- hashCode(byte[0, seed) should be private; the hashcode algorithm
should not be variable.
ImageLocation:
- Merge ImageLocation functions into ImageLocationBase and rename.
ImageLocation does not have enough unique functionality to be a
separate class.
- It is a poor design to create an instance that may not be valid.
It would be better to have an ImageLocation factory method that
encapsulated the creation and checking
ImageLocationBase:
- Failing asserts will not cause a runtime error in production.
But will degenerate into unpredictable other exceptions
ImageStream:
- compress(): Too heavyweight an object to be used and discarded just
for compress / decompress of small values.
ImageBufferCache:
- Comments on each method would make the intent clear
- getBuffer: mixing for loop and explicit indexes is fragile
- releaseBuffer: check buffer.capacity before threadLocal.get() for
early return
- Logic of i,j is flawed, i is never changed; j is/becomes 0
The first buffer is removed if there are more than max
Its not clear what algorithm was intended.
A buffer that is in use can be removed.
- ImageBufferCache constructor: the isUsed flag should be set in
getBuffer()
when it is added to the list (not in the constructor)
It keeps it consistent with when it is set for a re-used buffer.
ImageHeader:
- BADMAGIC is not used
- readFrom could throw BufferUnderflowException - undocumented
ImageModuleData:
- ImageModuleData seems to be unused; allModuleNames() is unreferenced
ImageReader:
- ImageReader(path): unused - remove
- Use of assert is non-reassuring, ineffective in production builds
- In a long running application the per thread buffers will get to the
point wehere they are unused and should be released.
There is no support for soft or weak references to handle that case.
Alternatively, if the buffers were not per thread then they would
have a lesser impact.
Also, why should there be more than one buffer per thread?
ImageReader.Resource:
- unused create method
- unused parent argument (0) in Resource constructor (dir, loc, attr)
ImageReader.LinkNode:
- constructor has unused parent arg
decompress/CompressIndexes:
- readInt: wasteful allocation of a single byte array and then another
small byte array;
and then another heap buffer allocation in the ByteBuffer.
- decompress is hugely wasteful of allocations of ByteBuffers of small
sizes.
decompress/CompressedResourceHeader:
- MAGIC does not need to be public
decompress/Decompressor:
- IOException "Plugin name not found" should include the missing name
- StringsProvider is an unnecessary interface; could use Supplier<String>
- Redundant public modifiers in interface declarations
decompress/SignatureParser:
- Comments describing the input and output formats needed
- style around operators does not consistently include spaces; and "if("...
decompress/StringSharingDecompressor:
- Wasteful use of ByteBuffers and small arrays without re-use (safeAdd...)
- StringSharingDecompressor constructor with unused argument - remove
decompress/ZipDecompressor:
- Should be more specific about the zip algorithms supported
since it needs to be stable across releases
- decompress(): use try-with-resources for safer inflation
src/java.base/share/native/libjimage/imageDecompressor.cpp
- "// If ptr is not aligned, sparc will fail." implies input argument
must be aligned
- General doubt about using assert; for example to check on malloc
allocation failure
- In production environment will exhibit as segv
