RFR: 8078644: CDS needs to support JVMTI CFLH

[Daniel D. Daugherty]

On 9/6/16 6:14 PM, Jiangli Zhou wrote:
> Hi,
>
> Please review the following change thats support JVMTI class_file_load_hook(CFLH) during initial loading of shared classes. The webrev also removes the temporary workaround that disables CDS when JVMTI CFLH is required (JDK-8141341 <https://bugs.openjdk.java.net/browse/JDK-8141341>).
>
> 	webrev:http://cr.openjdk.java.net/~jiangli/8078644/webrev.00/

General comment

- please make sure all copyright years are updated.
- there are some jcheck space at end-of-line violations
   (There are more than what I flagged in the comments below).

- I think there might be a data leak with the _cached_class_file
   field, but that pre-dates your changes and I have to think about it.


src/share/vm/oops/instanceKlass.hpp
     No comments.

src/share/vm/oops/instanceKlass.cpp
     L2119:   // deallocate the cached class file
     L2120:   if (_cached_class_file != NULL) {
     L2121:     os::free(_cached_class_file);
     L2122:     _cached_class_file = NULL;
     L2123:   }
         Doesn't this use of _cached_class_file need to be checked
         against the shared spaces? I don't think os::free() will
         be happy if _cached_class_file points into shared space.

src/share/vm/memory/metaspaceShared.hpp
     No comments.

src/share/vm/memory/metaspaceShared.cpp
     No comments.

src/share/vm/memory/metaspace.cpp
     No comments.

src/share/vm/classfile/klassFactory.hpp
     No comments.

src/share/vm/classfile/klassFactory.cpp
     L219:     if ((result->get_cached_class_file()) != NULL) {
     L220:       // JFR might set cached_class_file
     L221:       len = result->get_cached_class_file_len();
     L222:       bytes = result->get_cached_class_file_bytes();
     L223:     } else {
         Perhaps:
               if ((bytes = result->get_cached_class_file()) != NULL) {
                 // event based tracing might set cached_class_file
                 len = result->get_cached_class_file_len();
               } else {

         so two changes:
           - mv init of bytes into the if-statement
           - change 'JFR' -> 'event based tracing'

src/share/vm/classfile/systemDictionary.cpp
     No comments.

src/share/vm/memory/filemap.hpp
     L260:   }
     L261:   // The estimated optional space size. Only the portion 
containning data is

         Please add a blank line between L260 and L261.

         Typo: 'containning' -> 'containing'

         There's a trailing space on L261; jcheck won't like it.

     L266:   }
     L267:   // Total shared_spaces size includes the ro, rw, md, mc and 
od spaces

         Please add a blank line between L266 and L267.

     L264:   static size_t optional_space_size() {
     L265:     return core_spaces_size();

         It is not clear why core_spaces_size() is being returned as
         an estimate of the optional space size.

         There's a trailing space on L265; jcheck won't like it.

src/share/vm/memory/filemap.cpp
     No comments.

src/share/vm/utilities/debug.hpp
     No comments.

src/share/vm/utilities/debug.cpp
     L289:     "shared miscellaneous code space",
     L290:   };
         The name string for the new space is missing.
         Perhaps "shared optional space"...

Dan


>          bug: JDK-8078644 <https://bugs.openjdk.java.net/browse/JDK-8078644>
>
> Class file data is needed when posting CFLH. For shared classes, the class file data are now archived at dump time. That avoids re-reading the class file data from the JAR file or reconstituting the data at runtime, which would add class loading overhead for shared classes.
>
> To minimize the runtime memory overhead, the archived class file data are stored in a separate shared space, called ‘optional data space’ (od).  The ‘od’ space a new region in the archive. It does not increase runtime memory usage when JvmtiExport::should_post_class_file_load_hook() is false. The memory contains the archived class file data is only paged in when the VM needs to post CFLH.  The ‘od’ space can be shared by different processes.
>
> When loading shared class at runtime, we now call JvmtiExport::post_class_file_load_hook() with the archive class file data if needed. If JVMTI agent modifies the class, new InstanceKlass is created using the modified class data and the archived class is ignored.
>
> Tested with JPRT, CDS tests and QA tests.
>
> Thanks,
> Jiangli