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

Yumin Qi yumin.qi at oracle.com
Tue Jun 23 20:39:17 UTC 2020


+1.

BTW, v02-delta:

src/hotspot/share/memory/filemap.cpp:

I think you added an empty line by accident. Don't need extra webrev.


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


More information about the hotspot-runtime-dev mailing list