Review Request: JDK-8173978: Lookup.in should allow teleporting from a lookup class in a named module without dropping all access

[Peter Levart]

Hi Mandy,

On 7/2/19 7:20 PM, Mandy Chung wrote:
> Webrev updated:
> http://cr.openjdk.java.net/~mchung/jdk14/8173978/webrev.01/

I just skimmed across code and I think there's a little optimization 
possible in VerifyAccess:

  224             // cross-module access
  225             // 1. refc is in different module from lookupModule, or
  226             // 2. refc is in lookupModule and a different module 
from prevLookupModule
  227             Module prevLookupModule = prevLookupClass != null ? 
prevLookupClass.getModule()
  228                                                               : 
lookupModule;
  229             assert refModule != lookupModule || refModule != 
prevLookupModule;
  230             if (isModuleAccessible(refc, lookupModule, 
prevLookupModule))
  231                 return true;

...instead of seting prevLookupModule to lookupModule in case 
prevLookupClass is null (common case) causing double canRead and 
isExported checks against the same module in isModuleAccessible() (for 
positive outcome - the case where we want the check to be quick):

  250     public static boolean isModuleAccessible(Class<?> refc,  
Module m1, Module m2) {
  251         Module refModule = refc.getModule();
  252         assert refModule != m1 || refModule != m2;
  253         int mods = getClassModifiers(refc);
  254         if (isPublic(mods)) {
  255             if (m1.canRead(refModule) && m2.canRead(refModule)) {
  256                 String pn = refc.getPackageName();
  257
  258                 // refc is exported package to at least both m1 and m2
  259                 if (refModule.isExported(pn, m1) && 
refModule.isExported(pn, m2))
  260                     return true;
  261             }
  262         }
  263         return false;
  264     }


...you could set prevLookupModule to null if prevLookupClass is null:

             Module prevLookupModule = prevLookupClass != null
                                                               ? 
prevLookupClass.getModule()
                                                               : null;

...and then check against null in isModuleAccessible():

     public static boolean isModuleAccessible(Class<?> refc, Module m1, 
Module m2) {
         Module refModule = refc.getModule();
         assert refModule != m1 || refModule != m2;
         int mods = getClassModifiers(refc);
         if (isPublic(mods)) {
             if (m1.canRead(refModule) && (m2 == null || 
m2.canRead(refModule))) {
                 String pn = refc.getPackageName();

                 // refc is exported package to at least both m1 and m2
                 if (refModule.isExported(pn, m1) && (m2 == null || 
refModule.isExported(pn, m2)))
                     return true;
             }
         }
         return false;
     }


I think this would be faster since canRead and isExported are not trivial...

Regards, Peter