RFR(S): 8209598: Clean up how messages are printed when CDS aborts start-up

David Holmes david.holmes at oracle.com
Fri Oct 26 00:06:49 UTC 2018 

Thanks Calvin!

David

On 26/10/2018 10:02 AM, Calvin Cheung wrote:
> Hi David,
> 
> On 10/25/18, 4:41 PM, David Holmes wrote:
>> Hi Calvin,
>>
>> The code changes look good to me - thanks.
> Thanks for your review.
>>
>> The bug synopsis and description needs updating to reflect the actual 
>> change though.
> The bug synopsis was updated yesterday. I've updated the email subject 
> with the new synopsis.
> I've also added a comment to the bug report.
> 
> thanks,
> Calvin
> 
>>
>> Thanks,
>> David
>>
>> On 26/10/2018 8:51 AM, Calvin Cheung wrote:
>>> Hi Ioi,
>>>
>>> I've updated classLoader.cpp based on your suggestions:
>>>
>>> http://cr.openjdk.java.net/~ccheung/8209598/webrev.03/src/hotspot/share/classfile/classLoader.cpp.sdiff.html 
>>>
>>>
>>> thanks,
>>> Calvin
>>>
>>> On 10/25/18, 2:03 PM, Ioi Lam wrote:
>>>> Hi Calvin,
>>>>
>>>> 1474     bool path_stat = get_canonical_path(path, 
>>>> canonical_class_src_path, JVM_MAXPATHLEN);
>>>> 1475     assert(path_stat, "Bad src path");
>>>>
>>>> 1478       path_stat = get_canonical_path(ent->name(), 
>>>> canonical_path_table_entry, JVM_MAXPATHLEN);
>>>> 1479       assert(path_stat, "Bad shared path");
>>>>
>>>> I think the variable "path_stat" is not clear -- it seems to suggest 
>>>> some sort of statistics. How about just use 'bool success'?
>>>>
>>>> Also, I think we need a comment to say why this assert is here. 
>>>> Something like
>>>>
>>>> // At this point all the paths have already been checked in 
>>>> ....where?..... and
>>>> // must be valid.
>>>> assert(success, "must be valid path");
>>>>
>>>> The rest of the changes look good.
>>>>
>>>> Thanks
>>>>
>>>> - Ioi
>>>>
>>>>
>>>>
>>>>
>>>> On 10/25/18 10:09 AM, Calvin Cheung wrote:
>>>>> Hi Jiangli,
>>>>>
>>>>> Thanks for your review.
>>>>>
>>>>> On 10/24/18, 4:39 PM, Jiangli Zhou wrote:
>>>>>> Hi Calvin,
>>>>>>
>>>>>> - src/hotspot/share/classfile/classLoader.cpp
>>>>>>
>>>>>> 1474     if (!get_canonical_path(path, canonical_class_src_path, 
>>>>>> JVM_MAXPATHLEN)) {
>>>>>> 1475       log_error(cds)("Bad pathname %s. CDS dump aborted.", 
>>>>>> path);
>>>>>> 1476       vm_exit(1);
>>>>>> 1477     }
>>>>>>
>>>>>> 1480       if (!get_canonical_path(ent->name(), 
>>>>>> canonical_path_table_entry, JVM_MAXPATHLEN)) {
>>>>>> 1481         log_error(cds)("Bad pathname %s. CDS dump aborted.", 
>>>>>> ent->name());
>>>>>> 1482         vm_exit(1);
>>>>>> 1483       }
>>>>>>
>>>>>> By the time ClassLoader::record_result() is called, it should be 
>>>>>> guaranteed that the paths from the class stream source and the 
>>>>>> recorded path table are valid. Both of the above log_error & 
>>>>>> vm_exit should be replaced by asserting the return value of 
>>>>>> get_canonical_path() is true.
>>>>> I've made the above change.
>>>>>>
>>>>>> - src/hotspot/share/memory/metaspaceShared.cpp
>>>>>>
>>>>>> 1648     if (IgnoreUnverifiableClassesDuringDump) {
>>>>>> 1649       // This is useful when running JCK or SQE tests. You 
>>>>>> should not
>>>>>> 1650       // enable this when running real apps.
>>>>>> 1651       SystemDictionary::remove_classes_in_error_state();
>>>>>> 1652     } else {
>>>>>> 1653       log_error(cds)("Please remove the unverifiable classes 
>>>>>> from your class list and try again");
>>>>>> 1654       exit(1);
>>>>>> 1655     }
>>>>>>
>>>>>> IgnoreUnverifiableClassesDuringDump is enabled by default and we 
>>>>>> don't include unverifiable classes in the archive. The above 
>>>>>> comment is outdated. We probably can remove the diagnostic option, 
>>>>>> IgnoreUnverifiableClassesDuringDump altogether now. We used to 
>>>>>> quit dumping process when encountering unverifiable classes (we 
>>>>>> didn't have the class removal part), then 
>>>>>> IgnoreUnverifiableClassesDuringDump was added but set to false 
>>>>>> initially (to keep the old behavior). 
>>>>>> IgnoreUnverifiableClassesDuringDump was turned on by default 
>>>>>> later. The current behavior is the proper behavior and we can 
>>>>>> clean up the code.
>>>>>
>>>>> I've updated the comment. I think we should handle the removal of 
>>>>> the flag in a separate bug.
>>>>>
>>>>> I've also added a vm_exit_during_cds_dumping() function as 
>>>>> suggested by David so that we don't need to do explicit exit(1) at 
>>>>> various places.
>>>>>
>>>>> Here's an updated webrev:
>>>>>     http://cr.openjdk.java.net/~ccheung/8209598/webrev.02/
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Jiangli
>>>>>>
>>>>>>
>>>>>> On 10/23/18 9:36 PM, Calvin Cheung wrote:
>>>>>>> Ioi, David,
>>>>>>>
>>>>>>> Here's an updated webrev:
>>>>>>>     http://cr.openjdk.java.net/~ccheung/8209598/webrev.01/
>>>>>>>
>>>>>>> I've run those 2 tests locally on linux-x64.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>>>
>>>>>>> On 10/23/18, 8:50 PM, Ioi Lam wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/23/18 7:27 PM, Calvin Cheung wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 10/23/18, 5:18 PM, Ioi Lam wrote:
>>>>>>>>>> Hi Calvin,
>>>>>>>>>>
>>>>>>>>>>    if (PrintSharedArchiveAndExit) {
>>>>>>>>>>      if (PrintSharedDictionary) {
>>>>>>>>>> -      tty->print_cr("\nShared classes:\n");
>>>>>>>>>> +      log_info(cds)("\nShared classes:\n");
>>>>>>>>>>        SystemDictionary::print_shared(tty);
>>>>>>>>>>      }
>>>>>>>>>>      if (_archive_loading_failed) {
>>>>>>>>>> -      tty->print_cr("archive is invalid");
>>>>>>>>>> +      log_error(cds)("archive is invalid");
>>>>>>>>>>        vm_exit(1);
>>>>>>>>>>      } else {
>>>>>>>>>> -      tty->print_cr("archive is valid");
>>>>>>>>>> +      log_info(cds)("archive is valid");
>>>>>>>>>>        vm_exit(0);
>>>>>>>>>>      }
>>>>>>>>>>    }
>>>>>>>>>>
>>>>>>>>>> I think this part should use print_cr, because the option says 
>>>>>>>>>> "Print ...". It shouldn't be necessary to explicitly set 
>>>>>>>>>> -Xlog:cds in order to get the printed message.
>>>>>>>>> Should I just revert log_info changes and leave the log_error 
>>>>>>>>> there?
>>>>>>>>
>>>>>>>> I think the log_error should be reverted as well, because it 
>>>>>>>> would look out of place with the rest of the output.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> - Ioi
>>>>>>>>>>
>>>>>>>>>> If you revert this, I think the two test cases also can be 
>>>>>>>>>> reverted.
>>>>>>>>> Yes, the changes to the two tests were due to the log_info 
>>>>>>>>> changes.
>>>>>>>>>>
>>>>>>>>>> The rest of the changes  look OK.
>>>>>>>>> Thanks for your review.
>>>>>>>>>
>>>>>>>>> Calvin
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>
>>>>>>>>>> - Ioi
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 10/23/18 2:45 PM, Calvin Cheung wrote:
>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8209598
>>>>>>>>>>>
>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8209598/webrev.00/
>>>>>>>>>>>
>>>>>>>>>>> Use log_error(cds) instead of tty->print_cr for CDS error 
>>>>>>>>>>> messages. Also converted 2 CDS info messages to log_info(cds).
>>>>>>>>>>>
>>>>>>>>>>> Testing: mach5 hs-tier{1,2,3}
>>>>>>>>>>>
>>>>>>>>>>> thanks,
>>>>>>>>>>> Calvin
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>

More information about the hotspot-runtime-dev mailing list