RFR(XS) 8249096: Clean up code for DumpLoadedClassList

[Yumin Qi]

Hi, Dan

   Thanks for the review. For local tests on jtreg/runtime/cds. I will submit a mach5 test on tier1-4 to make sure there is no regression.


Thanks

Yumin

On 8/21/20 12:18 PM, Daniel D. Daugherty wrote:
> On 8/20/20 7:28 PM, Yumin Qi wrote:
>> Hi, Ioi
>>
>>   updated at new link: http://cr.openjdk.java.net/~minqi/2020/8249096/webrev-02/
>
> > Summary of changes:    108 lines changed: 54 ins; 51 del; 3 mod; 14941 unchg
>
> At >100 lines changed I wouldn't call this an XS review and it definitely
> does not qualify as Trivial.
>
>
> src/hotspot/share/oops/instanceKlass.hpp
>     No comments.
>
> src/hotspot/share/classfile/classFileParser.cpp
>     No comments.
>
> src/hotspot/share/classfile/systemDictionary.cpp
>     No comments.
>
> src/hotspot/share/oops/instanceKlass.cpp
>     InstanceKlass::log_to_classlist() appears to be an equivalent
>     rewrite of the big logic block removed from classFileParser.cpp,
>     but that's really the call of someone that knows CDS better than
>     I do. Ioi has already said it's good so...
>
>
> Thumbs up.
>
> What testing was done on the webrev-02 version?
>
> Dan
>
>>
>>   Moved the call to 'log_to_classlist' to beginning of 'print_class_load_logging'. Removed the check for if class+load logging is enabled, since it will by pass the output for DumpLoadedClassList if not enabled.
>>
>>   In 'print_class_load_logging', there exists a check for if the log is enabled.
>>
>> Thanks
>>
>> Yumin
>>
>>
>> On 8/20/20 1:57 PM, Yumin Qi wrote:
>>> Ioi,
>>>
>>>   Thanks for the review, I will update with new webrev --- yes, print_class_load_logging will work the same way.
>>>
>>>
>>> Thanks
>>>
>>> YUmin
>>>
>>> On 8/20/20 11:37 AM, Ioi Lam wrote:
>>>> Hi Yumin,
>>>>
>>>> This looks like a good clean up. I think it can be further simplified by moving the call to InstanceKlass::log_to_classlist to beginning of InstanceKlass::print_class_load_logging().
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>> On 8/20/20 10:46 AM, Yumin Qi wrote:
>>>>> Hi, Please review the very small change for cleaning up DumpLoadedClassList code.
>>>>>
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8249096
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~minqi/2020/8249096/webrev-01/
>>>>>
>>>>>
>>>>> There are two places where when DumpLoadedClassList is set we log loaded class to log file, one is in ClassFileParser::parse_stream, and the other is in SystemDictionaryShared::load_shared_class_misc which is after shared class loaded from CDS. The former is positioned after parsing constants, and before parsing interface, fields and methods etc. There is possibility an exception or error happens before the InstanceKlass is created, so the fix move the logging to after the InstanceKlass is successfully created, also move the logging code to InstanceKlass which seems more reasonable.
>>>>>
>>>>>
>>>>> Test: local jtreg on cds.
>>>>>
>>>>> Mach5 tier1-4
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>> Yumin
>>>>>
>>>>
>