[16] RFR(S) 8246546 Simplify SystemDictionary::is_shared_class_visible

[Ioi Lam]

On 6/23/20 1:39 PM, Yumin Qi wrote:
> +1.
>
> BTW, v02-delta:
>
> src/hotspot/share/memory/filemap.cpp:
>
> I think you added an empty line by accident. Don't need extra webrev.
>

Hi Yumin,

Thanks for the review. That empty was removed by accident in version 
v01, so I removed it in v02.

Thanks
- Ioi
>
> Thanks
>
> Yumin
>
> On 6/23/20 1:27 PM, Calvin Cheung wrote:
>> Hi Ioi,
>>
>> The updated webrev looks good.
>>
>> thanks,
>>
>> Calvin
>>
>> On 6/23/20 12:38 PM, Ioi Lam wrote:
>>> Hi Calvin,
>>>
>>> Thanks for the review. I've updated the webrev according to feedback 
>>> by you and Yumin:
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk16/8246546_simplify_is_shared_class_visible.v02/ 
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk16/8246546_simplify_is_shared_class_visible.v02-delta/ 
>>>
>>>
>>> On 6/22/20 5:37 PM, Calvin Cheung wrote:
>>>> Hi Ioi,
>>>>
>>>> In the new implementation of 
>>>> SystemDictionary::is_shared_class_visible_impl, it seems to be 
>>>> missing the following check which was in the old implementation:
>>>>
>>>> 1309         if (mod_entry != NULL && mod_entry->is_patched()) {
>>>> 1310           return false;
>>>> 1311         }
>>>>
>>>> Maybe the check could be included in line 1377; check if the shared 
>>>> path indexes are the same and !mod_entry->is_patched?
>>>>
>>>
>>> I think currently we disable CDS when --patch-module is specified. 
>>> Anyway, for forward-compatibility, I added this code
>>>
>>>   void set_is_patched() {
>>>       _is_patched = true;
>>> +    CDS_ONLY(_shared_path_index = -1); // Mark all shared classes 
>>> in this module invisible.
>>>   }
>>>
>>> I also added an assert in 
>>> SystemDictionary::is_shared_class_visible_impl:
>>>
>>>     if (should_be_in_named_module) {
>>>       // Is the module loaded from the same location as during dump 
>>> time?
>>>       visible = mod_entry->shared_path_index() == scp_index;
>>> +     if (visible) {
>>> +       assert(!mod_entry->is_patched(), "cannot load archived 
>>> classes for patched module");
>>> +     }
>>>
>>> I also modified the PatchModule/Simple.java test case to create a 
>>> valid JSA file. So in case we start support --patch-module with CDS, 
>>> this new code will be tested.
>>>
>>>
>>>
>>>
>>>> Couple of minor nits:
>>>>
>>>> filemap.cpp
>>>>
>>>> Blank line #380 was accidentally deleted?
>>>>
>>>
>>> Reverted
>>>
>>>> systemDictionary.hpp
>>>>
>>>> 632   static bool is_shared_class_visible_impl(Symbol* class_name, 
>>>> InstanceKlass* ik,
>>>>
>>>> If you keep the second parameter in a separate line as before, the 
>>>> above change is unnecessary.
>>>>
>>>
>>> Fixed.
>>>
>>>
>>> Thanks
>>> - Ioi
>>>> thanks,
>>>>
>>>> Calvin
>>>>
>>>>
>>>> On 6/16/20 3:53 PM, Ioi Lam wrote:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8246546
>>>>> http://cr.openjdk.java.net/~iklam/jdk16/8246546_simplify_is_shared_class_visible.v01/ 
>>>>>
>>>>>
>>>>> The current implementation of 
>>>>> SystemDictionary::is_shared_class_visible has grown more and more 
>>>>> complex over time. Now it has checks for many special cases and 
>>>>> the code is hard to understand.
>>>>>
>>>>> I have simplified the logic to:
>>>>>
>>>>> Between dump time and run time, if a class:
>>>>>
>>>>>   - has the same class loader
>>>>>   - belongs to the same module
>>>>>   - is loaded from the same location
>>>>>
>>>>> ... then this class is visible at run time.
>>>>>
>>>>> The new check should be more robust and faster.
>>>>>
>>>>> For validation, I kept the old implementation and assert that the 
>>>>> new code produces the exact same result. I will remove 
>>>>> SystemDictionary::is_shared_class_visible_impl_old() and 
>>>>> SystemDictionaryShared::is_shared_class_visible_for_classloader() 
>>>>> when I do the actual push.
>>>>>
>>>>> Testing -- all CDS tests passed locally. Running mach5 tiers 1-4 now.
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>