We rework the VM initialization such that lambdas can be used very early after
phase 1 initialization is done and before module system initialization.
I’m not sure where jimage uses of lambdas for this case. In general, we wanted
to enable use of new language features early during startup and we have this
solve in jake (startup performance is a different topic).
Mandy
> On Mar 11, 2016, at 6:27 AM, Roger Riggs <roger.ri...@oracle.com> wrote:
>
> Thanks Jim.
>
> Beware of use of Lambdas, jimage may be used early enough in the boot cycle
> that lambdas
> are not yet fully operative. The failure is pretty obvious but can be trial
> and error to narrow it down.
>
> It would be useful to create an jira entry for the changes that are being
> delayed.
>
> Roger
>
>
> On 3/10/2016 6:22 PM, Jim Laskey (Oracle) wrote:
>> Thank you Roger. Comments inline. In general, I will hold off changes
>> until after merge so as not to destabilize. Note that ImageBufferCache and
>> Decompression are not currently used.
>>
>>> On Mar 8, 2016, at 2:07 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
>>>
>>> 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.
>> Noted.
>>
>>> 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.
>> Replaced with specific exceptions.
>>
>>> - findLocation() should use a more explicit computation for the redirection
>>> than rehashing; this would allow the hash seed to be encapsulated.
>> As part of the review change set, I've documented the Minimal Perfect Hash
>> Method (see PerfectHashBuilder.) The algorithm requires a one time one
>> rehash when there is a collision. A new seed forces a different
>> distribution of the colliding strings. If you started with the existing
>> hashCode, you would not get a new distribution, merely a shift of all the
>> colliding elements. Random distribution is the essence of algorithm.
>>
>>> - slice() synchronizes on the ByteBuffer; does every other access?
>> Byte buffer use here is immutable, however slicing of the ByteBuffer does
>> require modifying position and limit. Wrapping or cloning the buffers would
>> be of little benefit and would take a performance hit. The ByteBuffer API
>> might have been well served with an atomic non-modifying slice operation.
>>
>>> - getAllLocations() is unused and does not respect its sorted argument;
>>> Best to remove the method for now.
>> Removed.
>>
>>> - getResourceBuffer(): unused - remove
>> Removed.
>>
>>> - getResourceStream(): unused - remove
>> Removed.
>>
>>> ImageStringsReader:
>>>
>>> - Is Modified UTF-8 really needed here; using normal UTF-8 would allow
>>> re-use and better performance. Alternatively, Modified UTF-8 should be
>>> added as a supported encoding.
>> Modified UTF-8 is used to be more compact and to simplify use in native
>> code. The jimage string table has overlapping use with constant pool
>> compaction so is really an older revision of the Modified UTF-8 standard.
>> Not sure why Modifed UTF-8 is not offered as an encoding. It would have
>> made constant pool management easier. And note that relying on a particular
>> encoding is problematic. Encoding standards do change, affecting the
>> stability of the hash algorithm and may prevent the reading of older jimage
>> files.
>>
>>> - 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.
>>>
>> Reasonable and will consider. Not sure what you mean by stable across
>> revisions. This is not string hashCode. This is a hashCode that is specific
>> to MPHM and is deterministic.
>>
>>> - stringFromMUTF8(byte[]) - there may be some optimization to avoid
>>> expanding ascii to chars and then re-encoding in String.
>> Will touch base with the Aleksey.
>>
>>> - charsFromByteBufferLength/charsFromByteBuffer should throw an exception
>>> if no terminating null, not just when assertions are enabled
>> Will throw InternalError instead.
>>
>>> - inconsistent assert errors vs exceptions for malformed mutf8.
>>> Use of assert will not detect errors in production builds
>> Will throw InternalError instead.
>>
>>> - mutf8FromChars allocates a new working buffer (byte[8]) potentially for
>>> every UNICODE char.
>> Will hoist the buffer allocation and reuse.
>>
>>> - mutf8FromString: should take advantage of new String(bytes, offset,
>>> length, charsetName) to avoid extra allocations.
>>> (would need a Modified utf-8 charset).
>> Modified UTF-8 not available.
>>
>>> - hashCode(byte[0, seed) should be private; the hashcode algorithm should
>>> not be variable.
>>>
>> This is not String hashCode. see above.
>>
>>> ImageLocation:
>>> - Merge ImageLocation functions into ImageLocationBase and rename.
>>> ImageLocation does not have enough unique functionality to be a separate
>>> class.
>> Merged.
>>
>>> - 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
>>>
>> Noted.
>>
>>> ImageLocationBase:
>>> - Failing asserts will not cause a runtime error in production.
>>> But will degenerate into unpredictable other exceptions
>> Will throw InternalError instead.
>>
>>> ImageStream:
>>> - compress(): Too heavyweight an object to be used and discarded just for
>>> compress / decompress of small values.
>> compress()? I don’t see ImageStream used anywhere except for large
>> buffers. As a note, dynamic ByteBuffers really should be part of the JDK.
>>
>>>
>>> ImageBufferCache:
>>> - Comments on each method would make the intent clear
>> Will add comments.
>>
>>> - 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.
>> Rewrote using lambdas to clarify the algorithm.
>>
>>> - 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.
>>>
>> Modified.
>>
>>> ImageHeader:
>>> - BADMAGIC is not used
>> Removed.
>>
>>> - readFrom could throw BufferUnderflowException - undocumented
>> get(i) can IndexOutOfBoundsException [and get() BufferUnderflowException]
>> Will add explicit check.
>>
>>
>>> ImageModuleData:
>>> - ImageModuleData seems to be unused; allModuleNames() is unreferenced
>>>
>> Removed.
>>
>>> ImageReader:
>>> - ImageReader(path): unused - remove
>> Removed.
>>
>>> - Use of assert is non-reassuring, ineffective in production builds
>> Added explicit check.
>>
>>> - 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.
>> The core libs team was concerned about the performance of class loading.
>> The pattern used here is the one followed by NIO. We will probably come up
>> with a shared NIO cache at some point.
>>
>>> Also, why should there be more than one buffer per thread?
>> The image cache is only used by 32 bit systems and decompression. It is
>> possible for a channel read buffer and a decompression buffer to be
>> allocated at the same time.
>>
>>> ImageReader.Resource:
>>> - unused create method
>> Removed.
>>
>>> - unused parent argument (0) in Resource constructor (dir, loc, attr)
>> Removed.
>>
>>> ImageReader.LinkNode:
>>> - constructor has unused parent arg
>>>
>>>
>> Removed
>>
>>> 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.
>> Removed all allocation.
>>
>>> - decompress is hugely wasteful of allocations of ByteBuffers of small
>>> sizes.
>>>
>> Removed all allocation.
>>
>>
>>> 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
>>>
>> I think decompression needs significant reworking..
>>
>>> 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
>>>
>> Noted.
>>
>
