Hi Lois, On 18 Jun 2015, at 19:55, Lois Foltan <lois.fol...@oracle.com> wrote:
> > On 6/18/2015 10:59 AM, Karen Kinnear wrote: >> Code review for the hotspot component: >> >> Thank you for the cleanups to jvm.cpp to make them JVM_ENTRY and fix >> thread_state. >> >> If you pass the testing and Lois ok's the hotspot code, then I am ok with >> checking in the >> hotspot code as is. > > I have reviewed and am okay with checking in the hotspot code as is once > testing is complete. One comment inlined below. > Lois > >> >> Comments for next round of changes: >> >> 1. mutexLocker.cpp - new lock should say "false" not "true" for whether the >> VMThread should block >> >> 2. os.hpp >> restartable_read_at is added but I don't see it used >> >> 3. imageFile.cpp >> We've been asked to always used the counted form of strcpy - i.e. strncpy >> to ensure we never >> have buffer overruns. e.g. 311, 437, ... >> >> 4. jvm.cpp - seems odd that the interface would need to know if the resource >> was compressed >> or uncompressed. In the next round of API design, seems like the java code >> would pass in >> the uncompressedAddress and let the internals handle buffering the >> compressed data and >> doing the uncompressing transparently. >> >> 5. imageDecompressor.hpp >> get_decompressor >> I think you've mixed your exception handling approaches. You have a >> CHECK_NULL for new_symbol >> call, which means it will throw an exception if it returns NULL. Then you >> check for HAS_PENDING_EXCEPTION >> which is the logic you would use if did not have the CHECK, but then you >> throw the exception away. >> If you want to throw an exception from get_decompressor you should pass in >> the last argument as TRAPS macro >> which will give you the THREAD. >> >> see macros in exceptions.hpp >> >> So the way it is right now, you will throw an exception. >> >> You have a choice - you can throw an exception >> pass in TRAPS to get_decompressor, keep the CHECK_NULL and remove the if >> HAS_PENDING_EXCEPTION. >> - this is the recommended approach >> >> Alternative - change CHECK_NULL to pass in THREAD. Keep the rest. Given you >> are planning to take this >> out of the vm - this would make more sense. > > Whatever mechanism you choose to use in get_decompressor, please also use for > image_decompressor_init( )/createSymbol() within imageDecompressor.cpp as > well. OK, well noted. Thanks. JF > >> >> thanks, >> Karen >> >> >> >> On Jun 17, 2015, at 8:08 PM, Jim Laskey (Oracle) wrote: >> >>> https://bugs.openjdk.java.net/browse/JDK-8080511 >>> >>> This is an long overdue refresh of the jimage support in the JDK9-dev repo. >>> This includes native support for reading jimage files, improved jrt-fs >>> (java runtime file system) support for retrieving modules and packages from >>> the runtime, and improved performance for langtools in the presence of >>> jrt-fs. >>> >>> http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-top >>> <http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-top> >>> http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-jdk >>> <http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-jdk> >>> http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-hotspot >>> <http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-hotspot> >>> http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-langtools >>> <http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-langtools> >>> >>> >>> Details: >>> >>> - jrt-fs provides access, via the nio FileSystem API, to the classes in a >>> .jimage file, organized by module or by package. >>> - Shared code for jimage support converted to native. Currently residing >>> in hotspot, but will migrate to it’s own jdk library >>> https://bugs.openjdk.java.net/browse/JDK-8087181 >>> <https://bugs.openjdk.java.net/browse/JDK-8087181> >>> - A new archive abstraction for class/resource sources. >>> - java based implementation layer for jimage reading to allow backport to >>> JDK8 (jrt-fs.jar - IDE support.) >>> - JNI support for jimage into hotspot. >>> - White box tests written to exercise native jimage support. >>> >
