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

Ioi Lam ioi.lam at oracle.com
Tue Jun 23 19:38:21 UTC 2020


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