RFR(S): 8209598: Clean up how messages are printed when CDS aborts start-up
Calvin Cheung
calvin.cheung at oracle.com
Fri Oct 26 00:02:57 UTC 2018
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