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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
In reply to this post by Alan Bateman
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

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Alan Bateman

> On Jun 14, 2017, at 9:52 AM, Alan Bateman <[hidden email]> 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<String> 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<URL> getResources(String name) throws IOException {
 407         // this loader
 408         List<URL> 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

Reply | Threaded
Open this post in threaded view
|

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

Peter Levart
In reply to this post by 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<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)
>          );
> }

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

Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
In reply to this post by Peter Levart
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
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
In reply to this post by Mandy Chung


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<String> 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<URL> getResources(String name) throws IOException {
>   407         // this loader
>   408         List<URL> 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.
Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Alan Bateman

> On Jun 15, 2017, at 12:34 AM, Alan Bateman <[hidden email]> wrote:
>
>>
>> java/lang/Module.java
>>  901     void implAddOpensToAllUnnamed(Iterator<String> 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.


Mandy
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> wrote:
>>
>>> java/lang/Module.java
>>>   901     void implAddOpensToAllUnnamed(Iterator<String> 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
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
In reply to this post by Peter Levart
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