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

[Calvin Cheung]

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
>