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