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

Yumin Qi yumin.qi at oracle.com
Mon Jun 22 22:10:40 UTC 2020


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.

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

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?

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


More information about the hotspot-runtime-dev mailing list