RFR: 8194759: Support caching class mirror objects

[Ioi Lam]

On 2/26/18 3:21 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
> Thanks for reviewing the update!
>
>> On Feb 26, 2018, at 2:25 PM, Ioi Lam <ioi.lam at oracle.com 
>> <mailto: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.

OK, how about renaming it to "is_signed_when_dumping"? Then it's clear 
from the name that the function won't return true at runtime even when 
the class is signed.
>
>>
>>
>> 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.
>
Sounds good. Thanks
- Ioi
> 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/%7Ejiangli/8194759/webrev.01/> 
>>> <http://cr.openjdk.java.net/~jiangli/8194759/webrev.01/ 
>>> <http://cr.openjdk.java.net/%7Ejiangli/8194759/webrev.01/>>
>>>
>>> Incremental webrev:
>>> http://cr.openjdk.java.net/~jiangli/8194759/webrev.01.inc/ 
>>> <http://cr.openjdk.java.net/%7Ejiangli/8194759/webrev.01.inc/> 
>>> <http://cr.openjdk.java.net/~jiangli/8194759/webrev.01.inc/ 
>>> <http://cr.openjdk.java.net/%7Ejiangli/8194759/webrev.01.inc/>>
>>>
>>> Rerunning tiered testing ...
>>>
>>> Thanks,
>>> Jiangli
>>>
>>
>