Initial webrev with changes for JDK 9 - jimage

[Roger Riggs]

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