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

[Vladimir Kozlov]

Thank you, Kim

On 4/5/19 4:56 PM, Kim Barrett wrote:
>> 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.

Yes, I was also puzzled about that. I will add check.

> 
> ------------------------------------------------------------------------------
> 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.

I will fix it.

> 
> ------------------------------------------------------------------------------
> 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.

Agree.

> 
> 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.

It is only one list. I don't want to complicate it.

Thanks,
Vladimir