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

[Alan Bateman]

Thanks for looking through this.

On 14/06/2017 22:44, Peter Levart wrote:
> :
>
> ...where you use the 'reflectionFactory' field one time and 
> 'getReflectionFactory()' method another time. The field might not 
> already be initialized...
Well spotted, it should be using getReflectionFactory(), not 
reflectionFactory. I'm not sure how that crept in.


> :
>
>
> In j.u.ServiceLoader:
>
> :
>
> ...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.
I think I'd prefer to just remove this completely. As you probably 
gathered, this is just to disambiguate error cases when 
Class.isAssignableFrom indicates the provider listed in 
META-INF/services/<service> is not an implementation of the service 
type. It's a long standing SL issue (pre-dates JDK 9) and probably not 
worth trying to improve this now.

>
>
> 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...
Are you concerned about the permit case or the warn/debug case? For the 
permit (default) case then the logger is discarded at the first illegal 
access. At worse, then several threads compete (and synchronized) to be 
the first offender but there is no synchronization or logging after that.

With the warn/debug case then someone has opt'ed in to get warnings or 
stack traces. If there is really bad code with continuous calls to log 
because of illegal reflective access then it could indeed be a 
bottleneck.  I don't think we should be too concerned with that. Yes, it 
could be improved if it really is an issue so I think we should wait to 
see what the default setting will be in JDK 10. If it becomes "warn" 
then we might have to look at it again.

-Alan