[13] RFR(L) 8220623: [JVMCI] Update JVMCI to support JVMCI based Compiler compiled into shared library
Vladimir Kozlov
vladimir.kozlov at oracle.com
Sat Apr 6 00:58:34 UTC 2019
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
More information about the graal-dev
mailing list