Re: RFR(XS): 8143127: InvokerBytecodeGenerator emitConst should handle Byte, Short, Character

[forax]

- Mail original -
> De: "Claes Redestad" 
> À: "Remi Forax" 
> Cc: "core-libs-dev@openjdk.java.net Libs" 
> Envoyé: Mardi 17 Novembre 2015 23:08:55
> Objet: Re: RFR(XS): 8143127: InvokerBytecodeGenerator emitConst should handle 
> Byte, Short, Character
> 
> Hi Remi,
> 
> On 2015-11-17 22:13, Remi Forax wrote:
> > Hi Claes,
> > I fail to see how this code will not throw a CCE at runtime
> >if (con instanceof Integer || con instanceof Byte || con instanceof
> >Short || con instanceof Character) {
> >   emitIconstInsn((int) con);
> >   ...
> >
> > (int)con is translated by the compiler to ((Integer)con).intValue()
> >
> > you have to write something like that
> >(con instanceof Character)? (char)con: ((Number)con).intValue()
> 
> Well, this is embarrassing. I was fooled by my own sanity testing since
> javac makes the unboxing+cast work when type is known:
> 
> Character c = 'a';
> System.out.println((int)c);
> 
> but not when going through an Object reference:
> 
> Object o = 'a';
> System.out.println((int)o);
> 
> This works better:
> 
>  if (con instanceof Integer) {
>  emitIconstInsn((int) con);
>  return;
>  }
>  if (con instanceof Byte) {
>  emitIconstInsn((int)(byte)con);
>  return;
>  }
>  if (con instanceof Short) {
>  emitIconstInsn((int)(short)con);
>  return;
>  }
>  if (con instanceof Character) {
>  emitIconstInsn((int)(char)con);
>  return;
>  }
> 
> Updated in-place.
> 
> /Claes
> 

I agree, far more readable :)

Rémi

Re: Proposed API for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 17, 2015, at 1:13 PM, Peter Levart  wrote:
> 
> But (as described in my other message), Runnable::run is not an entry point. 
> Thread::run is. And Thread::run (a Java method) delegates to Runnable::run. 
> So in this case Thread.class will be returned as a normal caller (which it 
> really is). Are you thinking of detecting this situation and special-casing 
> it?

Thanks for the feedback.   I am in the process of clarifying that the javadoc 
and I see the confusion there.  This is the CallerFromMain.java test (some 
comments need to be cleaned up) but it shows what the getCallerClass is 
intended to return.  

1) call from main - throw UOE

// StackWalker::getCallerClass
// CallerFromMain::main
// no caller

2) return Runnable anonymous class

// StackWalker::getCallerClass
// Runnable::run
// Thread::run

3) return MyRunnable

// StackWalker::getCallerClass
// MyRunnable::run
// Thread::run

4) return MyThread::run 

// StackWalker::getCallerClass
// Runnable::run
// MyThread::run

5) return CallerFromMain

// StackWalker::getCallerClass
// CallerFromMain::doit
// Thread::run

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

Hi Alan,

Thanks for looking at this.

>> Please review the new webrev that addresses the issues raised by Sherman and 
>> Alan in the last iteration.  In particular:
>> - fixed the race condition in isMultiRelease() and another one with the 
>> variables ‘version’ and ‘configured’
>> - changed the fragment for JarURLConnection runtime versioning from 
>> ‘rtversioned’ to ‘runtime’, and created documentation for it
>> - used try with resources to open JarFile in all the tests
>> 
>> Issue:  
>> https://bugs.openjdk.java.net/browse/JDK-8132734
>>   
>> JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305 
>>  
>> Webrev: http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/ 
>>  
>> 
> The updated webrev looks must better. In JarURLConnection then it would be 
> good if the reference to multi-release JARs should link to the description in 
> the JarFile spec.

Sure, just put a “see JarFile” comment in?

> 
> In the previous round then we were discussing renaming the 
> jdk.util.jar.multirelease property. Has there been any more on that?

Less of a discussion than a suggestion ;-)  I didn’t change it because I wanted 
to see how important it was — I figured if you didn’t comment it was okay.  But 
you did comment, so I guess it’s important.  I’ll change it.

> 
> The test MultiReleaseJarURLConnection uses @library 
> /lib/testlibrary/java/util/jar so it's reaching across the file system to use 
> the infrastructure of the JarFile tests. It might be clearer to move the test 
> to the JarFile directory.

I can do that.  I didn’t think that was the right directory to put it in.  
Seems like it should be with the other JarURLConnection tests, that way I could 
just run jtreg on the directory and get all the JarURLConnection tests run, 
including the multi-release jar test.

> 
> It would be nice if we could reduce some of the really long lines if 
> possible, just to make future side-by-side a bit easier (avoid horizontal 
> scrolling).

I’ll try.

> 
> -Alan.

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

On Nov 17, 2015, at 4:00 PM, Ian Rogers  wrote:
> 
> Should the StackWalker.StackFrame interface provide a way to get the 
> java.lang.reflect.Method/Constructor/Member?
> 

Not in the scope of this JEP.

Mandy

> Thanks,
> Ian
> 
> On Tue, Nov 17, 2015 at 3:56 PM, Mandy Chung  wrote:
> Thanks Peter.
> 
> > - threre's no ResourceBundle.getBundle(String, ClassLoader) method.
> > - Util -> ResourceBundleUtil (or ResourceBundleUtil -> Util)
> >
> 
> Fixed.
> 
> > :
> > - Stream.findFirst() returns Optional, not E.
> >
> 
> Fixed.
> 
> I updated javadoc for getCallerClass per our discussion.
> 
> /**
>  * Gets the {@code Class} object of the caller invoking the method
>  * that calls this {@code getCallerClass} method.
>  *
>  *  Reflection frames, {@link java.lang.invoke.MethodHandle} and
>  * hidden frames are filtered regardless of the
>  * {@link Option#SHOW_REFLECT_FRAMES SHOW_REFLECT_FRAMES}
>  * and {@link Option#SHOW_HIDDEN_FRAMES SHOW_HIDDEN_FRAMES} options
>  * if this {@code StackWalker} has been configured.
>  *
>  *  This method throws {@code UnsupportedOperationException} if
>  * this {@code StackWalker} is not configured with
>  * {@link Option#RETAIN_CLASS_REFERENCE RETAIN_CLASS_REFERENCE} option
>  * or this method is called from the last frame on the stack.
>  *
>  * @apiNote
>  * For example, {@code Util::getResourceBundle} loads a resource bundle
>  * on behalf of the caller.  It calls this {@code getCallerClass} method
>  * to find the method calling {@code Util::getResourceBundle} and use the 
> caller's
>  * class loader to load the resource bundle. The caller class in this example
>  * is the {@code MyTool} class.
>  *
>  * {@code
>  * class Util {
>  * private final StackWalker walker = 
> StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
>  * public ResourceBundle getResourceBundle(String bundleName) {
>  * Class caller = walker.getCallerClass();
>  * return ResourceBundle.getBundle(bundleName, 
> Locale.getDefault(), caller.getClassLoader());
>  * }
>  * }
>  *
>  * class MyTool {
>  * private final Util util = new Util();
>  * private void init() {
>  * ResourceBundle rb = util.getResourceBundle("mybundle");
>  * }
>  * }
>  * }
>  *
>  * An equivalent way to find the caller class using the
>  * {@link StackWalker#walk walk} method is as follows
>  * (filtering the reflection frames, {@code MethodHandle} and hidden frames
>  * not shown below):
>  * {@code
>  * Optional> caller = walker.walk(s ->
>  * s.map(StackFrame::getDeclaringClass)
>  *  .skip(2)
>  *  .findFirst());
>  * }
>  *
>  * When the {@code getCallerClass} method is called from a method that
>  * is the last frame on the stack,
>  * for example, {@code static public void main} method launched by the
>  * {@code java} launcher or a method invoked from a JNI attached thread.
>  * {@code UnsupportedOperationException} is thrown.
>  *
>  * @return {@code Class} object of the caller's caller invoking this method.
>  *
>  * @throws UnsupportedOperationException if this {@code StackWalker}
>  * is not configured with {@link Option#RETAIN_CLASS_REFERENCE
>  * Option.RETAIN_CLASS_REFERENCE}.
>  * @throws UnsupportedOperationException if there is no caller frame, i.e.
>  * when this {@code getCallerClass} method is called from a method
>  * which is the last frame on the stack.
>  */
> 
> Mandy
> 

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

Thanks Peter.  

> - threre's no ResourceBundle.getBundle(String, ClassLoader) method.
> - Util -> ResourceBundleUtil (or ResourceBundleUtil -> Util)
> 

Fixed.

> :
> - Stream.findFirst() returns Optional, not E.
> 

Fixed.

I updated javadoc for getCallerClass per our discussion.

/**
 * Gets the {@code Class} object of the caller invoking the method
 * that calls this {@code getCallerClass} method.
 *
 *  Reflection frames, {@link java.lang.invoke.MethodHandle} and
 * hidden frames are filtered regardless of the
 * {@link Option#SHOW_REFLECT_FRAMES SHOW_REFLECT_FRAMES}
 * and {@link Option#SHOW_HIDDEN_FRAMES SHOW_HIDDEN_FRAMES} options
 * if this {@code StackWalker} has been configured.
 *
 *  This method throws {@code UnsupportedOperationException} if
 * this {@code StackWalker} is not configured with
 * {@link Option#RETAIN_CLASS_REFERENCE RETAIN_CLASS_REFERENCE} option
 * or this method is called from the last frame on the stack.
 *
 * @apiNote
 * For example, {@code Util::getResourceBundle} loads a resource bundle
 * on behalf of the caller.  It calls this {@code getCallerClass} method
 * to find the method calling {@code Util::getResourceBundle} and use the 
caller's
 * class loader to load the resource bundle. The caller class in this example
 * is the {@code MyTool} class.
 *
 * {@code
 * class Util {
 * private final StackWalker walker = 
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
 * public ResourceBundle getResourceBundle(String bundleName) {
 * Class caller = walker.getCallerClass();
 * return ResourceBundle.getBundle(bundleName, Locale.getDefault(), 
caller.getClassLoader());
 * }
 * }
 *
 * class MyTool {
 * private final Util util = new Util();
 * private void init() {
 * ResourceBundle rb = util.getResourceBundle("mybundle");
 * }
 * }
 * }
 *
 * An equivalent way to find the caller class using the
 * {@link StackWalker#walk walk} method is as follows
 * (filtering the reflection frames, {@code MethodHandle} and hidden frames
 * not shown below):
 * {@code
 * Optional> caller = walker.walk(s ->
 * s.map(StackFrame::getDeclaringClass)
 *  .skip(2)
 *  .findFirst());
 * }
 *
 * When the {@code getCallerClass} method is called from a method that
 * is the last frame on the stack,
 * for example, {@code static public void main} method launched by the
 * {@code java} launcher or a method invoked from a JNI attached thread.
 * {@code UnsupportedOperationException} is thrown.
 *
 * @return {@code Class} object of the caller's caller invoking this method.
 *
 * @throws UnsupportedOperationException if this {@code StackWalker}
 * is not configured with {@link Option#RETAIN_CLASS_REFERENCE
 * Option.RETAIN_CLASS_REFERENCE}.
 * @throws UnsupportedOperationException if there is no caller frame, i.e.
 * when this {@code getCallerClass} method is called from a method
 * which is the last frame on the stack.
 */

Mandy

Re: Proposed API for JEP 259: Stack-Walking API

[Peter Levart]

On 11/17/2015 09:50 PM, Mandy Chung wrote:

On Nov 17, 2015, at 11:54 AM, Peter Levart  wrote:



On 11/16/2015 08:16 PM, Mandy Chung wrote:

On Nov 15, 2015, at 10:59 AM, Peter Levart 
  wrote:

OTOH in the described cases, a caller of walker.getCallerClass() is actually expecting to be called 
by a Java method, right? So what would it be if such "caller-sensitive" method demanded 
to be called by a Java method and throw IllegalArgumentException("Not called by Java 
method") otherwise?

Have you thought of that possibility?


Are you thinking about a JNI method calling getCallerClass?  Or refer 
getCallerClass being the bottom of the stack?

I don’t see any issue for a JNI method calling getCallerClass and it’s doing:
* Class caller = walker.walk(s ->
* s.map(StackFrame::getDeclaringClass)
*  .skip(2)
*  .findFirst());

Mandy


Hi Mandy,

No, I was not thinking of StackWalker::getCallerClass being called by JNI. I was thinking of a 
"caller-sensitive" method calling getCallerClass() and that "caller-sensitive" 
method being called by JNI from a newly attached thread.

1st, I don't think anyone will attempt calling getCallerClass() from the static 
main() method invoked by java launcher. That's silly and need not be supported.
2nd, I don't think anyone will attempt calling getCallerClass() from overridden 
Thread::run() method. That too need not be supported.

So what we are left with are other "caller-sensitive" methods calling 
getCallerClass(). Like for example:

public class Strings {
 public static ResourceBundle getBundle() {
 Class cc = stackWalker.getCallerClass();
 return ResourceBundle.getBundle(cc.getName() + "$Strings", 
Locale.getDefault(), cc.getClassLoader());
 }
}

Such method obviously expects to be called by some Java method. So it may as 
well demand it. By throwing exception if it is called by JNI from newly 
attached thread.

"caller-sensitive" methods could be viewed as taking an implicit parameter - their 
caller. If that parameter is not specified, they could behave like when some other explicit 
parameter was not specified or invalid - throw new IllegalArgumentException("Not called by 
Java method") …


I thought about this case and I consider it the same category as the static 
main method case which is a method being invoked by newly JNI attached thread 
where the entry point is the last frame on the stack.


Right.


   I think common cases would have the entry point to invoke other java methods 
including caller-sensitive ones.


Yes, and that's not a problem, since caller-sensitive methods are 
invoked by a Java method in this case (be it entry-point or not, it 
doesn't matter).



I see your point of those methods being abused.

The other alternative I considered is throwing UnsupportedOperationException if 
there is no caller frame, i.e. skip(2) doesn’t exist.  I agree that it’s a 
better option than returning (n-1)-th frame if it’s the last frame on the stack.


Yes, UnsupportedOperationException is maybe even better (not many would 
think of caller as being an implicit argument).



I will keep returning the thread’s entry point case to return the class of the 
runnable instead of returning Thread.class.


But (as described in my other message), Runnable::run is not an entry 
point. Thread::run is. And Thread::run (a Java method) delegates to 
Runnable::run. So in this case Thread.class will be returned as a normal 
caller (which it really is). Are you thinking of detecting this 
situation and special-casing it?


Regards, Peter


Mandy


So instead of "pretending" they were called by themselves, which might not be 
right, simply refuse the invocation with exception. I don't think this would hinder any 
such caller-sensitive method in it's utility. More importantly, it would prevent such 
methods from being abused.
You said about possible use-cases for getCallerClass:


I have been thinking what the users would do when there is no caller.

The JDK use  of getCallerClass are to:
1. find the caller's class loader for class loader hierarchy check

Abuse: calling such "caller-sensitive" method by JNI from newly-attached thread 
would use the caller-sensitive method's declaring class ClassLoader for hierarchy check - 
wrong!


2. find the caller’s class loader to load something on behalf of the caller 
(visibility)

Abuse: ... would use the caller-sensitive method's declaring class ClassLoader 
to load something - might work, might not


3. looking forward to work with modules, one would want to get the caller’s 
module
- to load something on behalf of the caller's module

Abuse: ...would load something from caller-sensitive method's declaring class 
module - wrong.


- to add read edge to the caller’s module before accessing its type

Abuse: ...would add a read edge to itself before accessing some type - wrong.


In all those cases throwing exception would be the right thing to do.


What do you think?

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Remi Forax]

Hi Konstantin,
Technically, getInvokedynamicRefInfoAt should be 
getNameAndTypeOfInvokedynamicRefInfoAt because it only extract the nameAndType 
value of the InvokeDynamicRef.

In term of API, I think it's better to decompose the API, i.e. to have a method
  int getInvokedynamicRefNameAndTypeIndex(int index)
that returns the nameAndType index and to reuse getNameAndTypeRefInfoAt(index) 
to get the corresponding array of Strings. 

cheers,
Rémi

- Mail original -
> De: "Christian Thalinger" 
> À: "Konstantin Shefov" 
> Cc: "hotspot-dev developers" , 
> core-libs-dev@openjdk.java.net
> Envoyé: Lundi 16 Novembre 2015 23:41:45
> Objet: Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool
> 
> [CC'ing core-libs-dev]
> 
> > On Nov 13, 2015, at 4:55 AM, Konstantin Shefov
> >  wrote:
> > 
> > Hello
> > 
> > Please review an enhancement to add three new methods to
> > sun.reflect.ConstantPool class.
> > The following methods are suggested:
> > 
> > public String[] getNameAndTypeRefInfoAt(int index) - returns string
> > representation of name and type from a NameAndType constant pool entry
> > with the specified index
> > 
> > public String[] getInvokedynamicRefInfoAt(int index) - returns string
> > representation of name and type from an InvokeDynamic constant pool entry
> > with the specified index
> > 
> > public Tag getTagAt(int index) - returns a Tag enum instance of a constant
> > pool entry with the specified index
> > 
> > These three methods could be used for testing API working with constant
> > pool, e.g. JVMCI.
> > 
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8141615
> > Webrev hotspot:
> > http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.00
> > Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00
> > 
> > Thanks
> > -Konstantin
> 
> 

Re: Code Review for JEP 259: Stack-Walking API

[Peter Levart]

Hi Mandy,

On 11/17/2015 01:13 AM, Mandy Chung wrote:

I’d like to get the code review done by this week.

I renamed the static factory method from create to getInstance since “create” 
implies to create a new instance but the method returns a cached instance 
instead.  I also changed the spec of getCallerClass per [1].  There is not much 
change since webrev.01.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02

javadoc:
http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/

Mandy
[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036589.html


Just read the javadoc so-far...

There are several mistakes in getCallerClass() API Note (some probably a 
left-over from previous iterations of the API):


 471  *  If this {@code getCallerClass} method is called by the 
entry point
 472  * of a thread, the {@code Class} of the method invoked at 
thread's start

 473  * time will be returned.

Hm, I can't decipher that. What about something like:

If this {@code getCallerClass} method is called by the entry point of a 
thread - a method overriding {@link Thread#run} (that's the only 
possibility),
the declaring class of the method overriding {@link Thread#run} will be 
returned.


Or, if you accept my latest suggestion:

If this {@code getCallerClass} method is called by the entry point of a 
thread - a method overriding {@link Thread#run},

{@link IllegalArgumentException} will be thrown.


 474  *
 475  * @apiNote
 476  * For example, {@code ResourceBundleUtil::getBundle} loads a 
resource bundle
 477  * on behalf of the caller.  It calls this {@code 
getCallerClass} method
 478  * to find the method calling {@code Util::getResourceBundle} 
and use the caller's
 479  * class loader to load the resource bundle. The caller class 
in this example

 480  * is the {@code MyTool} class.
 481  *
 482  * {@code
 483  * class ResourceBundleUtil {
 484  * private final StackWalker walker = 
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);

 485  * public ResourceBundle getBundle(String bundleName) {
 486  * Class caller = walker.getCallerClass();
 487  * return ResourceBundle.getBundle(bundleName, 
caller.getClassLoader());

 488  * }
 489  * }
 490  *
 491  * class MyTool {
 492  * private void init() {
 493  * ResourceBundle rb = 
Util.getResourceBundle("mybundle");

 494  * }
 495  * }
 496  * }

- threre's no ResourceBundle.getBundle(String, ClassLoader) method.
- Util -> ResourceBundleUtil (or ResourceBundleUtil -> Util)


 497  *
 498  * An equivalent way to find the caller class using the
 499  * {@link StackWalker#walk walk} method is as follows
 500  * (filtering the reflection frames, {@code MethodHandle} and 
hidden frames

 501  * not shown below):
 502  * {@code
 503  * Class caller = walker.walk(s ->
 504  * s.map(StackFrame::getDeclaringClass)
 505  *  .skip(2)
 506  *  .findFirst());
 507  * }

- Stream.findFirst() returns Optional, not E.

 508  *
 509  * When the {@code getCallerClass} method is invoked from 
thread {@code t}'s
 510  * entry point, i.e. {@code PrimeRun::run}, it returns {@code 
PrimeRun} class
 511  * that is the first stack frame below the stack frame for 
{@code getCallerClass}

 512  * instead.
 513  *
 514  * {@code
 515  * class PrimeRun implements Runnable {
 516  * public void run() {
 517  * Class c = 
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE)

 518  * .getCallerClass();
 519  * }
 520  * }
 521  * Thread t = new Thread(new PrimeRun()).start();
 522  * }
 523  *
 524  * Similarly, when the {@code getCallerClass} method is called 
from the
 525  * {@code static public void main} method launched by the 
{@code java} launcher,

 526  * it returns the {@code Class} of the {@code main} method.
 527  *
 528  * @return {@code Class} object of the caller's caller 
invoking this method;
 529  * or the {@code Class} object of the method invoked by a 
thread at start time.


- In above example, t's entry point is Thread::run (not PrimeRun::run). 
Thread::run then delegates to PrimeRun::run:


public class Thread {
...
public void run() {
if (target != null) {
target.run();
}
}


...so this example is not really suitable to describe the effect of 
invoking getCallerClass from thread's entry-point.


An example of that kind would be something like:

class PrimeThread extends Thread {
@Override public void run() {
Class c = StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE)
.getCallerClass();
}
}

Thread t = new 

Re: Proposed API for JEP 259: Stack-Walking API

[Mandy Chung]

> Apart from the orphaned paragraph fragment at the end looks good to me, but 
> that’s just my opinion.

I caught that that after I clicked sent :( 

This is a better version.

/**
 * Gets the {@code Class} object of the caller invoking the method
 * that calls this {@code getCallerClass} method.
 *
 *  Reflection frames, {@link java.lang.invoke.MethodHandle} and
 * hidden frames are filtered regardless of the
 * {@link Option#SHOW_REFLECT_FRAMES SHOW_REFLECT_FRAMES}
 * and {@link Option#SHOW_HIDDEN_FRAMES SHOW_HIDDEN_FRAMES} options
 * if this {@code StackWalker} has been configured.
 *
 *  This method throws {@code UnsupportedOperationException} if
 * this {@code StackWalker} is not configured with
 * {@link Option#RETAIN_CLASS_REFERENCE RETAIN_CLASS_REFERENCE} option
 * or this method is called from the last frame on the stack.
 *
 * @apiNote
 * For example, {@code Util::getResourceBundle} loads a resource bundle
 * on behalf of the caller.  It calls this {@code getCallerClass} method
 * to find the method calling {@code Util::getResourceBundle} and use the 
caller's
 * class loader to load the resource bundle. The caller class in this example
 * is the {@code MyTool} class.
 *
 * {@code
 * class Util {
 * private final StackWalker walker = 
StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
 * public ResourceBundle getResourceBundle(String bundleName) {
 * Class caller = walker.getCallerClass();
 * return ResourceBundle.getBundle(bundleName, Locale.getDefault(), 
caller.getClassLoader());
 * }
 * }
 *
 * class MyTool {
 * private final Util util = new Util();
 * private void init() {
 * ResourceBundle rb = util.getResourceBundle("mybundle");
 * }
 * }
 * }
 *
 * An equivalent way to find the caller class using the
 * {@link StackWalker#walk walk} method is as follows
 * (filtering the reflection frames, {@code MethodHandle} and hidden frames
 * not shown below):
 * {@code
 * Optional> caller = walker.walk(s ->
 * s.map(StackFrame::getDeclaringClass)
 *  .skip(2)
 *  .findFirst());
 * }
 *
 * When the {@code getCallerClass} method is called from a method that
 * is the last frame on the stack,
 * for example, {@code static public void main} method launched by the
 * {@code java} launcher or a method invoked from a JNI attached thread.
 * {@code UnsupportedOperationException} is thrown.
 *
 * @return {@code Class} object of the caller's caller invoking this method.
 *
 * @throws UnsupportedOperationException if this {@code StackWalker}
 * is not configured with {@link Option#RETAIN_CLASS_REFERENCE
 * Option.RETAIN_CLASS_REFERENCE}.
 * @throws UnsupportedOperationException if there is no caller frame, i.e.
 * when this {@code getCallerClass} method is called from a method
 * which is the last frame on the stack.
 */

Mandy

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Joel Borggrén-Franck]

Looks good to me.

In an unmodularized world I would think twice before adding the
hotspot specific tags.

(I'm not a HotSpot Reviewer).

cheers
/Joel

On Mon, Nov 16, 2015 at 11:41 PM, Christian Thalinger
 wrote:
> [CC'ing core-libs-dev]
>
>> On Nov 13, 2015, at 4:55 AM, Konstantin Shefov 
>>  wrote:
>>
>> Hello
>>
>> Please review an enhancement to add three new methods to 
>> sun.reflect.ConstantPool class.
>> The following methods are suggested:
>>
>> public String[] getNameAndTypeRefInfoAt(int index) - returns string 
>> representation of name and type from a NameAndType constant pool entry with 
>> the specified index
>>
>> public String[] getInvokedynamicRefInfoAt(int index) - returns string 
>> representation of name and type from an InvokeDynamic constant pool entry 
>> with the specified index
>>
>> public Tag getTagAt(int index) - returns a Tag enum instance of a constant 
>> pool entry with the specified index
>>
>> These three methods could be used for testing API working with constant 
>> pool, e.g. JVMCI.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8141615
>> Webrev hotspot: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.00
>> Webrev jdk: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.00
>>
>> Thanks
>> -Konstantin
>

Re: Code Review for JEP 259: Stack-Walking API

[Daniel Fuchs]

On 17/11/15 01:13, Mandy Chung wrote:

I’d like to get the code review done by this week.

I renamed the static factory method from create to getInstance since “create” 
implies to create a new instance but the method returns a cached instance 
instead.  I also changed the spec of getCallerClass per [1].  There is not much 
change since webrev.01.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02

javadoc:
http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/

Mandy
[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036589.html



Looks good to me Mandy.
Though I wonder whether we really need this optimization.

best regards,

-- daniel

Re: RFR(XS): 8143127: InvokerBytecodeGenerator emitConst should handle Byte, Short, Character

[Claes Redestad]

Hi Remi,

On 2015-11-17 22:13, Remi Forax wrote:

Hi Claes,
I fail to see how this code will not throw a CCE at runtime
   if (con instanceof Integer || con instanceof Byte || con instanceof Short || 
con instanceof Character) {
  emitIconstInsn((int) con);
  ...

(int)con is translated by the compiler to ((Integer)con).intValue()

you have to write something like that
   (con instanceof Character)? (char)con: ((Number)con).intValue()


Well, this is embarrassing. I was fooled by my own sanity testing since 
javac makes the unboxing+cast work when type is known:


Character c = 'a';
System.out.println((int)c);

but not when going through an Object reference:

Object o = 'a';
System.out.println((int)o);

This works better:

if (con instanceof Integer) {
emitIconstInsn((int) con);
return;
}
if (con instanceof Byte) {
emitIconstInsn((int)(byte)con);
return;
}
if (con instanceof Short) {
emitIconstInsn((int)(short)con);
return;
}
if (con instanceof Character) {
emitIconstInsn((int)(char)con);
return;
}

Updated in-place.

/Claes

Re: Proposed API for JEP 259: Stack-Walking API

[Peter Levart]

On 11/17/2015 10:13 PM, Peter Levart wrote:



I will keep returning the thread’s entry point case to return the class of the 
runnable instead of returning Thread.class.


But (as described in my other message), Runnable::run is not an entry 
point. Thread::run is. And Thread::run (a Java method) delegates to 
Runnable::run. So in this case Thread.class will be returned as a 
normal caller (which it really is). Are you thinking of detecting this 
situation and special-casing it?


Regards, Peter



I think this is not a good idea.

1st it's difficult to reliably detect this situation. For example:

Runnable r = ...;



new Thread(r).start();

vs.

class MyThread extends Thread {
MyThread(Runnable r) {
super(r);
}
@Override public void run() {
// .. some setup
super.run();
}
}

new MyThread(r).start();


2nd why would this be limited to Thread delegate? What about:

Executable exe = Executables.newFixedThreadPool(...);

exe.execute(r);

...and how would you detect that?



I think that calling getCallerClass() from implementation of 
Runnable::run should expect it to return a system class. It may be 
Thread.class or ThreadPoolExecutor$Worker.class or anything actually.



Regards, Peter

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

Hi Sherman,

Thanks for looking at this.  Comments in-line below.

> On Nov 17, 2015, at 9:49 AM, Xueming Shen  wrote:
> 
> On 11/11/15 8:44 AM, Steve Drach wrote:
>> Hi,
>> 
>> Please review the new webrev that addresses the issues raised by Sherman and 
>> Alan in the last iteration.  In particular:
>> - fixed the race condition in isMultiRelease() and another one with the 
>> variables ‘version’ and ‘configured’
>> - changed the fragment for JarURLConnection runtime versioning from 
>> ‘rtversioned’ to ‘runtime’, and created documentation for it
>> - used try with resources to open JarFile in all the tests
>> 
>> Issue:  
>> https://bugs.openjdk.java.net/browse/JDK-8132734
>>   
>> JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305 
>>  
>> Webrev: http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/ 
>>  
>> 
>> 
> 
> Hi Steve,
> 
> Seems like the "version" is now a little confused.

More likely it’s my confusion.

> 
> (1) getVersioned() says "or 0 if this jar file is processed as if it is an 
> unversioned or is
> not a multi-release jar file". The implementation now returns 8  
> (BASE_VERSION is
> 8 now) if not a multi-release jar (?).

Yep.  It should return 0 if version <= BASE_VERSION

> 
> (2) setVersioned() says "If the specified version is 0 then this is 
> configured to be an
> unversioned jar file". The implementation seems treat anything < BASE_VERSION 
> as
> such? Sure, technically it's still correct. Given the BASE_VERSION is a 
> private field, any
> thing between 0 and BASE_VERSION becomes unspecified gray area.

As you say, it’s technically correct.  We expect people will set version to 0 
if they want unversioned processing.  BASE_VERSION is not something clients 
should be concerned with, or even know about.  So, yes, they can set versions 
to something less than BASE_VERSION but it doesn’t really matter, we know what 
they mean even if they don’t ;-)  It’s a bit of a loose interpretation of the 
spec that doesn’t cause any harm.  It doesn’t seem as useful if we strictly 
enforce this by throw an IllegalArgumentException for example.


> 
> (3) getRuntimeVersionedEntry() spec says "If the JarFile is a multi-release 
> jar file and is
> configured to be processed as such, then ...". However the implementation
> 
> if (RUNTIME_VERSION > BASE_VERSION && !name.startsWith(META_INF) && 
> isMultiRelease()) {
>   ...
> }
> 
> does not appears to check the logic "is configured to be …"?

Yes, clearly an error in the spec.

> 
> And
> 
>  391 if (manifestRead) {
>  392 return isMultiRelease;
>  393 }
>  394 synchronized (this) {
>  395 // this needs to be set prior to getManifest() call to 
> prevent recursive loop
>  396 manifestRead = true;
>  397 try {
>  398 Manifest man = getManifest();
>  399 isMultiRelease = (man != null) && 
> man.getMainAttributes().containsKey(MULTI_RELEASE);
>  400 } catch (IOException x) {
>  401 isMultiRelease = false;
>  402 }
>  403 }
>  404 return isMultiRelease;
> don't we have a race condition if #391/2 is outside the synchronized block? 
> for sample, one
> thread is inside sync block and set the manifestRead = true, but before 
> isMultiRelease = true/false,
> and the second thread gets to #391.

You’re right.  once upon a time, there was a third boolean to deal with the 
potential loop, but I took it out and tried to overload manifestRead with dual 
meaning.  I need to put the third variable back in.  This is a surprisingly 
complex method, hard to get right.

> 
> thanks,
> -Sherman
> 

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Xueming Shen]

On 11/11/15 8:44 AM, Steve Drach wrote:

Hi,

Please review the new webrev that addresses the issues raised by 
Sherman and Alan in the last iteration.  In particular:
- fixed the race condition in isMultiRelease() and another one with 
the variables ‘version’ and ‘configured’
- changed the fragment for JarURLConnection runtime versioning from 
‘rtversioned’ to ‘runtime’, and created documentation for it

- used try with resources to open JarFile in all the tests

Issue: https://bugs.openjdk.java.net/browse/JDK-8132734
JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305
Webrev: 
http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/ 






Hi Steve,

Seems like the "version" is now a little confused.

(1) getVersioned() says "or 0 if this jar file is processed as if it is 
an unversioned or is
not a multi-release jar file". The implementation now returns 8 
(BASE_VERSION is

8 now) if not a multi-release jar (?).

(2) setVersioned() says "If the specified version is 0 then this is 
configured to be an
unversioned jar file". The implementation seems treat anything < 
BASE_VERSION as
such? Sure, technically it's still correct. Given the BASE_VERSION is a 
private field, any

thing between 0 and BASE_VERSION becomes unspecified gray area.

(3) getRuntimeVersionedEntry() spec says "If the JarFile is a 
multi-release jar file and is

configured to be processed as such, then ...". However the implementation

if (RUNTIME_VERSION > BASE_VERSION && !name.startsWith(META_INF) && 
isMultiRelease()) {

  ...
}

does not appears to check the logic "is configured to be ..."?

And

391 if (manifestRead) {
392 return isMultiRelease;
393 }
394 synchronized (this) {
395 // this needs to be set prior to getManifest() call to prevent 
recursive loop

396 manifestRead = true;
397 try {
398 Manifest man = getManifest();
399 isMultiRelease = (man != null) && 
man.getMainAttributes().containsKey(MULTI_RELEASE);

400 } catch (IOException x) {
401 isMultiRelease = false;
402 }
403 }
404 return isMultiRelease;

don't we have a race condition if #391/2 is outside the synchronized 
block? for sample, one
thread is inside sync block and set the manifestRead = true, but before 
isMultiRelease = true/false,

and the second thread gets to #391.

thanks,
-Sherman

Re: RFR(XS): 8143127: InvokerBytecodeGenerator emitConst should handle Byte, Short, Character

[Remi Forax]

Hi Claes,
I fail to see how this code will not throw a CCE at runtime
  if (con instanceof Integer || con instanceof Byte || con instanceof Short || 
con instanceof Character) {
 emitIconstInsn((int) con); 
 ...

(int)con is translated by the compiler to ((Integer)con).intValue()

you have to write something like that
  (con instanceof Character)? (char)con: ((Number)con).intValue()

cheers,
Rémi

- Mail original -
> De: "Claes Redestad" 
> À: "core-libs-dev@openjdk.java.net Libs" 
> Envoyé: Mardi 17 Novembre 2015 17:17:50
> Objet: RFR(XS): 8143127: InvokerBytecodeGenerator emitConst should handle 
> Byte, Short, Character
> 
> Hi,
> 
> this small patch allows InvokerBytecodeGenerator to generate more
> compact bytecode for small byte, short and char constants:
> 
> webrev: http://cr.openjdk.java.net/~redestad/8143127/webrev.01/
> bug: https://bugs.openjdk.java.net/browse/JDK-8143127
> 
> /Claes
> 

Re: Proposed API for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 17, 2015, at 11:54 AM, Peter Levart  wrote:
> 
> 
> 
> On 11/16/2015 08:16 PM, Mandy Chung wrote:
>>> On Nov 15, 2015, at 10:59 AM, Peter Levart 
>>>  wrote:
>>> 
>>> OTOH in the described cases, a caller of walker.getCallerClass() is 
>>> actually expecting to be called by a Java method, right? So what would it 
>>> be if such "caller-sensitive" method demanded to be called by a Java method 
>>> and throw IllegalArgumentException("Not called by Java method") otherwise?
>>> 
>>> Have you thought of that possibility?
>>> 
>> Are you thinking about a JNI method calling getCallerClass?  Or refer 
>> getCallerClass being the bottom of the stack?
>> 
>> I don’t see any issue for a JNI method calling getCallerClass and it’s doing:
>> * Class caller = walker.walk(s ->
>> * s.map(StackFrame::getDeclaringClass)
>> *  .skip(2)
>> *  .findFirst());
>> 
>> Mandy
>> 
> 
> Hi Mandy,
> 
> No, I was not thinking of StackWalker::getCallerClass being called by JNI. I 
> was thinking of a "caller-sensitive" method calling getCallerClass() and that 
> "caller-sensitive" method being called by JNI from a newly attached thread. 
> 
> 1st, I don't think anyone will attempt calling getCallerClass() from the 
> static main() method invoked by java launcher. That's silly and need not be 
> supported.
> 2nd, I don't think anyone will attempt calling getCallerClass() from 
> overridden Thread::run() method. That too need not be supported.
> 
> So what we are left with are other "caller-sensitive" methods calling 
> getCallerClass(). Like for example:
> 
> public class Strings {
> public static ResourceBundle getBundle() {
> Class cc = stackWalker.getCallerClass();
> return ResourceBundle.getBundle(cc.getName() + "$Strings", 
> Locale.getDefault(), cc.getClassLoader());
> }
> }
> 
> Such method obviously expects to be called by some Java method. So it may as 
> well demand it. By throwing exception if it is called by JNI from newly 
> attached thread.
> 
> "caller-sensitive" methods could be viewed as taking an implicit parameter - 
> their caller. If that parameter is not specified, they could behave like when 
> some other explicit parameter was not specified or invalid - throw new 
> IllegalArgumentException("Not called by Java method") …
> 

I thought about this case and I consider it the same category as the static 
main method case which is a method being invoked by newly JNI attached thread 
where the entry point is the last frame on the stack.  I think common cases 
would have the entry point to invoke other java methods including 
caller-sensitive ones.

I see your point of those methods being abused.

The other alternative I considered is throwing UnsupportedOperationException if 
there is no caller frame, i.e. skip(2) doesn’t exist.  I agree that it’s a 
better option than returning (n-1)-th frame if it’s the last frame on the stack.

I will keep returning the thread’s entry point case to return the class of the 
runnable instead of returning Thread.class.

Mandy

> So instead of "pretending" they were called by themselves, which might not be 
> right, simply refuse the invocation with exception. I don't think this would 
> hinder any such caller-sensitive method in it's utility. More importantly, it 
> would prevent such methods from being abused.

> 
> You said about possible use-cases for getCallerClass:
> 
>> I have been thinking what the users would do when there is no caller.
>> 
>> The JDK use  of getCallerClass are to:
>> 1. find the caller's class loader for class loader hierarchy check
> 
> Abuse: calling such "caller-sensitive" method by JNI from newly-attached 
> thread would use the caller-sensitive method's declaring class ClassLoader 
> for hierarchy check - wrong!
> 
>> 2. find the caller’s class loader to load something on behalf of the caller 
>> (visibility)
> 
> Abuse: ... would use the caller-sensitive method's declaring class 
> ClassLoader to load something - might work, might not
> 
>> 3. looking forward to work with modules, one would want to get the caller’s 
>> module
>> - to load something on behalf of the caller's module
> 
> Abuse: ...would load something from caller-sensitive method's declaring class 
> module - wrong.
> 
>> - to add read edge to the caller’s module before accessing its type
> 
> Abuse: ...would add a read edge to itself before accessing some type - wrong.
> 
> 
> In all those cases throwing exception would be the right thing to do.
> 
> 
> What do you think?

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

> On Nov 17, 2015, at 10:42 AM, Daniel Fuchs  wrote:
> 
> On 17/11/15 01:13, Mandy Chung wrote:
>> I’d like to get the code review done by this week.
>> 
>> I renamed the static factory method from create to getInstance since 
>> “create” implies to create a new instance but the method returns a cached 
>> instance instead.  I also changed the spec of getCallerClass per [1].  There 
>> is not much change since webrev.01.
>> 
>> Webrev:
>>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02
>> 
>> javadoc:
>>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/
>> 
>> Mandy
>> [1] 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036589.html
>> 
> 
> Looks good to me Mandy.
> Though I wonder whether we really need this optimization.

Thanks for the review.

Caching of StackWalker instances is not highly needed and I want to avoid the 
method name to preclude such possibility.

Mandy

Re: RFR: 8140344: add support for 3 digit update release numbers

[Rob McKenna]

I'd expect this to be superseded by 223 completely but I've cc'd 
verona-dev in case there are objections. At this point the fix is more 
for the benefit of 7 & 8. I'd be happy to mark this 9-na if that makes 
more sense?


-Rob

On 17/11/15 15:19, Alan Bateman wrote:



On 17/11/2015 14:47, Rob McKenna wrote:

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing
infrastructure balks at the extra digit. With that in mind I'd like to
update the logic surrounding the parsing of version numbers and the
test itself to use regex instead of the less flexible
CharacterSequence method.

http://cr.openjdk.java.net/~robm/8140344/webrev.01/

Would it be better to bring this to verona-dev so that this can be
looked at in the context of the JEP 223 changes?

-Alan

Re: RFR: 8140344: add support for 3 digit update release numbers

[Alan Bateman]

On 17/11/2015 14:47, Rob McKenna wrote:

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing 
infrastructure balks at the extra digit. With that in mind I'd like to 
update the logic surrounding the parsing of version numbers and the 
test itself to use regex instead of the less flexible 
CharacterSequence method.


http://cr.openjdk.java.net/~robm/8140344/webrev.01/
Would it be better to bring this to verona-dev so that this can be 
looked at in the context of the JEP 223 changes?


-Alan

Re: Proposed API for JEP 259: Stack-Walking API

[Peter Levart]

On 11/16/2015 08:16 PM, Mandy Chung wrote:

On Nov 15, 2015, at 10:59 AM, Peter Levart  wrote:

OTOH in the described cases, a caller of walker.getCallerClass() is actually expecting to be called 
by a Java method, right? So what would it be if such "caller-sensitive" method demanded 
to be called by a Java method and throw IllegalArgumentException("Not called by Java 
method") otherwise?

Have you thought of that possibility?

Are you thinking about a JNI method calling getCallerClass?  Or refer 
getCallerClass being the bottom of the stack?

I don’t see any issue for a JNI method calling getCallerClass and it’s doing:
* Class caller = walker.walk(s ->
* s.map(StackFrame::getDeclaringClass)
*  .skip(2)
*  .findFirst());

Mandy


Hi Mandy,

No, I was not thinking of StackWalker::getCallerClass being called by 
JNI. I was thinking of a "caller-sensitive" method calling 
getCallerClass() and that "caller-sensitive" method being called by JNI 
from a newly attached thread.


1st, I don't think anyone will attempt calling getCallerClass() from the 
static main() method invoked by java launcher. That's silly and need not 
be supported.
2nd, I don't think anyone will attempt calling getCallerClass() from 
overridden Thread::run() method. That too need not be supported.


So what we are left with are other "caller-sensitive" methods calling 
getCallerClass(). Like for example:


public class Strings {
public static ResourceBundle getBundle() {
Class cc = stackWalker.getCallerClass();
return ResourceBundle.getBundle(cc.getName() + "$Strings", 
Locale.getDefault(), cc.getClassLoader());

}
}

Such method obviously expects to be called by some Java method. So it 
may as well demand it. By throwing exception if it is called by JNI from 
newly attached thread.


"caller-sensitive" methods could be viewed as taking an implicit 
parameter - their caller. If that parameter is not specified, they could 
behave like when some other explicit parameter was not specified or 
invalid - throw new IllegalArgumentException("Not called by Java 
method") ...


So instead of "pretending" they were called by themselves, which might 
not be right, simply refuse the invocation with exception. I don't think 
this would hinder any such caller-sensitive method in it's utility. More 
importantly, it would prevent such methods from being abused.


You said about possible use-cases for getCallerClass:


I have been thinking what the users would do when there is no caller.

The JDK use  of getCallerClass are to:
1. find the caller's class loader for class loader hierarchy check


Abuse: calling such "caller-sensitive" method by JNI from newly-attached 
thread would use the caller-sensitive method's declaring class 
ClassLoader for hierarchy check - wrong!


2. find the caller’s class loader to load something on behalf of the 
caller (visibility)


Abuse: ... would use the caller-sensitive method's declaring class 
ClassLoader to load something - might work, might not


3. looking forward to work with modules, one would want to get the 
caller’s module

- to load something on behalf of the caller's module


Abuse: ...would load something from caller-sensitive method's declaring 
class module - wrong.



- to add read edge to the caller’s module before accessing its type


Abuse: ...would add a read edge to itself before accessing some type - 
wrong.



In all those cases throwing exception would be the right thing to do.


What do you think?

Regards, Peter

RFR: JDK-8057804: AnnotatedType interfaces provide no way to get annotations on owner type

[Joel Borggrén-Franck]

Hi,

When reflecting over annotated types, there is currently no way to get
the potentially annotated owner of a type. For example, given you have
an instance of '@A Outer . @B Inner' you can't traverse it to get '@A
Outer' .

This API addition fixes this. Because both parameterized and
non-generic types can have an owner, this addition goes into the base
AnnotatedType interface together with a default implementation. CCC
has been filed.

The parsing code and annotated type factory had to be fixed to deal
with navigating inside nested types.

Bug:   https://bugs.openjdk.java.net/browse/JDK-8057804
Webrev: http://cr.openjdk.java.net/~rbackman/jbf/8057804/webrev.00/


(OCA is signed and processed).

Cheers
/Joel

Re: RFR: 8143142: AssertionError in MethodHandleImpl

[Vladimir Ivanov]

Reviewed.

Best regards,
Vladimir Ivanov

On 11/17/15 5:21 PM, Claes Redestad wrote:

Hi,

it seems I've been testing assertions with the wrong flag.

8142334 added the same assertion check to MethodHandleImpl that was
there for DirectMethodHandle, which
due to some bootstrapping dependency fails for the MethodHandleImpl
case. This patch removes that assertion:

webrev: http://cr.openjdk.java.net/~redestad/8143142/webrev.01/
bug: https://bugs.openjdk.java.net/browse/JDK-8143142

/Claes

RFR: 8143142: AssertionError in MethodHandleImpl

[Claes Redestad]

Hi,

it seems I've been testing assertions with the wrong flag.

8142334 added the same assertion check to MethodHandleImpl that was 
there for DirectMethodHandle, which
due to some bootstrapping dependency fails for the MethodHandleImpl 
case. This patch removes that assertion:


webrev: http://cr.openjdk.java.net/~redestad/8143142/webrev.01/
bug: https://bugs.openjdk.java.net/browse/JDK-8143142

/Claes

Re: RFR: jsr166 jdk9 integration wave 2

[Aleksey Shipilev]

On 11/17/2015 02:04 AM, Martin Buchholz wrote:
> On Mon, Nov 16, 2015 at 2:42 PM, Aleksey Shipilev
> mailto:aleksey.shipi...@oracle.com>> wrote:
> ForkJoinWorkerThread.newThread/:
>  * I think this one requires CCC, because it changes public API in
> newThread.
> 
> 
> That's where someone from Oracle needs to help out ...

Yes, that's a trivial API correction, as Doug notes.
Paul had already submitted CCC, and it is fast-tracking now.

> PhaserBasic/:
>  * Are those tracing statements ("// trace("barrier action",
> startTime);") kept for a reason? Should we conditionalize them with
> "static final boolean"-s?
> 
> Phaser/Basic has been troublesome, and it still has unresolved rare 
> failures... Those traces were obviously added for debugging, but I'm
> still reluctant to remove them...

Yes. My point is that it may be better to conditionalize the debug
printing on static final boolean field, rather than having a
non-compiled commented line.

Thanks,
-Aleksey

Re: RFR(L): 8139885: implement JEP 274: enhanced method handles

[Vladimir Ivanov]

Awesome! Looks really good, Michael!

src/java.base/share/classes/java/lang/invoke/MethodHandles.java:
 if (!hasPrivateAccess()
 || (specialCaller != lookupClass()
+   // ensure non-abstract methods in 
superinterfaces can be special-invoked
+&& !(refc != null && refc.isInterface() && 
refc.isAssignableFrom(specialCaller))

 && !(ALLOW_NESTMATE_ACCESS &&

Is it a fix for an existing bug? If it's the case, I'd prefer to see it 
as a stand alone fix.


src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java:
+static final MethodHandle MH_looper;
+static final MethodHandle MH_countedLoopPred;
+static final MethodHandle MH_countedLoopStep;
+static final MethodHandle MH_iteratePred;
+static final MethodHandle MH_initIterator;
+static final MethodHandle MH_iterateNext;
+static final MethodHandle MH_tryFinallyExec;
+static final MethodHandle MH_tryFinallyVoidExec;

I think you have to adjust that part since Claes made MH constant 
initialization lazy.


Also, does it make sense to provide bytecode intrinsics for tryFinally 
and loop combinators in InvokerBytecodeGenerator to compile them in more 
efficient bytecode shapes. If yes, please, file corresponding RFEs.


Best regards,
Vladimir Ivanov

On 11/13/15 7:39 PM, Michael Haupt wrote:

Dear all,

please review this change.
RFE: https://bugs.openjdk.java.net/browse/JDK-8139885
Corresponding JEP: https://bugs.openjdk.java.net/browse/JDK-8130227
Webrev: http://cr.openjdk.java.net/~mhaupt/8139885/webrev.00/

Thanks,

Michael

--

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




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

Re: RFR: 8142487: Cleanup sun.invoke.util.Wrapper zeroes to be both reliable and lazy

[Vladimir Ivanov]

Looks good.

Best regards,
Vladimir Ivanov

On 11/17/15 2:43 PM, Claes Redestad wrote:

On 2015-11-17 12:22, Vladimir Ivanov wrote:

src/java.base/share/classes/java/lang/invoke/LambdaForm.java:
+ private static void createFormsFor(BasicType type) {
+ synchronized (LF_identity) {

Looks a bit confusing since the method touches both LF_identity and
LF_zero array now. Why not simply make the method synchronized?
LambdaForm is package-private and shouldn't leak outside j.l.i.


Fixed.



test/sun/invoke/util/WrapperTest.java:
@run junit/othervm

Why do you specify /othervm mode?


Changed to @run junit

http://cr.openjdk.java.net/~redestad/8142487/webrev.05

Thanks!

/Claes

Re: RFR: 8142487: Cleanup sun.invoke.util.Wrapper zeroes to be both reliable and lazy

[Claes Redestad]

On 2015-11-17 12:22, Vladimir Ivanov wrote:

src/java.base/share/classes/java/lang/invoke/LambdaForm.java:
+ private static void createFormsFor(BasicType type) {
+ synchronized (LF_identity) {

Looks a bit confusing since the method touches both LF_identity and 
LF_zero array now. Why not simply make the method synchronized? 
LambdaForm is package-private and shouldn't leak outside j.l.i.


Fixed.



test/sun/invoke/util/WrapperTest.java:
@run junit/othervm

Why do you specify /othervm mode?


Changed to @run junit

http://cr.openjdk.java.net/~redestad/8142487/webrev.05

Thanks!

/Claes

Re: RFR: 8142487: Cleanup sun.invoke.util.Wrapper zeroes to be both reliable and lazy

[Vladimir Ivanov]

src/java.base/share/classes/java/lang/invoke/LambdaForm.java:
+ private static void createFormsFor(BasicType type) {
+ synchronized (LF_identity) {

Looks a bit confusing since the method touches both LF_identity and 
LF_zero array now. Why not simply make the method synchronized? 
LambdaForm is package-private and shouldn't leak outside j.l.i.


test/sun/invoke/util/WrapperTest.java:
@run junit/othervm

Why do you specify /othervm mode?

Best regards,
Vladimir Ivanov

On 11/17/15 2:04 PM, Claes Redestad wrote:


On 2015-11-14 01:05, Vladimir Ivanov wrote:



I think we have to add a storeFence or two to avoid publishing
NamedFunctions whose resolvedHandle may otherwise be observed as only
partially visible:

http://cr.openjdk.java.net/~redestad/8142487/webrev.03/

Same here. No need in Unsafe.storeFence if proper constructor is used.


Right, together with the added lazy initialization safety we added in
8142334[1] I think this is more than enough:

http://cr.openjdk.java.net/~redestad/8142487/webrev.04/

/Claes

[1] https://bugs.openjdk.java.net/browse/JDK-8142334

Re: RFR: 8142487: Cleanup sun.invoke.util.Wrapper zeroes to be both reliable and lazy

[Claes Redestad]

On 2015-11-14 01:05, Vladimir Ivanov wrote:



I think we have to add a storeFence or two to avoid publishing
NamedFunctions whose resolvedHandle may otherwise be observed as only
partially visible:

http://cr.openjdk.java.net/~redestad/8142487/webrev.03/

Same here. No need in Unsafe.storeFence if proper constructor is used.


Right, together with the added lazy initialization safety we added in 
8142334[1] I think this is more than enough:


http://cr.openjdk.java.net/~redestad/8142487/webrev.04/

/Claes

[1] https://bugs.openjdk.java.net/browse/JDK-8142334

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Claes Redestad]

Thank you, Vladimir!

/Claes

On 2015-11-17 11:25, Vladimir Ivanov wrote:

Looks good!

Best regards,
Vladimir Ivanov

PS: I'm impressed by your courage and persistence, Claes :-) Untwining 
bootstrapping knot is notoriously hard, especially when you are new to 
the code.


On 11/16/15 7:17 PM, Claes Redestad wrote:

Thanks for the explanation, and patience, Vladimir!

Reworked:
http://cr.openjdk.java.net/~redestad/8142334/webrev.09/

/Claes

On 2015-11-16 13:49, Vladimir Ivanov wrote:

The trick here is @Stable annotation.

If the @Stable field is written to non-default value in constructor,
it should be equivalent to final field initialization.

If the field is written to later, the initialization should be either
idempotent or properly synchronized. I'd like to note that though
multiple writes to @Stable fields are allowed now, the behavior is
undefined [1], so it would be better to ensure the field is written
only once. Also, though we haven't seen any issues with NamedFunction
initialization before (probably, because it is performed during
bootstrap), we saw problems in other parts of the framework when
multiple non-default values are observed (e.g. JDK-8005873 [2]).

There are 2 ways to delay initialization:
  (1) lazily instantiate NamedFunctions;
  (2) lazily initialize NamedFucntion.

Both approaches should give startup savings.

In the first case, there should be some exceptions to let the system
bootstrap (some instances should be lazily resolved).

Second case looks cleaner, but requires proper publication as you 
noted.


I'm fine with (2): make NF.resolve() synchronized and access the field
only through accessor (make the field private to guarantee that).

I don't see any need in NamedFunction caches then - it should be cheap
to instantiate NFs.

Best regards,
Vladimir Ivanov

[1] @Stable javadoc:
"It is (currently) undefined what happens if a field annotated as
stable is given a third value. In practice, if the JVM relies on this
annotation to promote a field reference to a constant, it may be that
the Java memory model would appear to be broken, if such a constant
(the second value of the field) is used as the value of the field even
after the field value has changed."

[2] https://jbs.oracle.com/browse/JDK-8005873

On 11/15/15 12:29 PM, Peter Levart wrote:

Hi Vladimir,

On 11/14/2015 12:59 AM, Vladimir Ivanov wrote:

NamedFunction.resolvedHandle should be usually pre-resolved, but for
bootstrapping purposes it is done lazily in some cases. I don't see
any reason why NamedFunction.resolve() should be called on a freshly
created instance. Use proper constructor instead.


I agree, but I think that's just a style issue. Even if you use a 
proper

constructor, the fact that resolvedHandle is not a final field means
that such instance has to be published safely to other threads or you
have to guarantee that resolvedHandle of such instance is accessed 
only

through accessor method that checks for null and (re)initializes the
field if it finds it still being null.

In the later case (when resolvedHandle is pre-populated and
NamedFunction is published unsafely), it can happen that some threads
get the pre-populated instance of resolvedHandle and other threads 
that

don't yet see the pre-populated instance (because of reorderings) get
the lazily resolved instance computed by one of them (if 
double-checked

locking is used).

So if always returning a single instance from resolvedHandle() 
accessor
is important, resolvedHandle field should not be pre-populated if 
unsafe

publication of NamedFunction instance is exercised. What role does
pre-resolving play? I only see it as a means to throw exceptions 
early.

So perhaps an assert in selected constructors that resolves the handle
but does not assign it to resolvedHandle field could catch any coding
mistakes in tests for the price of double-resolving when assertions 
are

enabled.


NamedFunction(MethodHandle resolvedHandle)
NamedFunction(MemberName member, MethodHandle resolvedHandle)
NamedFunction(MethodType basicInvokerType)

// The next 3 constructors are used to break circular
dependencies on MH.invokeStatic, etc.
// Any LambdaForm containing such a member is not
interpretable.
// This is OK, since all such LFs are prepared with special
primitive vmentry points.
// And even without the resolvedHandle, the name can still be
compiled and optimized.
NamedFunction(Method method) {
this(new MemberName(method));
}
NamedFunction(Field field) {
this(new MemberName(field));
}
NamedFunction(MemberName member) {
this.member = member;
this.resolvedHandle = null;
}

There are 2 non-final fields in NamedFunction:
  static class NamedFunction {
  final MemberName member;
  @Stable MethodHandle resolvedHandle;
  @Stable MethodHandle invoker;

Repeated initialization of NamedFunction.invoker is benign sin

RFR(XS): 8143127: InvokerBytecodeGenerator emitConst should handle Byte, Short, Character

[Claes Redestad]

Hi,

this small patch allows InvokerBytecodeGenerator to generate more 
compact bytecode for small byte, short and char constants:


webrev: http://cr.openjdk.java.net/~redestad/8143127/webrev.01/
bug: https://bugs.openjdk.java.net/browse/JDK-8143127

/Claes

RE: Code Review for JDK-8141243: Unexpected timezone returned after parsing a date

[Ramanand Patil]

Hi Masayoshi,

Thank you for your feedback and suggestion.

As you said, I understood that There's no perfect solution on the issue because 
the short display names(abbreviations) can't be unique.

I was trying to apply the workaround by considering the cases where the short 
display names are same as the Zone IDs. Anyways if the issue is reproducible in 
JDK 9 with the legacy time zone names I will dig more into it and try to find 
the correct fix.

Sorry, if I am asking very simple question(I am new to this group/process and 
want to understand correctly),
I didn't understand what is meant by your suggestion of moving UTC display 
names in all resource bundles (all TimeZoneNames*.java under 
sun.util.resources) up to the compatibility group.


Thank you for your help.

Regards,
Ramanand.



-Original Message-
From: Masayoshi Okutsu 
Sent: Tuesday, November 17, 2015 2:24 PM
To: Ramanand Patil; i18n-dev; core-libs-dev; jdk8u-dev
Subject: Re:  Code Review for JDK-8141243: Unexpected timezone 
returned after parsing a date

Hi Ramanand,

I don't think this fix is correct. This change mixes up time zone IDs and 
display names. SimpleDateFomat should parse only display names.

There's no perfect solution on the issue because the short display names
(abbreviations) can't be unique. So, I'd suggest that the UTC display names in 
all resource bundles (all TimeZoneNames*.java under
sun.util.resources) move up to the compatibility group.

BTW, the symptom is reproducible in JDK 9 with the legacy time zone names (run 
java with -Djava.locale.providers=COMPAT). So, the fix should be done in JDK 9 
first.

Thanks,
Masayoshi

On 11/16/2015 10:38 PM, Ramanand Patil wrote:
> Hi all,
>
>   
>
> Please review a fix for Bug Id - HYPERLINK 
> "https://bugs.openjdk.java.net/browse/JDK-8141243"JDK-8141243.
>
> 
>
>   
>
> Issue - When parsing the virtual timezone "UTC" with 
> java.text.SimpleDateFormat, the timezone is set to the first timezone that 
> matches an actual timezone in the UTC group, which is Antarctica/Troll. When 
> comparing this timezone with the result of TimeZone.getTimeZone("UTC"), we 
> fail.
>
>   
>
> Webrev - http://cr.openjdk.java.net/~aefimov/8141243/webrev.00/
>
>   
>
>   
>
> Regards,
>
> Ramanand.
>
>   

Re: RFR: jsr166 jdk9 integration wave 2

[Paul Sandoz]

> On 16 Nov 2015, at 22:39, Martin Buchholz  wrote:
> 
> Smaller than wave 1, but still large.  Nothing like a looming deadline to 
> spur work ...
> 
> Oracle folks will need to help with API review.
> 
> https://bugs.openjdk.java.net/issues/?jql=(subcomponent%20%3D%20java.util.concurrent)%20AND%20status%20%3D%20%22In%20Progress%22%20ORDER%20BY%20updatedDate%20DESC
>  
> 
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/ 
> 
> 
> The primary focus is making jtreg tests more robust and faster.
> It also contains the changes to j.u.c.atomic that Aleksey is waiting for.

For the jtregTest updates, if you have any excess energy remaining, you might 
want to consider adding the jtreg randomness tag (i cannot recall its exact 
name) to the test metadata.

Paul.

Re: RFR: jsr166 jdk9 integration wave 2

[Paul Sandoz]

> On 16 Nov 2015, at 22:39, Martin Buchholz  wrote:
> 
> Smaller than wave 1, but still large.  Nothing like a looming deadline to 
> spur work ...
> 
> Oracle folks will need to help with API review.
> 
> https://bugs.openjdk.java.net/issues/?jql=(subcomponent%20%3D%20java.util.concurrent)%20AND%20status%20%3D%20%22In%20Progress%22%20ORDER%20BY%20updatedDate%20DESC
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
> 
> The primary focus is making jtreg tests more robust and faster.
> It also contains the changes to j.u.c.atomic that Aleksey is waiting for.


src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
—

CCC created.


src/java.base/share/classes/java/util/concurrent/atomic/AtomicReferenceFieldUpdater.java
—

 398 private final void valueCheck(V v) {
 399 if (v != null && !(vclass.isInstance(v)))
 400 throwCCE();
 401 }

Why not directly use vclass.cast ?

I think there may be a subtle change in behaviour with the updates to A*FU, 
where previously a CCE would be thrown and now a IAE is thrown e.g. consider an 
erased A*FU to a public field of some class and and a receiver of the incorrect 
type is passed to an access method. Does not really matter in practice, 
although the error message will be confusing.

Paul.

RFR: 8140344: add support for 3 digit update release numbers

[Rob McKenna]

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing 
infrastructure balks at the extra digit. With that in mind I'd like to 
update the logic surrounding the parsing of version numbers and the test 
itself to use regex instead of the less flexible CharacterSequence method.


http://cr.openjdk.java.net/~robm/8140344/webrev.01/

-Rob

Re: RFR: 8143142: AssertionError in MethodHandleImpl

[Claes Redestad]

Vladimir, Paul,

thanks for looking at this so quickly!

Pushed.

/Claes

On 2015-11-17 15:30, Paul Sandoz wrote:

On 17 Nov 2015, at 15:21, Claes Redestad  wrote:

Hi,

it seems I've been testing assertions with the wrong flag.


To share some knowledge: when running jtreg locally it is important to use the 
same parameters that are used when running the automated tests. To the best of 
my knowledge those parameters are:

   -ea -esa -avm



8142334 added the same assertion check to MethodHandleImpl that was there for 
DirectMethodHandle, which
due to some bootstrapping dependency fails for the MethodHandleImpl case. This 
patch removes that assertion:

webrev: http://cr.openjdk.java.net/~redestad/8143142/webrev.01/
bug: https://bugs.openjdk.java.net/browse/JDK-8143142


Looks ok to me (but i am getting a little lost in the maze of improvements and 
various iterations :-) ).

Paul.

Re: RFR: 8143142: AssertionError in MethodHandleImpl

[Paul Sandoz]

> On 17 Nov 2015, at 15:21, Claes Redestad  wrote:
> 
> Hi,
> 
> it seems I've been testing assertions with the wrong flag.
> 

To share some knowledge: when running jtreg locally it is important to use the 
same parameters that are used when running the automated tests. To the best of 
my knowledge those parameters are:

  -ea -esa -avm


> 8142334 added the same assertion check to MethodHandleImpl that was there for 
> DirectMethodHandle, which
> due to some bootstrapping dependency fails for the MethodHandleImpl case. 
> This patch removes that assertion:
> 
> webrev: http://cr.openjdk.java.net/~redestad/8143142/webrev.01/
> bug: https://bugs.openjdk.java.net/browse/JDK-8143142
> 

Looks ok to me (but i am getting a little lost in the maze of improvements and 
various iterations :-) ).

Paul.

Re: RFR: jsr166 jdk9 integration wave 2

[Paul Sandoz]

> On 17 Nov 2015, at 13:01, Doug Lea  wrote:
> We added more tck tests to check this, and all pass.
> ClassCastException was/is thrown in two cases, without distinguishing them,
> which allowed a path merge by arranging that "cclass" is now the same as 
> "vclass" unless field is protected.
> 

My bad, sorry for the noise, i misread the code in throwAccessCheckException

—

Separately but related, i have a solution that allows VarHandles to be used in 
the A*FU implementations. However, i am not sure it would be possible to 
preserve the exact exception throwing behaviour and their content e.g. the type 
of a CAS expect value would likely be checked.

Paul.

Re: RFR: jsr166 jdk9 integration wave 2

[Claes Redestad]

On 2015-11-17 00:06, Martin Buchholz wrote:

On Mon, Nov 16, 2015 at 2:42 PM, Aleksey Shipilev <
aleksey.shipi...@oracle.com> wrote:


The primary focus is making jtreg tests more robust and faster.
It also contains the changes to j.u.c.atomic that Aleksey is waiting for.

Yes! Thank you. If you push everything in one batch, please split that
one ("atomic/") out to ease backporting to 8u.


Each webrev will become one changeset.


+1 - having a history to bisect over if anything crops up is worth a lot.

/Claes

Re: RFR: jsr166 jdk9 integration wave 2

[Doug Lea]

On 11/17/2015 05:16 AM, Paul Sandoz wrote:


src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java
—

CCC created.


Thanks, although it is just a clarification that makes
ForkJoinWorkerThreadFactory spec more similar to ThreadFactory.




src/java.base/share/classes/java/util/concurrent/atomic/AtomicReferenceFieldUpdater.java
—

  398 private final void valueCheck(V v) {
  399 if (v != null && !(vclass.isInstance(v)))
  400 throwCCE();
  401 }

Why not directly use vclass.cast ?


Only to preserve the exact exception thrown. Class.cast adds a detail message
to the ClassCastException that can be deceptive in...



I think there may be a subtle change in behaviour with the updates to A*FU, 
where previously a CCE would be thrown and now a IAE is thrown e.g. consider an 
erased A*FU to a public field of some class and and a receiver of the incorrect 
type is passed to an access method. Does not really matter in practice, 
although the error message will be confusing.



We added more tck tests to check this, and all pass.
ClassCastException was/is thrown in two cases, without distinguishing them,
which allowed a path merge by arranging that "cclass" is now the same as 
"vclass" unless field is protected.


-Doug

Re: Code Review for JDK-8141243: Unexpected timezone returned after parsing a date

[Masayoshi Okutsu]

Hi Ramanand,

I don't think this fix is correct. This change mixes up time zone IDs 
and display names. SimpleDateFomat should parse only display names.


There's no perfect solution on the issue because the short display names 
(abbreviations) can't be unique. So, I'd suggest that the UTC display 
names in all resource bundles (all TimeZoneNames*.java under 
sun.util.resources) move up to the compatibility group.


BTW, the symptom is reproducible in JDK 9 with the legacy time zone 
names (run java with -Djava.locale.providers=COMPAT). So, the fix should 
be done in JDK 9 first.


Thanks,
Masayoshi

On 11/16/2015 10:38 PM, Ramanand Patil wrote:

Hi all,

  


Please review a fix for Bug Id - HYPERLINK 
"https://bugs.openjdk.java.net/browse/JDK-8141243"JDK-8141243.



  


Issue - When parsing the virtual timezone "UTC" with java.text.SimpleDateFormat, the 
timezone is set to the first timezone that matches an actual timezone in the UTC group, which is 
Antarctica/Troll. When comparing this timezone with the result of 
TimeZone.getTimeZone("UTC"), we fail.

  


Webrev - http://cr.openjdk.java.net/~aefimov/8141243/webrev.00/

  

  


Regards,

Ramanand.

  

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Paul Sandoz]

> On 17 Nov 2015, at 11:25, Vladimir Ivanov  
> wrote:
> 
> Looks good!
> 
> Best regards,
> Vladimir Ivanov
> 
> PS: I'm impressed by your courage and persistence, Claes :-) Untwining 
> bootstrapping knot is notoriously hard, especially when you are new to the 
> code.
> 

+1 to that.

Paul.

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Vladimir Ivanov]

Looks good!

Best regards,
Vladimir Ivanov

PS: I'm impressed by your courage and persistence, Claes :-) Untwining 
bootstrapping knot is notoriously hard, especially when you are new to 
the code.


On 11/16/15 7:17 PM, Claes Redestad wrote:

Thanks for the explanation, and patience, Vladimir!

Reworked:
http://cr.openjdk.java.net/~redestad/8142334/webrev.09/

/Claes

On 2015-11-16 13:49, Vladimir Ivanov wrote:

The trick here is @Stable annotation.

If the @Stable field is written to non-default value in constructor,
it should be equivalent to final field initialization.

If the field is written to later, the initialization should be either
idempotent or properly synchronized. I'd like to note that though
multiple writes to @Stable fields are allowed now, the behavior is
undefined [1], so it would be better to ensure the field is written
only once. Also, though we haven't seen any issues with NamedFunction
initialization before (probably, because it is performed during
bootstrap), we saw problems in other parts of the framework when
multiple non-default values are observed (e.g. JDK-8005873 [2]).

There are 2 ways to delay initialization:
  (1) lazily instantiate NamedFunctions;
  (2) lazily initialize NamedFucntion.

Both approaches should give startup savings.

In the first case, there should be some exceptions to let the system
bootstrap (some instances should be lazily resolved).

Second case looks cleaner, but requires proper publication as you noted.

I'm fine with (2): make NF.resolve() synchronized and access the field
only through accessor (make the field private to guarantee that).

I don't see any need in NamedFunction caches then - it should be cheap
to instantiate NFs.

Best regards,
Vladimir Ivanov

[1] @Stable javadoc:
"It is (currently) undefined what happens if a field annotated as
stable is given a third value. In practice, if the JVM relies on this
annotation to promote a field reference to a constant, it may be that
the Java memory model would appear to be broken, if such a constant
(the second value of the field) is used as the value of the field even
after the field value has changed."

[2] https://jbs.oracle.com/browse/JDK-8005873

On 11/15/15 12:29 PM, Peter Levart wrote:

Hi Vladimir,

On 11/14/2015 12:59 AM, Vladimir Ivanov wrote:

NamedFunction.resolvedHandle should be usually pre-resolved, but for
bootstrapping purposes it is done lazily in some cases. I don't see
any reason why NamedFunction.resolve() should be called on a freshly
created instance. Use proper constructor instead.


I agree, but I think that's just a style issue. Even if you use a proper
constructor, the fact that resolvedHandle is not a final field means
that such instance has to be published safely to other threads or you
have to guarantee that resolvedHandle of such instance is accessed only
through accessor method that checks for null and (re)initializes the
field if it finds it still being null.

In the later case (when resolvedHandle is pre-populated and
NamedFunction is published unsafely), it can happen that some threads
get the pre-populated instance of resolvedHandle and other threads that
don't yet see the pre-populated instance (because of reorderings) get
the lazily resolved instance computed by one of them (if double-checked
locking is used).

So if always returning a single instance from resolvedHandle() accessor
is important, resolvedHandle field should not be pre-populated if unsafe
publication of NamedFunction instance is exercised. What role does
pre-resolving play? I only see it as a means to throw exceptions early.
So perhaps an assert in selected constructors that resolves the handle
but does not assign it to resolvedHandle field could catch any coding
mistakes in tests for the price of double-resolving when assertions are
enabled.


NamedFunction(MethodHandle resolvedHandle)
NamedFunction(MemberName member, MethodHandle resolvedHandle)
NamedFunction(MethodType basicInvokerType)

// The next 3 constructors are used to break circular
dependencies on MH.invokeStatic, etc.
// Any LambdaForm containing such a member is not
interpretable.
// This is OK, since all such LFs are prepared with special
primitive vmentry points.
// And even without the resolvedHandle, the name can still be
compiled and optimized.
NamedFunction(Method method) {
this(new MemberName(method));
}
NamedFunction(Field field) {
this(new MemberName(field));
}
NamedFunction(MemberName member) {
this.member = member;
this.resolvedHandle = null;
}

There are 2 non-final fields in NamedFunction:
  static class NamedFunction {
  final MemberName member;
  @Stable MethodHandle resolvedHandle;
  @Stable MethodHandle invoker;

Repeated initialization of NamedFunction.invoker is benign since
NamedFunction.computeInvoker uses synchronized
MethodTypeForm.setCachedMethodHandle to po