Hi Karen, On 18 Jun 2015, at 16:59, Karen Kinnear <karen.kinn...@oracle.com> wrote:
> Code review for the hotspot component: > > Thank you for the cleanups to jvm.cpp to make them JVM_ENTRY and fix > thread_state. > You are welcome. > If you pass the testing and Lois ok's the hotspot code, then I am ok with > checking in the > hotspot code as is. > Thanks. > Comments for next round of changes: > > 1. mutexLocker.cpp - new lock should say "false" not "true" for whether the > VMThread should block Done, along with not-leaf. > > 2. os.hpp > restartable_read_at is added but I don't see it used Removed. > > 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. > Thanks, we will implement 3/4/5 in the next round. > 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. >> >
