8181087: Module system implementation refresh (6/2017 update)

[Peter Levart]

Hi Alan,

Looking just at changes in jdk part...

On 06/14/2017 06:52 PM, Alan Bateman wrote:
> It's time to bring the changes accumulated in the jake forest into 
> jdk9/dev. I'm hoping we are near the end of these updates and that we 
> can close the jake forest soon.
>
> A summary of the changes that have accumulated for this update are 
> listed in this issue:
>     https://bugs.openjdk.java.net/browse/JDK-8181087
>
> Aside from the important spec/javadoc updates, this update will bring 
> the -illegal-access option into JDK 9 to replace the 
> --permit-illegal-access option.
>
> The webrevs with the changes are here:
>   http://cr.openjdk.java.net/~alanb/8181087/1/
>
> -Alan

In j.l.Class, you have this new method:

2447     List<Method> getDeclaredPublicMethods(String name, Class<?>... 
parameterTypes) {
2448         Method[] methods = privateGetDeclaredMethods(/* publicOnly 
*/ true);
2449         List<Method> result = new ArrayList<>();
2450         for (Method method : methods) {
2451             if (method.getName().equals(name)
2452                 && Arrays.equals(
2453 reflectionFactory.getExecutableSharedParameterTypes(method),
2454                     parameterTypes)) {
2455 result.add(getReflectionFactory().copyMethod(method));
2456             }
2457         }
2458         return result;
2459     }

...where you use the 'reflectionFactory' field one time and 
'getReflectionFactory()' method another time. The field might not 
already be initialized...

3479     // Fetches the factory for reflective objects
3480     private static ReflectionFactory getReflectionFactory() {
3481         if (reflectionFactory == null) {
3482             reflectionFactory =
3483                 java.security.AccessController.doPrivileged
3484                     (new 
ReflectionFactory.GetReflectionFactoryAction());
3485         }
3486         return reflectionFactory;
3487     }
3488     private static ReflectionFactory reflectionFactory;

Since 'reflectionFactory' field is not volatile, I would also introduce 
a local variable into getReflectionFactory() so that the field is read 
only once which would prevent theoretical possibility of reordering the 
re-reading of the non-volatile filed before the (1st) reading of the 
field which could cause  getReflectionFactory() to return null.


In j.l.Module:

There are two places in the same method that contain exactly the same 
fragment of code:

  566                 if (targets.contains(EVERYONE_MODULE) || 
targets.contains(other))
  567                     return true;
  568                 if (other != EVERYONE_MODULE
  569                     && !other.isNamed() && 
targets.contains(ALL_UNNAMED_MODULE))
  570                     return true;

Perhaps this could be factored out into separate private method which 
could also be made a little more optimal (when other == EVERYONE_MODULE 
and targets does not contain it, it is looked-up twice currently). For 
example:

private static boolean isIncludedIn(Module module, Set<Module> targets) {
     return
         targets != null && (
             targets.contains(EVERYONE_MODULE) ||
             !module.isNamed() && targets.contains(ALL_UNNAMED_MODULE) 
|| // ALL_UNNAMED_MODULE.isNamed() == false
             module != EVERYONE_MODULE && module != ALL_UNNAMED_MODULE 
&& targets.contains(module)
          );
}


In j.u.ServiceLoader:

The following method:

  601     /**
  602      * Returns true if the given class is assignable to a class or 
interface
  603      * with the given name.
  604      */
  605     private boolean isAssignable(Class<?> clazz, String className) {
  606         Class<?> c = clazz;
  607         while (c != null) {
  608             if (c.getName().equals(className)) {
  609                 return true;
  610             }
  611             for (Class<?> interf : c.getInterfaces()) {
  612                 if (interf.getName().equals(className)) {
  613                     return true;
  614                 }
  615             }
  616             c = c.getSuperclass();
  617         }
  618         return false;
  619     }

...does not return true for situations like:

public interface A {}
public interface B extends A {}
public class C implements B {}

isAssignable(C.class, "A") ???

If A is a service interface and C is its implementation class, should C 
be considered a valid service provider? If yes, the method might need 
some stack or queue or recursion.


In IllegalAccessLogger:

Is the following method:

280     private void log(Class<?> caller, String what, Supplier<String> 
msgSupplier) ...

Invoked frequently from different threads? Might synchronization in this 
method be a performance bottleneck? There are some optimizations possible...



That's all for now.


Regards, Peter