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

[forax]

Hi peter,
Given this is a stack walker, considering the stack as a state seems natural to 
me.

Rémi

- Mail original -
> De: "Peter Levart" 
> À: "Remi Forax" , "David Holmes" 
> Cc: "Mandy Chung" , "OpenJDK Dev list" 
> 
> Envoyé: Mercredi 18 Novembre 2015 13:26:32
> Objet: Re: Proposed API for JEP 259: Stack-Walking API
> 
> 
> 
> On 11/18/2015 12:22 PM, Remi Forax wrote:
> > - Mail original -
> >> De: "David Holmes" 
> >> À: "Mandy Chung" , "Peter Levart"
> >> 
> >> Cc: "OpenJDK Dev list" 
> >> Envoyé: Mercredi 18 Novembre 2015 08:58:56
> >> Objet: Re: Proposed API for JEP 259: Stack-Walking API
> >>
> >> On 18/11/2015 8:42 AM, Mandy Chung wrote:
>  On Nov 17, 2015, at 2:09 PM, Peter Levart 
>  wrote:
> 
>  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.
> 
> >>> I’m now convinced that it’s not a good idea to special case it.
> >>> getCallerClass will simply return the caller frame (i.e. top-2) on the
> >>> stack and throw UOE if there is no caller frame.  The user should call
> >>> StackWalker::walk instead if this special case matters.
> >> That sounds good to me too.
> >>
> >> David
> > Looks good to me too if IllegalStateException is used instead of
> > UnsupportedOperationException.
> > UnsuppportedOperationException is used when the operation is not available,
> > here, the same code can work or not depending how it is called.
> 
> But IllegalStateException is dependent on some state. There's no state
> involved here (in the sense "state" is characterized in Java). My 1st
> thought was an IllegalArgumentException. This requires some imagination
> to view the caller passed to the method as an implicit argument.
> 
> There's an obscure java.util.EmptyStackException but that is reserved
> for java.util.Stack operations.
> 
> If we consider the call stack to be part of the Thread state, then maybe
> java.lang.IllegalThreadStateException (a subclass of
> IllegalArgumentException) could be used...
> 
> Regards, Peter
> 
> > Runnable r = () -> System.out.println(stackWalker.getCallerClass());
> > new Thread(r).start() // throw ISE
> > r.run();  // prints main class
> >
> > Rémi
> >
> >>> How does this look?
> >>>
> >>> /**
> >>>* 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,
> >>>* i.e. invoked from a JNI attached thread (
> >>>* for example, {@code static public void main} method launched by the
> >>>* {@code java} launcher).
> >>>*
> >>>* @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,
> >>>caller.getClassLoader());
> >>>* }
> >>>* }
> >>>*
> >>>* class MyTool {
> >>>* 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 = 

Re: jmx-dev RFR 6425769: jmx remote bind address

[Severin Gehwolf]

Hi,

Re-posting this for review since I've done another version which
duplicates some of the code in SslRMIServerSocketFactory.java but does
not change API other than adding the new property. I personally don't
like it, but maybe this version is preferred?

Bug: https://bugs.openjdk.java.net/browse/JDK-6425769
webrev: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/03.no-rmi-ssl-factory-changes/

Thanks,
Severin

On Mon, 2015-11-09 at 10:32 +0100, Severin Gehwolf wrote:
> On Wed, 2015-11-04 at 11:54 +0100, Severin Gehwolf wrote:
> > Hi,
> > 
> > Updated webrev with jtreg test in Java:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6425769/02/
> > bug: https://bugs.openjdk.java.net/browse/JDK-6425769
> > 
> > I believe this updated webrev addresses all concerns and
> > incorporates
> > suggestions brought up by Jaroslav and Daniel.
> > 
> > I'm still looking for a sponsor and a hotspot/servicability-dev
> > reviewer. Could somebody maintaining javax.rmi.ssl have a look at
> > this
> > as well? Thank you!
> 
> Ping? Friendly reminder that I still need reviewers and a sponsor for
> this.
> 
> Thanks,
> Severin
> 
> > Cheers,
> > Severin
> > 
> > On Tue, 2015-11-03 at 15:45 +0100, Jaroslav Bachorik wrote:
> > > On 2.11.2015 19:06, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Thanks Jaroslav and Daniel for the reviews! Comments inline.
> > > > 
> > > > On Mon, 2015-11-02 at 16:54 +0100, Jaroslav Bachorik wrote:
> > > > > Hi,
> > > > > 
> > > > > On 2.11.2015 16:19, Daniel Fuchs wrote:
> > > > > > Hi Severin,
> > > > > > 
> > > > > > Adding serviceability-...@openjdk.java.net into the loop -
> > > > > > that's
> > > > > > a better alias than hotspot-dev for this kind of changes -
> > > > > > maybe
> > > > > > someone from serviceability-dev will offer to sponsor :-)
> > > > > > 
> > > > > > I will let serviceability team members comment on the
> > > > > > hotspot
> > > > > > changes.
> > > > > > 
> > > > > > ConnectorBootstrap.java
> > > > > > 
> > > > > > I have one suggestion and one concern:
> > > > > > 
> > > > > > Suggestion: I would suggest to reuse 'csf' (Client Socket
> > > > > > Factory)
> > > > > > and
> > > > > > ssf  (Server Socket Factory) variables rather than
> > > > > > introduce
> > > > > > the
> > > > > > two
> > > > > > new variables rmiServerSocketFactory and
> > > > > > rmiClientSocketFactory.
> > > > > > You might want to create a new boolean 'useSocketFactory'
> > > > > > variable,
> > > > > > if that makes the code more readable.
> > > > > > 
> > > > > > Concern: If I understand correctly how RMI socket factories
> > > > > > work,
> > > > > > the client socket factory will be serialized and sent to
> > > > > > the
> > > > > > client
> > > > > > side. This is problematic for interoperability, as the
> > > > > > class
> > > > > > may
> > > > > > not
> > > > > > present in the remote client - if the remote client is e.g.
> > > > > > jdk
> > > > > > 8.
> > > > > > 
> > > > > > As far as I can see, your new DefaultClientSocketFactory
> > > > > > doesn't do
> > > > > > anything useful - so I would suggest to simply get rid of
> > > > > > it,
> > > > > > and
> > > > > > only
> > > > > > set the Server Socket Factory when SSL is not involved.
> > > > 
> > > > Thanks. Fixed in updated webrev.
> > > > 
> > > > > > Tests:
> > > > > > 
> > > > > > Concerning the tests - we're trying to get rid of shell
> > > > > > scripts
> > > > > > rather than introducing new ones :-)
> > > > > > Could the test be rewritten in pure java using the Process
> > > > > > API?
> > > > > > 
> > > > > > I believe there's even a test library that will let you do
> > > > > > that
> > > > > > easily jdk/test/lib/testlibrary/jdk/testlibrary/
> > > > > > (see ProcessTools.java)
> > > > 
> > > > It'll take me a bit to rewrite the test in pure Java, but
> > > > should
> > > > be
> > > > fine. This is not yet fixed in the updated webrev.
> > > > 
> > > > > > Other:
> > > > > > 
> > > > > > Also - I believe the new option should be documented in:
> > > > > > src/java.management/share/conf/management.properties
> > > > 
> > > > Docs have been updated
> > > > in src/java.management/share/conf/management.properties.
> > > > 
> > > > > I share Daniel's concerns. Also, the part of the changeset is
> > > > > related
> > > > > to javax.rmi.ssl - someone maintaining this library should
> > > > > also
> > > > > comment here.
> > > > > 
> > > > > Also, the change is introducing new API (system property) and
> > > > > changing the existing one (adding SslRmiServerSocketFactory
> > > > > public
> > > > > constructors) so compatibility review process will have to be
> > > > > involved.
> > > > 
> > > > OK. What exactly is there for me to do? I'm not familiar with
> > > > this
> > > > process. Note that the intent would be to get this backported
> > > > to
> > > > JDK 8.
> > > Not much for you. But for the potential Oracle sponsor this means
> > > she
> > > will have to remember to go through some extra hoops before
> > > 

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

[Mandy Chung]

> On Nov 18, 2015, at 1:11 AM, Peter Levart  wrote:
> 
> Hi Mandy,
> 
> Just one nit...
> 
> On 11/17/2015 11:59 PM, Mandy Chung wrote:
>>> 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.
> 
> - if this {@code StackWalker} has been configured.
> + this {@code StackWalker} has been configured with.

Fixed.

> 
> Otherwise I think this is fine now.
> 
> I was just thinking of getCallerClass() suitability to be an instance method. 
> It requires StackWalker instance to be configured with RETAIN_CLASS_REFERENCE 
> and ignores SHOW_REFLECT_FRAMES and SHOW_HIDDEN_FRAMES options. Do you 
> anticipate other options in the future that could actually affect how 
> getCallerClass() operates? If not, then perhaps it could just be a static 
> method to simplify use. What do you think?
> 

It’s an instance method because the stack walker must have 
RETAIN_CLASS_REFERENCE option so that it will do stack-based permission check 
only at the getInstance time.  If it’s a static method, the getCallerClass will 
perform stack-based permission check at each call that we want to avoid.

Mandy

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

[Mandy Chung]

This is the update javadoc of getCallerClass.   Thanks for all the feedback and 
suggestion.  I have to finalize this API today to make the FC date :) 

/**
 * 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
 * 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,
 * This method should be called when a caller frame is present.  If
 * it is called from the last frame on the stack;
 * {@code IllegalStateException} will be thrown.
 *
 * @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 IllegalStateException} 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 IllegalStateException 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.
 */

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

[Mandy Chung]

I also think IllegalStateException is better than UOE for this case as 
getCallerClass is inappropriate to be called where there is no caller frame.

Mandy

> On Nov 18, 2015, at 5:12 AM, fo...@univ-mlv.fr wrote:
> 
> Hi peter,
> Given this is a stack walker, considering the stack as a state seems natural 
> to me.
> 
> Rémi
> 
> - Mail original -
>> De: "Peter Levart" 
>> À: "Remi Forax" , "David Holmes" 
>> Cc: "Mandy Chung" , "OpenJDK Dev list" 
>> 
>> Envoyé: Mercredi 18 Novembre 2015 13:26:32
>> Objet: Re: Proposed API for JEP 259: Stack-Walking API
>> 
>> 
>> 
>> On 11/18/2015 12:22 PM, Remi Forax wrote:
>>> - Mail original -
 De: "David Holmes" 
 À: "Mandy Chung" , "Peter Levart"
 
 Cc: "OpenJDK Dev list" 
 Envoyé: Mercredi 18 Novembre 2015 08:58:56
 Objet: Re: Proposed API for JEP 259: Stack-Walking API
 
 On 18/11/2015 8:42 AM, Mandy Chung wrote:
>> On Nov 17, 2015, at 2:09 PM, Peter Levart 
>> wrote:
>> 
>> 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.
>> 
> I’m now convinced that it’s not a good idea to special case it.
> getCallerClass will simply return the caller frame (i.e. top-2) on the
> stack and throw UOE if there is no caller frame.  The user should call
> StackWalker::walk instead if this special case matters.
 That sounds good to me too.
 
 David
>>> Looks good to me too if IllegalStateException is used instead of
>>> UnsupportedOperationException.
>>> UnsuppportedOperationException is used when the operation is not available,
>>> here, the same code can work or not depending how it is called.
>> 
>> But IllegalStateException is dependent on some state. There's no state
>> involved here (in the sense "state" is characterized in Java). My 1st
>> thought was an IllegalArgumentException. This requires some imagination
>> to view the caller passed to the method as an implicit argument.
>> 
>> There's an obscure java.util.EmptyStackException but that is reserved
>> for java.util.Stack operations.
>> 
>> If we consider the call stack to be part of the Thread state, then maybe
>> java.lang.IllegalThreadStateException (a subclass of
>> IllegalArgumentException) could be used...
>> 
>> Regards, Peter
>> 
>>> Runnable r = () -> System.out.println(stackWalker.getCallerClass());
>>> new Thread(r).start() // throw ISE
>>> r.run();  // prints main class
>>> 
>>> Rémi
>>> 
> How does this look?
> 
> /**
>   * 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,
>   * i.e. invoked from a JNI attached thread (
>   * for example, {@code static public void main} method launched by the
>   * {@code java} launcher).
>   *
>   * @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,
>   caller.getClassLoader());
>   * }
>   * }
>   *
>   * class MyTool {
>   * private void init() {
>   * ResourceBundle rb = Util.getResourceBundle("mybundle");
>   * }
>   * }
>   * }
>   *
>   * An equivalent way to find the caller class using the
>   * {@link StackWalker#walk 

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

[Brent Christian]

Hi, Mandy

I looked through the jdk code.  I think it's looking quite good.  I just 
noticed some small details to consider fixing up before pushing.


Docs:

StackWalker.StackFrame.getClassName():
the "binary name" link seems to be broken
(StackWalker.java line 100)

StackWalker.getInstance(Set options):
StackWalker.getInstance(Set options, int estimateDepth):

A couple missing words:

"Returns a StackWalker instance with the given *options* specifying..."

"If the given *["options"|"set"]* is empty, ..."

(Looks like a typo ("@ocde") on StackWalker.java lines 278 & 307)


Code:

==
jdk/src/java.base/share/classes/sun/util/logging/PlatformLogger.java

550:
The comment for get() states that it returns a StackTraceElement

==
hotspot/src/share/vm/prims/jvm.h (219)
jdk/src/java.base/share/native/include/jvm.h (196)

FWIW, the start_index argument of
JVM_FillStackFrames is an int in the hotspot file, and a jint in the jdk 
file.


==
jdk/src/java.logging/share/classes/java/util/logging/LogRecord.java

658:
The comment for get() states that it returns a StackTraceElement

==
jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java

 137 // VM writes to these arrays to fill the stack frame 
information

 138 // for each batch

I think this comment applies to a previous version of the code.


 195  * Subclass should override this method to change the 
batch size
 196  * or invoke the function passed to the StackWalker::walk 
method


I believe the function in question is the batchSizeMapper function, no 
longer being passed to walk(); line 196 can be removed.


==
jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java

66
Perhaps this.memberName does not need to be allocated in the case of 
-XX:-MemberNameInStackFrame ?


Thanks,
-Brent

On 11/16/15 4:13 PM, 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

Re: RFR: jsr166 jdk9 integration wave 2

[Martin Buchholz]

On Tue, Nov 17, 2015 at 2:26 AM, Paul Sandoz  wrote:

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

Most of the jtreg tests deliberately include non-determinism from use of
multiple threads, and there is no @key nondeterminstic tag, and adding
randomness tags to the tests that additionally invoke a prng doesn't really
seem useful.  Flakes R Us!

I support adding @key nondeterministic, and allowing that a tag to apply to
an entire tree (java.util.concurrent).  Do we have any tests dependent on
the phase of the moon?

Re: RFR: JDK-8140364: JEP 264 Platform Logger API and Service Implementation

[Mandy Chung]

> On Nov 3, 2015, at 12:12 PM, Daniel Fuchs  wrote:
> 
> Hi Mandy,
> 
> I have pushed an update that adds the diagnosability tweaks
> you asked me for in LoggerFinderLoader.
> I also added a couple of new tests.
> No changes in the API specification.
> :
> 
> http://cr.openjdk.java.net/~dfuchs/8046565/proto.02/webrev/index.html
> http://cr.openjdk.java.net/~dfuchs/8046565/proto.02/specdiff-api/overview-summary.html

This all looks good.

Mandy

Re: RFR: 8143253: java/lang/invoke/CompileThresholdBootstrapTest.java failing on mach5

[Claes Redestad]

Thanks!

/Claes

On 2015-11-18 20:56, Lance Andersen wrote:

lOOks ok
On Nov 18, 2015, at 2:22 PM, Claes Redestad > wrote:



Hi,

seems I pushed out the wrong version of CompileThresholdBootstrapTest 
with JDK-8143232 and didn't look twice enough. Any takers for this 
trivial test fix?


Webrev: cr.openjdk.java.net/~redestad/8143253/webrev.01 


Bugs: https://bugs.openjdk.java.net/browse/JDK-8143253

/Claes




Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Michael Haupt]

Hi Vladimir,

> Am 17.11.2015 um 13:08 schrieb Vladimir Ivanov :
> Awesome! Looks really good, Michael!

thank you very much.

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

This is the implementation of the findSpecial capability to bind non-abstract 
methods in super interfaces, as mentioned in the JEP document.

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

Yes, Claes had given me a heads-up early on (thanks Claes!), and the merge was 
easy enough. The new webrev is here: 
http://cr.openjdk.java.net/~mhaupt/8139885/webrev.01/

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

Absolutely; this is on my to-do list anyway. 
https://bugs.openjdk.java.net/browse/JDK-8143211

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

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

[Mandy Chung]

> On Nov 17, 2015, at 2:09 PM, Peter Levart  wrote:
> 
> 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. 
> 

I’m now convinced that it’s not a good idea to special case it.  getCallerClass 
will simply return the caller frame (i.e. top-2) on the stack and throw UOE if 
there is no caller frame.  The user should call StackWalker::walk instead if 
this special case matters.

How does this look?

/**
 * 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,
 * i.e. invoked from a JNI attached thread (
 * for example, {@code static public void main} method launched by the
 * {@code java} launcher).
 *
 * @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, 
caller.getClassLoader());
 * }
 * }
 *
 * class MyTool {
 * 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, i.e. invoked from a JNI attached thread,
 * for example, {@code static public void main} method launched by the
 * {@code java} launcher,
 *
 * @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

[Timo Kinnunen]

Hi, 

I’m happy to hear that, I was just about to post more convoluted examples 
against special-casing (i.e.

new Thread(r).start();
vs.
new Thread(r) {}.start();
vs.
new Thread(new Thread(r)) {}.start();
vs.
new Thread(new Thread(r) {}).start();
). 

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



Sent from Mail for Windows 10



From: Mandy Chung
Sent: Tuesday, November 17, 2015 23:43
To: Peter Levart
Cc: OpenJDK Dev list
Subject: Re: Proposed API for JEP 259: Stack-Walking API



> On Nov 17, 2015, at 2:09 PM, Peter Levart  wrote:
> 
> 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. 
> 

I’m now convinced that it’s not a good idea to special case it.  getCallerClass 
will simply return the caller frame (i.e. top-2) on the stack and throw UOE if 
there is no caller frame.  The user should call StackWalker::walk instead if 
this special case matters.

How does this look?

/**
 * 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,
 * i.e. invoked from a JNI attached thread (
 * for example, {@code static public void main} method launched by the
 * {@code java} launcher).
 *
 * @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, 
caller.getClassLoader());
 * }
 * }
 *
 * class MyTool {
 * 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, i.e. invoked from a JNI attached thread,
 * for example, {@code static public void main} method launched by the
 * {@code java} launcher,
 *
 * @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

[Christian Thalinger]

Kind of related but not really, I’ve filed a bug against ConstantPool to add 
JSR 292 “support”:

JDK-8077229: sun.reflect.ConstantPool should support class and method constant 
pools
https://bugs.openjdk.java.net/browse/JDK-8077229 


> On Nov 17, 2015, at 11:04 AM, Remi Forax  wrote:
> 
> 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: RFR: jsr166 jdk9 integration wave 2

[Doug Lea]

On 11/17/2015 07:43 AM, Paul Sandoz wrote:


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.



Because changes will probably be applied to 8u, I was (overly) careful to
preserve exact exception forms. But I don't think there is a reason
to do so for jdk9, so we might want to look into this after the rest
of VarHandles stablilize, possibly needing an uninteresting CCC change
to allow uninterestingly different exceptions.

-Doug

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

[Peter Levart]

Hi Mandy,

Just one nit...

On 11/17/2015 11:59 PM, Mandy Chung wrote:

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.


- if this {@code StackWalker} has been configured.
+ this {@code StackWalker} has been configured with.

Otherwise I think this is fine now.

I was just thinking of getCallerClass() suitability to be an instance 
method. It requires StackWalker instance to be configured with 
RETAIN_CLASS_REFERENCE and ignores SHOW_REFLECT_FRAMES and 
SHOW_HIDDEN_FRAMES options. Do you anticipate other options in the 
future that could actually affect how getCallerClass() operates? If not, 
then perhaps it could just be a static method to simplify use. What do 
you think?


Regards, Peter

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

[Peter Levart]

On 11/18/2015 12:22 PM, Remi Forax wrote:

- Mail original -

De: "David Holmes" 
À: "Mandy Chung" , "Peter Levart" 

Cc: "OpenJDK Dev list" 
Envoyé: Mercredi 18 Novembre 2015 08:58:56
Objet: Re: Proposed API for JEP 259: Stack-Walking API

On 18/11/2015 8:42 AM, Mandy Chung wrote:

On Nov 17, 2015, at 2:09 PM, Peter Levart  wrote:

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.


I’m now convinced that it’s not a good idea to special case it.
getCallerClass will simply return the caller frame (i.e. top-2) on the
stack and throw UOE if there is no caller frame.  The user should call
StackWalker::walk instead if this special case matters.

That sounds good to me too.

David

Looks good to me too if IllegalStateException is used instead of 
UnsupportedOperationException.
UnsuppportedOperationException is used when the operation is not available, 
here, the same code can work or not depending how it is called.


But IllegalStateException is dependent on some state. There's no state 
involved here (in the sense "state" is characterized in Java). My 1st 
thought was an IllegalArgumentException. This requires some imagination 
to view the caller passed to the method as an implicit argument.


There's an obscure java.util.EmptyStackException but that is reserved 
for java.util.Stack operations.


If we consider the call stack to be part of the Thread state, then maybe 
java.lang.IllegalThreadStateException (a subclass of 
IllegalArgumentException) could be used...


Regards, Peter


Runnable r = () -> System.out.println(stackWalker.getCallerClass());
new Thread(r).start() // throw ISE
r.run();  // prints main class

Rémi


How does this look?

/**
   * 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,
   * i.e. invoked from a JNI attached thread (
   * for example, {@code static public void main} method launched by the
   * {@code java} launcher).
   *
   * @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,
   caller.getClassLoader());
   * }
   * }
   *
   * class MyTool {
   * 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, i.e. invoked from a JNI attached
   thread,
   * for example, {@code static public void main} method launched by the
   * {@code java} launcher,
   *
   * @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: jsr166 jdk9 integration wave 2

[Paul Sandoz]

> On 18 Nov 2015, at 03:46, Martin Buchholz  wrote:
> 
> 
> 
> On Tue, Nov 17, 2015 at 2:26 AM, Paul Sandoz  > wrote:
> 
> 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.
> 
> Most of the jtreg tests deliberately include non-determinism from use of 
> multiple threads, and there is no @key nondeterminstic tag, and adding 
> randomness tags to the tests that additionally invoke a prng doesn't really 
> seem useful.  Flakes R Us!
> 

I am sure Joe, CC’ed will have an opinion on this. IIUC correct the existing 
randomness tag actually serves a simialr purpose as your proposed 
nondeterminstic tag.


> I support adding @key nondeterministic, and allowing that a tag to apply to 
> an entire tree (java.util.concurrent).  Do we have any tests dependent on the 
> phase of the moon?

Dunno :-) perhaps every one in a while a machine running tests is effected by 
neutrinos and causes a one off error?

Paul.

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

[David Holmes]

On 18/11/2015 1:30 AM, Rob McKenna wrote:

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?


I'd vote for a 7 (and 8?) specific fix. Didn't this already get fixed in 6.

David


 -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: Proposed API for JEP 259: Stack-Walking API

[David Holmes]

On 18/11/2015 8:42 AM, Mandy Chung wrote:



On Nov 17, 2015, at 2:09 PM, Peter Levart  wrote:

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.



I’m now convinced that it’s not a good idea to special case it.  getCallerClass 
will simply return the caller frame (i.e. top-2) on the stack and throw UOE if 
there is no caller frame.  The user should call StackWalker::walk instead if 
this special case matters.


That sounds good to me too.

David


How does this look?

/**
  * 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,
  * i.e. invoked from a JNI attached thread (
  * for example, {@code static public void main} method launched by the
  * {@code java} launcher).
  *
  * @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, 
caller.getClassLoader());
  * }
  * }
  *
  * class MyTool {
  * 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, i.e. invoked from a JNI attached thread,
  * for example, {@code static public void main} method launched by the
  * {@code java} launcher,
  *
  * @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: 8143232: Fix java.lang.invoke bootstrap when specifying COMPILE_THRESHOLD

[Vladimir Ivanov]

Reviewed.

Best regards,
Vladimir Ivanov

On 11/18/15 5:41 PM, Claes Redestad wrote:

Hi,

a recent change of mine to make LF bootstrap lazier[1] breaks when
explicitly interpreting LambdaForms due to a circular dependency. By
explicitly compiling identity and zero forms bootstrap to bytecode we
avoid this corner case, while maintaining the benefits of lazy
initialization. Also added a simple regression test.

Webrev: http://cr.openjdk.java.net/~redestad/8143232/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8143232

/Claes

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

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

[Remi Forax]

- Mail original -
> De: "David Holmes" 
> À: "Mandy Chung" , "Peter Levart" 
> 
> Cc: "OpenJDK Dev list" 
> Envoyé: Mercredi 18 Novembre 2015 08:58:56
> Objet: Re: Proposed API for JEP 259: Stack-Walking API
> 
> On 18/11/2015 8:42 AM, Mandy Chung wrote:
> >
> >> On Nov 17, 2015, at 2:09 PM, Peter Levart  wrote:
> >>
> >> 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.
> >>
> >
> > I’m now convinced that it’s not a good idea to special case it.
> > getCallerClass will simply return the caller frame (i.e. top-2) on the
> > stack and throw UOE if there is no caller frame.  The user should call
> > StackWalker::walk instead if this special case matters.
> 
> That sounds good to me too.
> 
> David

Looks good to me too if IllegalStateException is used instead of 
UnsupportedOperationException.
UnsuppportedOperationException is used when the operation is not available, 
here, the same code can work or not depending how it is called.

Runnable r = () -> System.out.println(stackWalker.getCallerClass());
new Thread(r).start() // throw ISE
r.run();  // prints main class

Rémi

> 
> > How does this look?
> >
> > /**
> >   * 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,
> >   * i.e. invoked from a JNI attached thread (
> >   * for example, {@code static public void main} method launched by the
> >   * {@code java} launcher).
> >   *
> >   * @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,
> >   caller.getClassLoader());
> >   * }
> >   * }
> >   *
> >   * class MyTool {
> >   * 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, i.e. invoked from a JNI attached
> >   thread,
> >   * for example, {@code static public void main} method launched by the
> >   * {@code java} launcher,
> >   *
> >   * @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
> >
> 

RFR: 8143232: Fix java.lang.invoke bootstrap when specifying COMPILE_THRESHOLD

[Claes Redestad]

Hi,

a recent change of mine to make LF bootstrap lazier[1] breaks when 
explicitly interpreting LambdaForms due to a circular dependency. By 
explicitly compiling identity and zero forms bootstrap to bytecode we 
avoid this corner case, while maintaining the benefits of lazy 
initialization. Also added a simple regression test.


Webrev: http://cr.openjdk.java.net/~redestad/8143232/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8143232

/Claes

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

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

[Konstantin Shefov]

Remi,

Thanks for reviewing. Your suggestion sounds sensibly.
May be it also has sense to make a method 
"getMethodRefNameAndTypeIndex(int index)" to acquire name-and-type entry 
index for methods also.


-Konstantin

On 11/18/2015 12:04 AM, Remi Forax wrote:

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

[Coleen Phillimore]

On 11/18/15 5:06 PM, Mandy Chung wrote:

On Nov 18, 2015, at 1:01 PM, Coleen Phillimore  
wrote:


One of the things that I'm struggling with is that StackFrameInfo contains both 
the collected information from walking the stack frames, method id, bci, 
mirror, version and cpref, and the digested information: interned string for 
class name, method name, line number and source file name.


method id, mirror, version and cpref are injected in StackFrameInfo.  I hope 
there is a way to make it conditional only if -XX:-MemberNameInThrowable is set 
(is it possible?)


That's really hard to do with the nice macros we have now in javaClasses.


-XX:-MemberNameInThrowable is temporary and disabled by default.  It is used 
for follow-on performance improvement as we discussed previously.   I still 
believe that there may be some low hanging fruit to reduce the initialization 
of MemberName for an already-linked method.We should aim to remove this 
flag in JDK 9 so that StackFrameInfo will have only MemberName and bci.


Given that that we were trying to stick to the original feature freeze 
date for 9, I don't think the performance of the MethodHandles code 
would make it in time.  There are some big problems with it, notably 
that it creates weak references for each MemberName and the GC people 
are not going to like that.   We have not to-date found a better 
solution for this to support redefinition.


I think if StackFrameInfo was trimmed to just what was needed for 
collecting the information during stack traces, it would be possible to 
make the new implementation performant enough to be low risk for 9 *and* 
would allow for removing the duplicated code in BacktraceBuilder.  This 
would be a very promising improvement!


The interned string for class name, method name, line number and source file 
name are requested lazily when StackFrame::getMethodName or other methods are 
called.  They are not eagerly allocated.


But the fields in StackFrameInfo are present for each element in the 
stack trace.  We had problems with GC scavenge times when we increased 
the size of the backtrace that we collect today.   The StackFrameInfo 
struct would be similarly sized if you didn't all the fields from 
StackTraceElement, so it would be good.



If this is to replace stack walking for exceptions, this will more than double 
the footprint of the information collected but rarely used. I don't understand 
why the digested information isn't still StackFrameElement?


If Throwable uses StackWalker, I expect it to use MemberName and 
-XX:-MemberNameInThrowable should be removed by that time.   Also VM no longer 
needs to fill in StackTraceElement as it should fill in StackFrameInfo.   
java_lang_StackTraceElement in javaClasses.[hc]pp can be removed at an 
appropriate time :)


I don't know why StackTraceElement should be removed.  What's wrong with 
StackTraceElement?


Throwable backtrace will keep an array of StackFrameInfo, one element per 
frame.  StackFrameInfo only captures the MemberName + bci.


Right (or the combination of things that we save now in the backtraces 
for efficiency).




When Throwable::getStackTraceElements() or printStackTrace() is called, the VM 
will allocate the intern strings for those names and fill in StackFrameInfo.  
Then convert them to StackTraceElement[] and throw away StackFrameInfo[].   
This is just the current implementation.   I expect further optimization can be 
done in the JDK side about StackTraceElement and StackFrameInfo.


This sounds really inefficient!  Why not fill in StackTraceElement 
directly?  And keep it?


Even in Java, having one class represent two different things isn't very 
object oriented.



That's just a high level comment.  I haven't read the java code yet for the 
rationale why this type is used for two different things.


The way I implement it is to prepare Throwable backtrace + StackTraceElement be 
replaced by StackFrameInfo in the VM.

The VM fills in StackFrameInfo for StackWalker.   When Throwable switches to 
use StackWalker, VM doesn’t need to know anything about StackTraceElement.


I don't see the advantage of this. 
java_lang_StackFrameInfo::fill_methodInfo() could just fill in a Java 
allocated array of StackTraceElement instead.  Again, making 
StackFrameInfo so large will have footprint and GC performance 
implications when it's almost always thrown away.  I included GC in the 
mailing list.  Hopefully with enough context if they want to comment.


thanks,
Coleen



Mandy

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

[Mandy Chung]

> On Nov 18, 2015, at 1:01 PM, Coleen Phillimore  
> wrote:
> 
> 
> One of the things that I'm struggling with is that StackFrameInfo contains 
> both the collected information from walking the stack frames, method id, bci, 
> mirror, version and cpref, and the digested information: interned string for 
> class name, method name, line number and source file name.
> 

method id, mirror, version and cpref are injected in StackFrameInfo.  I hope 
there is a way to make it conditional only if -XX:-MemberNameInThrowable is set 
(is it possible?)

-XX:-MemberNameInThrowable is temporary and disabled by default.  It is used 
for follow-on performance improvement as we discussed previously.   I still 
believe that there may be some low hanging fruit to reduce the initialization 
of MemberName for an already-linked method.We should aim to remove this 
flag in JDK 9 so that StackFrameInfo will have only MemberName and bci.

The interned string for class name, method name, line number and source file 
name are requested lazily when StackFrame::getMethodName or other methods are 
called.  They are not eagerly allocated.

> If this is to replace stack walking for exceptions, this will more than 
> double the footprint of the information collected but rarely used. I don't 
> understand why the digested information isn't still StackFrameElement?
> 

If Throwable uses StackWalker, I expect it to use MemberName and 
-XX:-MemberNameInThrowable should be removed by that time.   Also VM no longer 
needs to fill in StackTraceElement as it should fill in StackFrameInfo.   
java_lang_StackTraceElement in javaClasses.[hc]pp can be removed at an 
appropriate time :)

Throwable backtrace will keep an array of StackFrameInfo, one element per 
frame.  StackFrameInfo only captures the MemberName + bci.  When 
Throwable::getStackTraceElements() or printStackTrace() is called, the VM will 
allocate the intern strings for those names and fill in StackFrameInfo.  Then 
convert them to StackTraceElement[] and throw away StackFrameInfo[].   This is 
just the current implementation.   I expect further optimization can be done in 
the JDK side about StackTraceElement and StackFrameInfo.

> That's just a high level comment.  I haven't read the java code yet for the 
> rationale why this type is used for two different things.
> 

The way I implement it is to prepare Throwable backtrace + StackTraceElement be 
replaced by StackFrameInfo in the VM.

The VM fills in StackFrameInfo for StackWalker.   When Throwable switches to 
use StackWalker, VM doesn’t need to know anything about StackTraceElement.

Mandy

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

[Brent Christian]

On 11/18/15 2:24 PM, Mandy Chung wrote:

==
>jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java
>
>66
>Perhaps this.memberName does not need to be allocated in the case of 
-XX:-MemberNameInStackFrame ?
>

For more accurate footprint comparison, yes it should not allocate MemberName.  
Same for the injected fields if -XX:+MemberNameInStackFrame is set which is the 
default.  I prefer to leave it as the follow-on work for JDK-8141239.  This 
should give a good starting point to do performance measurement for Throwable.


That's fine with me.

Thanks,
-Brent

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

[Aleksey Shipilev]

On 11/18/2015 01:08 AM, Claes Redestad wrote:
> 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;
> }

By the way, I see there is a cleaner way to implement emitIconstInsn,
see java.lang.invoke.TypeConvertingMethodAdapter.iconst:

void iconst(final int cst) {
if (cst >= -1 && cst <= 5) {
mv.visitInsn(Opcodes.ICONST_0 + cst);
} else if (cst >= Byte.MIN_VALUE && cst <= Byte.MAX_VALUE) {
mv.visitIntInsn(Opcodes.BIPUSH, cst);
} else if (cst >= Short.MIN_VALUE && cst <= Short.MAX_VALUE) {
mv.visitIntInsn(Opcodes.SIPUSH, cst);
} else {
mv.visitLdcInsn(cst);
}
}

http://hg.openjdk.java.net/jdk9/dev/jdk/file/aa9e8b3916ae/src/java.base/share/classes/java/lang/invoke/TypeConvertingMethodAdapter.java#l285

Thanks,
-Aleksey

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

[David Holmes]

On 18/11/2015 10:26 PM, Peter Levart wrote:



On 11/18/2015 12:22 PM, Remi Forax wrote:

- Mail original -

De: "David Holmes" 
À: "Mandy Chung" , "Peter Levart"

Cc: "OpenJDK Dev list" 
Envoyé: Mercredi 18 Novembre 2015 08:58:56
Objet: Re: Proposed API for JEP 259: Stack-Walking API

On 18/11/2015 8:42 AM, Mandy Chung wrote:

On Nov 17, 2015, at 2:09 PM, Peter Levart 
wrote:

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.


I’m now convinced that it’s not a good idea to special case it.
getCallerClass will simply return the caller frame (i.e. top-2) on the
stack and throw UOE if there is no caller frame.  The user should call
StackWalker::walk instead if this special case matters.

That sounds good to me too.

David

Looks good to me too if IllegalStateException is used instead of
UnsupportedOperationException.
UnsuppportedOperationException is used when the operation is not
available, here, the same code can work or not depending how it is
called.


But IllegalStateException is dependent on some state. There's no state
involved here (in the sense "state" is characterized in Java). My 1st


I agree with Remi. "state" doesn't have to mean fields - there are 
numerous existing examples in the JDK. Calling a method in a context 
that is invalid is an illegal state to me. IllegalThreadStateException 
would also work. But UnsupportedOperationException ... more of a stretch.


David
-


thought was an IllegalArgumentException. This requires some imagination
to view the caller passed to the method as an implicit argument.

There's an obscure java.util.EmptyStackException but that is reserved
for java.util.Stack operations.

If we consider the call stack to be part of the Thread state, then maybe
java.lang.IllegalThreadStateException (a subclass of
IllegalArgumentException) could be used...

Regards, Peter


Runnable r = () -> System.out.println(stackWalker.getCallerClass());
new Thread(r).start() // throw ISE
r.run();  // prints main class

Rémi


How does this look?

/**
   * 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,
   * i.e. invoked from a JNI attached thread (
   * for example, {@code static public void main} method launched by
the
   * {@code java} launcher).
   *
   * @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,
   caller.getClassLoader());
   * }
   * }
   *
   * class MyTool {
   * 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, i.e. invoked from a JNI attached
   thread,
   * for example, {@code static public void main} method launched by
the
   * {@code java} launcher,
   *
   * @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
   * 

JEP 280: Indify String Concatenation

[mark . reinhold]

New JEP Candidate: http://openjdk.java.net/jeps/280

- Mark

Re: RFR: 8143253: java/lang/invoke/CompileThresholdBootstrapTest.java failing on mach5

[Lance Andersen]

lOOks ok
On Nov 18, 2015, at 2:22 PM, Claes Redestad  wrote:

> Hi,
> 
> seems I pushed out the wrong version of CompileThresholdBootstrapTest with 
> JDK-8143232 and didn't look twice enough. Any takers for this trivial test 
> fix?
> 
> Webrev: cr.openjdk.java.net/~redestad/8143253/webrev.01
> Bugs: https://bugs.openjdk.java.net/browse/JDK-8143253
> 
> /Claes



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

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

[Mandy Chung]

> On Nov 18, 2015, at 11:00 AM, Brent Christian  
> wrote:
> 
> Hi, Mandy
> 
> I looked through the jdk code.  I think it's looking quite good.  I just 
> noticed some small details to consider fixing up before pushing.
> 
> Docs:
> 
> StackWalker.StackFrame.getClassName():
> the "binary name" link seems to be broken
> (StackWalker.java line 100)
> 

It works for me.  It looks correct to me.

> StackWalker.getInstance(Set options):
> StackWalker.getInstance(Set options, int estimateDepth):
> 
> A couple missing words:
> 
> "Returns a StackWalker instance with the given *options* specifying..."
> 
> "If the given *["options"|"set"]* is empty, ..."
> 
> (Looks like a typo ("@ocde") on StackWalker.java lines 278 & 307)
> 

Fixed.

> 
> Code:
> 
> ==
> jdk/src/java.base/share/classes/sun/util/logging/PlatformLogger.java
> 
> 550:
> The comment for get() states that it returns a StackTraceElement
> 

Fixed.

> ==
> hotspot/src/share/vm/prims/jvm.h (219)
> jdk/src/java.base/share/native/include/jvm.h (196)
> 
> FWIW, the start_index argument of
> JVM_FillStackFrames is an int in the hotspot file, and a jint in the jdk file.

Fixed.

> 
> ==
> jdk/src/java.logging/share/classes/java/util/logging/LogRecord.java
> 
> 658:
> The comment for get() states that it returns a StackTraceElement
> 

Fixed.

> ==
> jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java
> 
> 137 // VM writes to these arrays to fill the stack frame information
> 138 // for each batch
> 
> I think this comment applies to a previous version of the code.
> 
> 
> 195  * Subclass should override this method to change the batch size
> 196  * or invoke the function passed to the StackWalker::walk method
> 
> I believe the function in question is the batchSizeMapper function, no longer 
> being passed to walk(); line 196 can be removed.

Fixed.

> 
> ==
> jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java
> 
> 66
> Perhaps this.memberName does not need to be allocated in the case of 
> -XX:-MemberNameInStackFrame ?
> 

For more accurate footprint comparison, yes it should not allocate MemberName.  
Same for the injected fields if -XX:+MemberNameInStackFrame is set which is the 
default.  I prefer to leave it as the follow-on work for JDK-8141239.  This 
should give a good starting point to do performance measurement for Throwable.  

Mandy

> 
> Thanks,
> -Brent
> 
> On 11/16/15 4:13 PM, 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
>> 

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

[Mandy Chung]

> On Nov 18, 2015, at 6:32 PM, David Holmes  wrote:
> 
> 
> I agree with Remi. "state" doesn't have to mean fields - there are numerous 
> existing examples in the JDK. Calling a method in a context that is invalid 
> is an illegal state to me. IllegalThreadStateException would also work. But 
> UnsupportedOperationException ... more of a stretch.

I also thought about IllegalThreadStateException.  It’s an old exception that 
will be repurposed if used for getCallerClass case and I think ISA would work.  
 FWIW - it extends IllegalArgumentException.

Mandy

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

[John Rose]

On Nov 13, 2015, at 8:39 AM, 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/ 
> 

Looks great.  One bug:

+static boolean isVoid(Class t) {
+return t == void.class || t == Void.class;
+}

I think there's nothing in the spec that calls for java.lang.Void to get 
special processing for try/finally.
If I'm right, get rid of isVoid; just check for void.class not Void.class.

I'd prefer to see reuse of misMatchedTypes() or newIllegalArgumentException() 
instead of err().  I would prefer to push formatting should into the 
error-producing subroutine.  This can be cleaned up later.  OTOH, the 
complexity of string concat instructions will be going down with Aleksey's 
work, so maybe I shouldn't be so sensitive to inline concat stuff.

My usual practice, though, is to move argument checking and error reporting 
code out of line, away from the main logic.  I think this is good style, and it 
gives the JIT a little bit (a very little bit) of help.

In that vein, the argument checking function foldArgumentChecks should stay 
private, shouldn't it?

I'm impressed to see how useful the Stream API is for doing the loop combinator.

Testing is good.  I'm glad to see new BigArity cases.

One wishful item:  It would be nice if the javadoc examples could be integrated 
into JavaDocExamplesTest.java.  I see that is messy since we are now using 
TestNG instead of JUnit.  The point of JavaDocExamplesTest was to make it easy 
to "sync" the javadoc with the examples, by having one place to copy-and-paste 
the javadoc.

Anyway, reviewed, conditional on that one bug fix.

— John

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

[John Rose]

On Nov 18, 2015, at 6:32 PM, David Holmes  wrote:
> 
>>> 
>>> Looks good to me too if IllegalStateException is used instead of
>>> UnsupportedOperationException.
>>> UnsuppportedOperationException is used when the operation is not
>>> available, here, the same code can work or not depending how it is
>>> called.
>> 
>> But IllegalStateException is dependent on some state. There's no state
>> involved here (in the sense "state" is characterized in Java). My 1st
> 
> I agree with Remi. "state" doesn't have to mean fields - there are numerous 
> existing examples in the JDK. Calling a method in a context that is invalid 
> is an illegal state to me. IllegalThreadStateException would also work. But 
> UnsupportedOperationException ... more of a stretch.

+1

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

[Coleen Phillimore]

One of the things that I'm struggling with is that StackFrameInfo 
contains both the collected information from walking the stack frames, 
method id, bci, mirror, version and cpref, and the digested information: 
interned string for class name, method name, line number and source file 
name.


If this is to replace stack walking for exceptions, this will more than 
double the footprint of the information collected but rarely used.  I 
don't understand why the digested information isn't still StackFrameElement?


That's just a high level comment.  I haven't read the java code yet for 
the rationale why this type is used for two different things.


Coleen

On 11/16/15 7:13 PM, 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

RFR: 8143253: java/lang/invoke/CompileThresholdBootstrapTest.java failing on mach5

[Claes Redestad]

Hi,

seems I pushed out the wrong version of CompileThresholdBootstrapTest 
with JDK-8143232 and didn't look twice enough. Any takers for this 
trivial test fix?


Webrev: cr.openjdk.java.net/~redestad/8143253/webrev.01
Bugs: https://bugs.openjdk.java.net/browse/JDK-8143253

/Claes

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

[Steve Drach]

Hi,

Please review the latest iteration of the webrev for runtime support of 
multi-release jar files.

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/ 

I believe I addressed all issues brought up in the last review as follows.

> In JarURLConnection then it would be good if the reference to multi-release 
> JARs should link to the description in the JarFile spec.

I included a comment in JarURLConnection referencing JarFile for more 
information regarding multi-release jar files.

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

I changed the property to jdk.util.jar.enableMultiRelease={true | false | 
force} as suggested.  I assume camel case is okay here.

> 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’m not sure what it means to reach across the file system.  The bulk of the 
multi-release tests are in jdk/test/java/util/jar/JarFile with the 
MultiReleaseJarURLConnection test in jdk/test/sun/net/www/protocol/jar.  Both 
test directories refer to the third directory, 
jdk/test/lib/testlibrary/java/util/jar, for common resources.  All three 
directories are in the jdk repo.  I don’t see anything obviously incorrect 
about where things are, the tests are in the same hierarchy as the components 
they test.  So pending further information, I left it as it was.

> 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 put the right margin for code at 100 and wrapped lines exceeding that.

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

I fixed it to return 0.

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

I clarified it in the documentation.  It now says "If the specified version is 
less than 9, then this JarFile is configured to be processed as if it is an 
unversioned jar file.”  I also made some additional clarifying documentation 
changes in overall documentation for JarFile.

> (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 …”?

I took the “is configured to be…” phrase out of the documentation.

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

We rewrote the isMultirelease() method to eliminate the race.  We also 
simplified it, removing the recursive invocation, with a small change to the 
getManEntry() method.

Thanks,
Steve