RFR: 8194759: Support caching class mirror objects

Jiangli Zhou jiangli.zhou at oracle.com
Mon Feb 26 23:21:30 UTC 2018


Hi Ioi,

Thanks for reviewing the update!

> On Feb 26, 2018, at 2:25 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> Hi Jiangli,
> 
> The changes look good. I have a few style suggestions:
> 
> 1.
> 
> Is the "classpath_index >= _app_paths_start_index" necessary in the following block? I think it's very rare that we would have signed classes in the bootappend JAR files, so removing this check would make the code simpler.
> 
>     // Ignore any App classes from signed JAR file during CDS archiving
>     // dumping
>     if (DumpSharedSpaces &&
> SharedClassUtil::is_classpath_entry_signed(classpath_index) &&
>         classpath_index >= _app_paths_start_index) {
>       tty->print_cr("Preload Warning: Skipping %s from signed JAR",
>                     context->class_name());
>       return false;
>     }

This is the dead code path that I intended to remove but added back in the updated webrev per your request. :-) I filed JDK-8198447 <https://bugs.openjdk.java.net/browse/JDK-8198447> and will handle it separately.

> 
> 
> 2.
> 
> For has_signer_and_not_archived -- I think it's still not clear that this function applies only during dumping.
> 
> How about always setting this flag (during run time and dump time), then it can be renamed simply as "has_signers()". This function would work both at run time and dump time (although there may not be a use case for this function during normal run time …).

Let’s keep has_signer_and_not_archived as is until there are other use case for it.

> 
> 
> 3.
> 
> I think you should move the THREAD_FIELDS_DO macro into javaClasses.hpp and use that to declare the fields for Thread as well. (Same for the other classes).
> 
> This would avoid code duplication. Also, this means you can't declare a new field without adding it to the THREAD_FIELDS_DO list.
> 
> #define THREAD_FIELDS_DO(macro) \
>   macro(_name_offset,          k, vmSymbols::name_name(), string_signature, false); \
>   macro(_group_offset,         k, vmSymbols::group_name(), threadgroup_signature, false); \
>   macro(_contextClassLoader_offset, k, vmSymbols::contextClassLoader_name(), classloader_signature, false); \
>   macro(_inheritedAccessControlContext_offset, k, vmSymbols::inheritedAccessControlContext_name(), accesscontrolcontext_signature, false); \
>   macro(_priority_offset,      k, vmSymbols::priority_name(), int_signature, false); \
>   macro(_daemon_offset,        k, vmSymbols::daemon_name(), bool_signature, false); \
>   macro(_eetop_offset,         k, "eetop", long_signature, false); \
>   macro(_stillborn_offset,     k, "stillborn", bool_signature, false); \
>   macro(_stackSize_offset,     k, "stackSize", long_signature, false); \
>   macro(_tid_offset,           k, "tid", long_signature, false); \
>   macro(_thread_status_offset, k, "threadStatus", int_signature, false); \
>   macro(_park_blocker_offset,  k, "parkBlocker", object_signature, false); \
>   macro(_park_event_offset,    k, "nativeParkEventPointer", long_signature, false)
> 
> class java_lang_Thread : AllStatic {
>  private:
>   // Note that for this class the layout changed between JDK1.and JDK1.3,
>   // so we compute the offsets at startup rather than hard-wiring them.
>   static int _name_offset;
>   static int _group_offset;
>   static int _contextClassLoader_offset;
>   static int _inheritedAccessControlContext_offset;
>   static int _priority_offset;
>   static int _eetop_offset;
>   static int _daemon_offset;
>   static int _stillborn_offset;
>   static int _stackSize_offset;
>   static int _tid_offset;
>   static int _thread_status_offset;
>   static int _park_blocker_offset;
>   static int _park_event_offset ;

Actually, I played that idea when I made the macro changes and decided against it. Modifying the field declarations in javaClasses.hpp are beyond the scope of current change and adds additional risks. It deserves its own RFE. I will file a follow up RFE for the field declarations.

Thanks,
Jiangli

> 
> Thanks
> 
> - Ioi
> 
> 
> On 2/22/18 10:24 PM, Jiangli Zhou wrote:
>> Here is the new webrev with updates that address feedbacks from Misha, Ioi, Coleen and Thomas:
>> 
>> http://cr.openjdk.java.net/~jiangli/8194759/webrev.01/ <http://cr.openjdk.java.net/~jiangli/8194759/webrev.01/>
>> 
>> Incremental webrev:
>> http://cr.openjdk.java.net/~jiangli/8194759/webrev.01.inc/ <http://cr.openjdk.java.net/~jiangli/8194759/webrev.01.inc/>
>> 
>> Rerunning tiered testing ...
>> 
>> Thanks,
>> Jiangli
>> 
> 



More information about the hotspot-runtime-dev mailing list