[13] RFR(L) 8220623: [JVMCI] Update JVMCI to support JVMCI based Compiler compiled into shared library

[Kim Barrett]

> On Apr 4, 2019, at 7:45 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> Thank you, Kim
> 
> I moved HotSpotJVMCI methods which call JNIHandles::resolve() to jvmciJavaClasses.cpp file so that I don't need to include jniHandles.inline.hpp into .hpp file.
> 
> I will update changes when I address your other comments.

Here are a few more comments.

I really want to better understand the MetadataHandle stuff, but I
don't think I will be able to seriously dig into it for a while. I'm
not sanguine about what seems to be yet another weak reference
mechanism.

------------------------------------------------------------------------------
src/hotspot/share/prims/jvmtiTagMap.cpp
3046   blk.set_kind(JVMTI_HEAP_REFERENCE_OTHER);
3047   Universe::oops_do(&blk);
3048 
3049 #if INCLUDE_JVMCI
3050   blk.set_kind(JVMTI_HEAP_REFERENCE_OTHER);
3051   JVMCI::oops_do(&blk);
3052   if (blk.stopped()) {
3053     return false;
3054   }
3055 #endif

(New code starts with line 3049.)

There should probably be a blk.stopped() check after the call to
Universe::oops_do.  This seems like a pre-existing bug, made more
apparent by the addition of the JVMCI code.

------------------------------------------------------------------------------
src/hotspot/share/classfile/classFileParser.cpp
5634   if (!is_internal()) {
5635     bool trace_class_loading = log_is_enabled(Info, class, load);
5636 #if INCLUDE_JVMCI
5637     bool trace_loading_cause = TraceClassLoadingCause != NULL &&
5638         (strcmp(TraceClassLoadingCause, "*") == 0 ||
5639          strstr(ik->external_name(), TraceClassLoadingCause) != NULL);
5640     trace_class_loading = trace_class_loading || trace_loading_cause;
5641 #endif
5642     if (trace_class_loading) {
5643       ResourceMark rm;
5644       const char* module_name = (module_entry->name() == NULL) ? UNNAMED_MODULE : module_entry->name()->as_C_string();
5645       ik->print_class_load_logging(_loader_data, module_name, _stream);
5646 #if INCLUDE_JVMCI
5647       if (trace_loading_cause) {
5648         JavaThread::current()->print_stack_on(tty);
5649       }
5650 #endif

This appears to be attempting to force a call to
print_class_load_logging if either log_is_enabled(Info, class, load)
is true or if TraceClassLoadingCause triggers it.  But
print_class_load_logging does nothing if that logging is not enabled,
with the result that if it isn't enabled, but the tracing option
matches, we'll get a mysterious stack trace printed and nothing else.
I suspect that isn't the desired behavior.

------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciRuntime.cpp
 135   // The following instance variables are only used by the first block in a chain.
 136   // Having two types of blocks complicates the code and the space overhead is negligible.
 137   MetadataHandleBlock* _last;                   // Last block in use
 138   intptr_t        _free_list;                   // Handle free list
 139   int             _allocate_before_rebuild;     // Number of blocks to allocate before rebuilding free list

There seems to be exactly one MetadataHandleBlock chain, in
_metadata_handles. So these instance variables could be static class
variables.

If there was more than one list, I think a better approach would have
a MetadataHandleBlockList class that had those members and a pointer
to the first block in the list. This code seems to have been pretty
much copy-paste-modified from JNIHandleBlock, which has somewhat
different requirements and usage pattern.

------------------------------------------------------------------------------