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

[Alan Bateman]

On 14/06/2017 22:44, Peter Levart wrote:

:


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

Okay, make sense, I'll do that.

-Alan

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

[Alan Bateman]

On 15/06/2017 15:28, Mandy Chung wrote:

On Jun 15, 2017, at 12:34 AM, Alan Bateman  wrote:


java/lang/Module.java
  901 void implAddOpensToAllUnnamed(Iterator iterator) {
  902 if (jdk.internal.misc.VM.isModuleSystemInited()) {
  903 iterator.forEachRemaining(pn ->
  904 implAddExportsOrOpens(pn, ALL_UNNAMED_MODULE, true, 
true));
  905 return;
  906 }

AFAICT this should only be called during module system initialization.
When will this method be called after initPhase 2?

It's for use during startup (initPhase2) only. If called later then it works as 
if the module somehow reflectively opened the packages to all unnamed modules. 
I wouldn't object to changing it to throwing an exception (assuming that is 
what you are thinking) as the JDK doesn't have any use for this after 
initPhase2.

Yes this is what I am thinking.  This method should catch when it’s called 
which is not expected.


Okay, I can do that.

-Alan

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

[Peter Levart]

Hi Alan,

On 06/15/2017 09:34 AM, Alan Bateman wrote:

2453 reflectionFactory.getExecutableSharedParameterTypes(method),

reflectionFactory should be accessed via getReflectionFactory().  I 
see Peter

already comments on this.
Thanks, it should be getReflectionFactory(). 


...and I would also introduce a local variable in getReflectionFactory() 
as was done recently in String.hashCode(). It might not be a problem 
now, but could become one in the future...


Regards, Peter

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

[Alan Bateman]

On 15/06/2017 06:37, Mandy Chung wrote:

:
java/lang/Class.java
2453 
reflectionFactory.getExecutableSharedParameterTypes(method),

reflectionFactory should be accessed via getReflectionFactory().  I see Peter
already comments on this.

Thanks, it should be getReflectionFactory().




java/lang/Module.java
  901 void implAddOpensToAllUnnamed(Iterator iterator) {
  902 if (jdk.internal.misc.VM.isModuleSystemInited()) {
  903 iterator.forEachRemaining(pn ->
  904 implAddExportsOrOpens(pn, ALL_UNNAMED_MODULE, true, 
true));
  905 return;
  906 }

AFAICT this should only be called during module system initialization.
When will this method be called after initPhase 2?
It's for use during startup (initPhase2) only. If called later then it 
works as if the module somehow reflectively opened the packages to all 
unnamed modules. I wouldn't object to changing it to throwing an 
exception (assuming that is what you are thinking) as the JDK doesn't 
have any use for this after initPhase2.




jdk/internal/loader/Loader.java
  406 public Enumeration getResources(String name) throws IOException {
  407 // this loader
  408 List urls = findResourcesAsList(name);
  409
  410 // parent loader
  411 
parent.getResources(name).asIterator().forEachRemaining(urls::add);
  412
  413 return Collections.enumeration(urls);
  414 }

We could consider returning an Enumeration that defers finding resources
from parent class loader after iterating the local resources.
Yes, this is possible but has a lot of potential issues. The new stream 
returning resources() methods is better for doing lazy consumption and 
it could override this (although still lots of potential issues, esp. 
when running with a security manager, concerns about context change like 
TCCL, etc.).




:

ModulePath.java
  408 private static final Attributes.Name AUTOMATIC_MODULE_NAME
  409 = new Attributes.Name("Automatic-Module-Name");

Should this be defined in java.util.jar.Attributes.Name?
As I recall, this was deliberately not included in the automatic name 
proposal.





ServiceLoader.java

1174 } else if (!isAssignable(clazz, serviceName)) {
1175 fail(service, clazz.getName() + " not a subtype”);

Peter raises the question on isAssignable that I think it should look at the 
interfaces recursively.  On the other hand, I wonder if this should simply fail 
if service.isAssignableFrom(clazz) returns false (which I think it’s the 
existing JDK 8 behavior).


Yes, I think I will remove this.

-Alan.

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

[Peter Levart]

Re-reading my post in the morning...

On 06/14/2017 11:44 PM, Peter Levart wrote:

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

 );
}


...this last method is not entirely correct. if called as 
isIncluded(EVERYONE_MODULE, targets) and targets does not contain 
EVERYONE_MODULE but it contains ALL_UNNAMED_MODULE, the method returns 
true, because EVERYONE_MODULE.isNamed() returns false, which is not 
correct I think. The correct logic would be this:


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

 );
}



Regards, Peter

Re: 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/ 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 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

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

[Mandy Chung]

> On Jun 14, 2017, at 9:52 AM, 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/
> 

java/lang/Class.java
2453 
reflectionFactory.getExecutableSharedParameterTypes(method),

reflectionFactory should be accessed via getReflectionFactory().  I see Peter
already comments on this.

java/lang/Module.java
 901 void implAddOpensToAllUnnamed(Iterator iterator) {
 902 if (jdk.internal.misc.VM.isModuleSystemInited()) {
 903 iterator.forEachRemaining(pn ->
 904 implAddExportsOrOpens(pn, ALL_UNNAMED_MODULE, true, true));
 905 return;
 906 }

AFAICT this should only be called during module system initialization.
When will this method be called after initPhase 2?

jdk/internal/loader/Loader.java
 406 public Enumeration getResources(String name) throws IOException {
 407 // this loader
 408 List urls = findResourcesAsList(name);
 409 
 410 // parent loader
 411 parent.getResources(name).asIterator().forEachRemaining(urls::add);
 412 
 413 return Collections.enumeration(urls);
 414 }

We could consider returning an Enumeration that defers finding resources
from parent class loader after iterating the local resources.

ModuleBootstrap.java
   line 406, 418 the formatting seems off.

ModulePath.java
 408 private static final Attributes.Name AUTOMATIC_MODULE_NAME
 409 = new Attributes.Name("Automatic-Module-Name");

Should this be defined in java.util.jar.Attributes.Name?

ServiceLoader.java

1174 } else if (!isAssignable(clazz, serviceName)) {
1175 fail(service, clazz.getName() + " not a subtype”);

Peter raises the question on isAssignable that I think it should look at the 
interfaces recursively.  On the other hand, I wonder if this should simply fail 
if service.isAssignableFrom(clazz) returns false (which I think it’s the 
existing JDK 8 behavior).

Mandy

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

[serguei.spit...@oracle.com]

Hi Alan,

I looked at the hotspot and top changes.
They look good.

Thanks,
Serguei


On 6/14/17 09:52, 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

Re: 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 getDeclaredPublicMethods(String name, Class... 
parameterTypes) {
2448 Method[] methods = privateGetDeclaredMethods(/* publicOnly 
*/ true);

2449 List 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 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 
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

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

[Alan Bateman]

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