RFR(XS) 8249096: Clean up code for DumpLoadedClassList

[Daniel D. Daugherty]

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
>>>>
>>>