Re: RFR: JDK-8136893: Improve early java.lang.invoke infrastructure initialization

2015-09-30 Thread Paul Sandoz

> On 25 Sep 2015, at 17:19, Peter Levart  wrote:
> 
> Hi Paul,
> 
> Thanks for looking into this.
> 

Apologies for the late reply.


> On 09/25/2015 04:07 PM, Paul Sandoz wrote:
>> Hi,
>> 
>> This looks like a partial dup of 
>> https://bugs.openjdk.java.net/browse/JDK-8076596
> 
> Ah, sorry, I wasn't aware this has already been registered in JIRA. I have 
> linked the two issues together as DUPs.
> 

Thanks.


>> 
>> The changes look ok, but I am concerned post initialization there may be 
>> code paths taken that require the system class loader to be used but instead 
>> the boot stream class loader is used instead. Is that a legitimate concern?
> 
> It is legitimate, but I have inspected usages and:
> 
> - In java.lang.invoke.BoundMethodHandle.Factory#makeCbmhCtor, null is passed 
> explicitly and this method is used only from 
> java.lang.invoke.BoundMethodHandle.Factory#makeCtors which is used from 
> java.lang.invoke.BoundMethodHandle.SpeciesData#initForBootstrap and 
> java.lang.invoke.BoundMethodHandle.SpeciesData#SpeciesData(java.lang.String, 
> java.lang.Class). These usages 
> only deal with erased MH types (Object, long, int, double, float).
> 
> - In java.lang.invoke.MemberName#getMethodType, the result of 
> MemberName.getClassLoader() is passed to the method. This is the class loader 
> of the member's declaring class. Any types referenced from the member 
> declaration should be resolvable from this class loader. A member declared by 
> a bootstrap class (MemberName.getClassLoader() returns null) can only 
> reference types resolvable from bootstrap loader.
> 
> - In java.lang.invoke.MemberName#getFieldType, the result of 
> MemberName.getClassLoader() is passed to the method. Similar applies as above.
> 
> - In java.lang.invoke.MethodHandleNatives#fixMethodType(Class callerClass, 
> Object type), the callerClass.getClassLoader() is passed to the method. This 
> is invoked from:
> java.lang.invoke.MethodHandleNatives#linkMethodImpl(
> Class callerClass, int refKind,
> Class defc, String name, Object type,
> Object[] appendixResult)
> which is called from java.lang.invoke.MethodHandleNatives#linkMethod(same 
> args)
> 
> I suppose this is an upcall from VM to link a call to the 
> @PolymorphicSignature method and callerClass is the class in which the 
> invocation bytecodes are executed. Am I right?

Yes.

And before that there is an upcall to MethodHandleNatives.findMethodHandleType, 
see SystemDictionary::find_method_handle_invoker in 
src/share/vm/classfile/systemDictionary.cpp.

AFAICT the “type” argument passed to the linkMethod should always be of 
MethodType:

static MemberName linkMethod(Class callerClass, int refKind,
 Class defc, String name, Object type,
 Object[] appendixResult) {
if (!TRACE_METHOD_LINKAGE)
return linkMethodImpl(callerClass, refKind, defc, name, type, 
appendixResult);
return linkMethodTracing(callerClass, refKind, defc, name, type, 
appendixResult);
}
static MemberName linkMethodImpl(Class callerClass, int refKind,
 Class defc, String name, Object type,
 Object[] appendixResult) {
try {
if (defc == MethodHandle.class && refKind == REF_invokeVirtual) {
return Invokers.methodHandleInvokeLinkerMethod(name, 
fixMethodType(callerClass, type), appendixResult);
}
} catch (Throwable ex) {
if (ex instanceof LinkageError)
throw (LinkageError) ex;
else
throw new LinkageError(ex.getMessage(), ex);
}
throw new LinkageError("no such method "+defc.getName()+"."+name+type);
}
private static MethodType fixMethodType(Class callerClass, Object type) {
if (type instanceof MethodType)
return (MethodType) type;
else
return MethodType.fromMethodDescriptorString((String)type, 
callerClass.getClassLoader());
}

Thus it seems fixMethodType is not necessary. I think this is confirmed when 
looking at up calls to linkMethodHandleConstant and linkCallSite.


> The reasoning is as follows: if callerClass.getClassLoader() is sufficient 
> for resolving types that appear at the call site in a non-bootstrap 
> callerClass, then bootstrap classloader should be sufficient to resolve types 
> that appear at the call site in a bootstrap callerClass. Does anybody have 
> any other opinion?
> 
> - sun.invoke.util.BytecodeDescriptor#parseMethod(java.lang.String, 
> java.lang.ClassLoader) is only used from the newly introduced 
> java.lang.invoke.MethodType#fromDescriptor
> 

Ok, i think you have things covered, thanks,
Paul.


signature.asc
Description: Message signed with OpenPGP using GPGMail
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev


Re: RFR: JDK-8136893: Improve early java.lang.invoke infrastructure initialization

2015-09-26 Thread Michael Haupt
Hi Peter,

> Am 25.09.2015 um 17:32 schrieb Peter Levart :
>> One question about MethodType: would you mind doing something about the 
>> naming of the newly introduced fromDescriptor() method? Its name does not 
>> suggest in any way that it chooses the class loader differently. The name is 
>> subtly different from that of fromDescriptorString(), and the signature is 
>> identical - it's probably really easy to confuse the two when working at the 
>> core libs level. Unfortunately, the only proposal for a name I can make, 
>> fromDescriptorStringBootCL(), is clunky. Maybe that's acceptable for a 
>> low-level method only visible at package level.
> 
> Well, the correct self-describing name would then be 
> fromDescriptorStringNullIsBootstrapCL()...

yes ... or the entire source code in camel case. ;-) I see your point, and 
wasn't happy with my original proposal either.

> Perhaps a note in the javadoc that this is the preferred method for internal 
> use would be sufficient? We can't make a reference from public method to the 
> package-private one in javadoc though.

That's a good idea.

Best,

Michael

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
  Oracle is committed to developing 
practices and products that help protect the environment

___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev


Re: RFR: JDK-8136893: Improve early java.lang.invoke infrastructure initialization

2015-09-25 Thread Peter Levart

Hi Michael,

On 09/25/2015 03:37 PM, Michael Haupt wrote:

Hi Peter,

thanks for this changeset. Note I'm not a Reviewer (with a capital R); please 
read this review in lower-case. ;-)

One question about MethodType: would you mind doing something about the naming 
of the newly introduced fromDescriptor() method? Its name does not suggest in 
any way that it chooses the class loader differently. The name is subtly 
different from that of fromDescriptorString(), and the signature is identical - 
it's probably really easy to confuse the two when working at the core libs 
level. Unfortunately, the only proposal for a name I can make, 
fromDescriptorStringBootCL(), is clunky. Maybe that's acceptable for a 
low-level method only visible at package level.


Well, the correct self-describing name would then be 
fromDescriptorStringNullIsBootstrapCL()...


Perhaps a note in the javadoc that this is the preferred method for 
internal use would be sufficient? We can't make a reference from public 
method to the package-private one in javadoc though.


Another option would be to create new method as public API and 
@Deprecate the old one. There is a convention that null means bootstrap 
loader in other public APIs as well (for example: Class.forName(String 
name, boolean initialize, ClassLoader loader); ) although passing null 
from user code is rarely needed if at all. This is usually just for 
internal use.


Regards, Peter




Am 25.09.2015 um 08:46 schrieb Peter Levart :
I have run the tests in java.lang.invoke and only have a problem with 1 test 
which seems to be related to the test or jtreg itself and happens also without 
my patch applied:

#Test Results (version 2)
#Tue Sep 22 09:48:38 CEST 2015
...
#section:script_messages
--messages:(0/0)--


test result: Error. Parse Exception: `@library' must appear before first `@run'

Yes. The test is also marked as ignored until another issue is fixed, so that 
can be ignored.

Other than the above remark/suggestion, this looks fine. I'll defer to an 
upper-case Reviewer, though.

Best,

Michael



___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev


Re: RFR: JDK-8136893: Improve early java.lang.invoke infrastructure initialization

2015-09-25 Thread Peter Levart

Hi Paul,

Thanks for looking into this.

On 09/25/2015 04:07 PM, Paul Sandoz wrote:

Hi,

This looks like a partial dup of 
https://bugs.openjdk.java.net/browse/JDK-8076596


Ah, sorry, I wasn't aware this has already been registered in JIRA. I 
have linked the two issues together as DUPs.




The changes look ok, but I am concerned post initialization there may be code 
paths taken that require the system class loader to be used but instead the 
boot stream class loader is used instead. Is that a legitimate concern?


It is legitimate, but I have inspected usages and:

- In java.lang.invoke.BoundMethodHandle.Factory#makeCbmhCtor, null is 
passed explicitly and this method is used only from 
java.lang.invoke.BoundMethodHandle.Factory#makeCtors which is used from 
java.lang.invoke.BoundMethodHandle.SpeciesData#initForBootstrap and 
java.lang.invoke.BoundMethodHandle.SpeciesData#SpeciesData(java.lang.String, 
java.lang.Class). These 
usages only deal with erased MH types (Object, long, int, double, float).


- In java.lang.invoke.MemberName#getMethodType, the result of 
MemberName.getClassLoader() is passed to the method. This is the class 
loader of the member's declaring class. Any types referenced from the 
member declaration should be resolvable from this class loader. A member 
declared by a bootstrap class (MemberName.getClassLoader() returns null) 
can only reference types resolvable from bootstrap loader.


- In java.lang.invoke.MemberName#getFieldType, the result of 
MemberName.getClassLoader() is passed to the method. Similar applies as 
above.


- In java.lang.invoke.MethodHandleNatives#fixMethodType(Class 
callerClass, Object type), the callerClass.getClassLoader() is passed to 
the method. This is invoked from:

 java.lang.invoke.MethodHandleNatives#linkMethodImpl(
 Class callerClass, int refKind,
 Class defc, String name, Object 
type,

 Object[] appendixResult)
which is called from 
java.lang.invoke.MethodHandleNatives#linkMethod(same args)


I suppose this is an upcall from VM to link a call to the 
@PolymorphicSignature method and callerClass is the class in which the 
invocation bytecodes are executed. Am I right? The reasoning is as 
follows: if callerClass.getClassLoader() is sufficient for resolving 
types that appear at the call site in a non-bootstrap callerClass, then 
bootstrap classloader should be sufficient to resolve types that appear 
at the call site in a bootstrap callerClass. Does anybody have any other 
opinion?


- sun.invoke.util.BytecodeDescriptor#parseMethod(java.lang.String, 
java.lang.ClassLoader) is only used from the newly introduced 
java.lang.invoke.MethodType#fromDescriptor



Regards, Peter



Paul.

On 25 Sep 2015, at 15:37, Michael Haupt  wrote:


Hi Peter,

thanks for this changeset. Note I'm not a Reviewer (with a capital R); please 
read this review in lower-case. ;-)

One question about MethodType: would you mind doing something about the naming 
of the newly introduced fromDescriptor() method? Its name does not suggest in 
any way that it chooses the class loader differently. The name is subtly 
different from that of fromDescriptorString(), and the signature is identical - 
it's probably really easy to confuse the two when working at the core libs 
level. Unfortunately, the only proposal for a name I can make, 
fromDescriptorStringBootCL(), is clunky. Maybe that's acceptable for a 
low-level method only visible at package level.


Am 25.09.2015 um 08:46 schrieb Peter Levart :
I have run the tests in java.lang.invoke and only have a problem with 1 test 
which seems to be related to the test or jtreg itself and happens also without 
my patch applied:

#Test Results (version 2)
#Tue Sep 22 09:48:38 CEST 2015
...
#section:script_messages
--messages:(0/0)--


test result: Error. Parse Exception: `@library' must appear before first `@run'

Yes. The test is also marked as ignored until another issue is fixed, so that 
can be ignored.

Other than the above remark/suggestion, this looks fine. I'll defer to an 
upper-case Reviewer, though.

Best,

Michael

--


Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
Oracle is committed to developing 
practices and products that help protect the environment



___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev


Re: RFR: JDK-8136893: Improve early java.lang.invoke infrastructure initialization

2015-09-25 Thread Paul Sandoz
Hi,

This looks like a partial dup of 
https://bugs.openjdk.java.net/browse/JDK-8076596

The changes look ok, but I am concerned post initialization there may be code 
paths taken that require the system class loader to be used but instead the 
boot stream class loader is used instead. Is that a legitimate concern?

Paul.

On 25 Sep 2015, at 15:37, Michael Haupt  wrote:

> Hi Peter,
> 
> thanks for this changeset. Note I'm not a Reviewer (with a capital R); please 
> read this review in lower-case. ;-)
> 
> One question about MethodType: would you mind doing something about the 
> naming of the newly introduced fromDescriptor() method? Its name does not 
> suggest in any way that it chooses the class loader differently. The name is 
> subtly different from that of fromDescriptorString(), and the signature is 
> identical - it's probably really easy to confuse the two when working at the 
> core libs level. Unfortunately, the only proposal for a name I can make, 
> fromDescriptorStringBootCL(), is clunky. Maybe that's acceptable for a 
> low-level method only visible at package level.
> 
>> Am 25.09.2015 um 08:46 schrieb Peter Levart :
>> I have run the tests in java.lang.invoke and only have a problem with 1 test 
>> which seems to be related to the test or jtreg itself and happens also 
>> without my patch applied:
>> 
>> #Test Results (version 2)
>> #Tue Sep 22 09:48:38 CEST 2015
>> ...
>> #section:script_messages
>> --messages:(0/0)--
>> 
>> 
>> test result: Error. Parse Exception: `@library' must appear before first 
>> `@run'
> 
> Yes. The test is also marked as ignored until another issue is fixed, so that 
> can be ignored.
> 
> Other than the above remark/suggestion, this looks fine. I'll defer to an 
> upper-case Reviewer, though.
> 
> Best,
> 
> Michael
> 
> --
> 
> 
> Dr. Michael Haupt | Principal Member of Technical Staff
> Phone: +49 331 200 7277 | Fax: +49 331 200 7561
> Oracle Java Platform Group | LangTools Team | Nashorn
> Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
> Oracle is committed to developing 
> practices and products that help protect the environment
> 



signature.asc
Description: Message signed with OpenPGP using GPGMail
___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev


Re: RFR: JDK-8136893: Improve early java.lang.invoke infrastructure initialization

2015-09-25 Thread Michael Haupt
Hi Peter,

thanks for this changeset. Note I'm not a Reviewer (with a capital R); please 
read this review in lower-case. ;-)

One question about MethodType: would you mind doing something about the naming 
of the newly introduced fromDescriptor() method? Its name does not suggest in 
any way that it chooses the class loader differently. The name is subtly 
different from that of fromDescriptorString(), and the signature is identical - 
it's probably really easy to confuse the two when working at the core libs 
level. Unfortunately, the only proposal for a name I can make, 
fromDescriptorStringBootCL(), is clunky. Maybe that's acceptable for a 
low-level method only visible at package level.

> Am 25.09.2015 um 08:46 schrieb Peter Levart :
> I have run the tests in java.lang.invoke and only have a problem with 1 test 
> which seems to be related to the test or jtreg itself and happens also 
> without my patch applied:
> 
> #Test Results (version 2)
> #Tue Sep 22 09:48:38 CEST 2015
> ...
> #section:script_messages
> --messages:(0/0)--
> 
> 
> test result: Error. Parse Exception: `@library' must appear before first 
> `@run'

Yes. The test is also marked as ignored until another issue is fixed, so that 
can be ignored.

Other than the above remark/suggestion, this looks fine. I'll defer to an 
upper-case Reviewer, though.

Best,

Michael

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany
  Oracle is committed to developing 
practices and products that help protect the environment

___
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev