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

[Ioi Lam]

HI Yumin,

Thanks for the review. I've updated the webrev according to feedback by 
you and Calvin:

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 3:10 PM, Yumin Qi wrote:
>
> Hi Ioi:
>
>   Looks good except for Q3, others are minor comments.
>
> 1) src/hotspot/share/classfile/moduleEntry.cpp:
>
>    57   if (location != NULL) {
>    58     location->increment_refcount();
> 59 CDS_ONLY(if (UseSharedSpaces) {
> 60 _shared_path_index = 
> FileMapInfo::get_module_shared_path_index(_location);
> 61 });
>    62   }
>   
>    Could you use either location or _location from line 57 to 62? It does not matter mix use, better keep using same variable.

I changed to

get_module_shared_path_index(location)


> 2) src/hotspot/share/classfile/moduleEntry.hpp:
> 72 CDS_ONLY(int _shared_path_index;) // >0 if this classes in this 
> module are in CDS archive this => those

I corrected the comment as:

          // >=0 if classes in this module are in CDS archive

== 0 means the module is loaded from "modules image".
 >0    means the module is loaded from --module-path.

> 94 CDS_ONLY(_shared_path_index = -1);
>
>       Custom shared class path index set to -999, please make sure the 
> checking of _shared_path_index not against '0' since both will give 
> same result.
>
We have this assert:

bool SystemDictionary::is_shared_class_visible_impl(Symbol* class_name,
                                                     InstanceKlass* ik,
                                                     PackageEntry* 
pkg_entry,
                                                     Handle 
class_loader, TRAPS) {
   int scp_index = ik->shared_classpath_index();
   assert(scp_index >= 0, "must be");

I also added

   assert(!ik->is_shared_unregistered_class(), "this function should be 
called for built-in classes only");


> 3) src/hotspot/share/classfile/systemDictionary.cpp:
>
> 1251 // (1) Check if we are loading into the same loader as in dump time.
> 1252
> 1253 if (ik->is_shared_boot_class()) {
> 1254 if (class_loader() != NULL) {
> 1255 return false;
> 1256 }
> 1257 } else if (ik->is_shared_platform_class()) {
> 1258 if (class_loader() != java_platform_loader()) {
> 1259 return false;
> 1260 }
> 1261 } else if (ik->is_shared_app_class()) {
> 1262 if (class_loader() != java_system_loader()) {
> 1263 return false;
> 1264 }
> 1265 } else {
> 1266 // ik was loaded by a custom loader during dump time
> 1267 if 
> (class_loader_data(class_loader)->is_builtin_class_loader_data()) {
> 1268       return false;
> 1269     } else {
> 1270       return true;
> 1271     }
> 1272   }
>
> the logic for line 1625 - 1272:
> If the class is shared custom class, and the loader is custom loader, it will return "true", is that true?
> What if the class is not loaded by the loader?

We allow any custom class loader to load any shared custom classes. This 
is the same behavior as before my patch.

>
> 4) src/hotspot/share/memory/filemap.cpp:
> 521 assert(shared_path(0)->is_modules_image(), "first shared_path must 
> be the modules image"); modules image => module image.

I searched our source tree. There are 21 cases of "modules image" and 4 
cases of "module image". Since the function name is "is_modules_image", 
I think it's better to say "modules image" here.

Thanks
- Ioi

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