date:20171107

RE: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c

2017-11-07 Thread Ramanand Patil

Thank you Martin for your review.

For the java file updates, currently I don’t use any script but just simple 
“find and replace” whenever possible and rest is manual. I run all the TZ 
related tests locally on Ubuntu and use JPRT for all other platforms.

Regards,

Ramanand.

From: Martin Buchholz [mailto:marti...@google.com] 
Sent: Wednesday, November 08, 2017 12:18 AM
To: Ramanand Patil 
Cc: Naoto Sato ; core-libs-dev 
; i18n-...@openjdk.java.net
Subject: Re:  JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 
8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c

Looks good to me too.

I've always wondered about how the corresponding java source files are 
maintained.  Is it all manual or do y'all have some magic script to help keep 
IANA data and java data aligned?  Do we need more testing that mistakes don't 
creep in?

On Tue, Nov 7, 2017 at 3:41 AM, Ramanand Patil mailto:ramanand.pa...@oracle.com"ramanand.pa...@oracle.com> wrote:

Hi Naoto,
Thank you for pointing that. I have modified both the affected 
files(ZoneName.java from src and test).
Here is the updated Webrev: 
http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.01/

Regards,
Ramanand.

> -Original Message-
> From: Naoto Sato
> Sent: Tuesday, November 07, 2017 5:18 AM
> To: Ramanand Patil  "mailto:ramanand.pa...@oracle.com"ramanand.pa...@oracle.com>; core-libs-dev 
>  HYPERLINK "mailto:d...@openjdk.java.net"d...@openjdk.java.net>; HYPERLINK 
> "mailto:i18n-...@openjdk.java.net"i18n-...@openjdk.java.net
> Subject: Re:  JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c
> and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c
>
> Hi Ramanand,
>
> In java/time/format/ZoneName.java, should the zid for Africa_Central map
> to "Africa/Maputo"?
>
> Naoto
>
> On 11/6/17 6:06 AM, Ramanand Patil wrote:
> > Hi all,
> > Please review the latest TZDATA integration (tzdata2017c) to JDK10.
> > Bugs:
> > https://bugs.openjdk.java.net/browse/JDK-8190258
> > https://bugs.openjdk.java.net/browse/JDK-8190259
> >
> > Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.00/
> >
> > All the TimeZone related tests are passed after integration.
> >
> > Regards,
> > Ramanand.
> >

Re: RFR: jsr166 jdk10 integration wave 5

2017-11-07 Thread Martin Buchholz

On Mon, Nov 6, 2017 at 5:33 PM, David Holmes 
wrote:

> On 7/11/2017 8:17 AM, Martin Buchholz wrote:
>
>> Thanks for the review!
>>
>> On Mon, Nov 6, 2017 at 1:36 PM, David Holmes > > wrote:
>>
>>
>> 8189764: Miscellaneous changes imported from jsr166 CVS 2017-11
>>
>>
>> All seem okay. Though I'm curious about the changes from
>> "catch(Throwable" to "catch(Exception" ?
>>
>>
>> There's a half-hearted attempt to appease
>> http://errorprone.info/bugpattern/TryFailThrowable
>> No actual bugs were fixed.
>>
>
> Hmmm. Not sure I see a problem if threadUnexpectedException just wraps and
> rethrows the original exception. On the other hand Errors are no longer
> being handled this way. Probably doesn't make a difference either way.
>

Yes, probably doesn't make a difference either way.  These  TryFailThrowable
 warnings just don't
have enough value, so they are all reverted back to catch (Throwable).

Re: [NEW BUG][JDK10] TimeUnit.timedWait(Object obj, long timeout) doc should emphasize possibility of IllegalMonitorStateException

2017-11-07 Thread Martin Buchholz

Thanks - you're right - I filed
https://bugs.openjdk.java.net/browse/JDK-8190889


On Tue, Nov 7, 2017 at 2:25 PM, Christoph Dreis 
wrote:

> Hi,
>
> I don't know if this is considered a real bug or just a lack of
> documentation, but I found that although the example in the documentation
> of
> TimeUnit.timedWait(Object obj, long timeout) shows the correct usage of the
> method, I think at least the documentation should emphasize the possibility
> of an IllegalMonitorStateException in case it isn't executed inside a
> synchronized block.
>
> The method seems to be not used inside the JDK itself after all, but people
> might use it.
>
> Please find a minimal reproduction and a possible documentation patch
> below.
> In case it is considered worthwhile I'd be happy if this is sponsored.
>
> Cheers,
> Christoph
>
> = MINIMAL REPRO ==
> public class Main {
>
> public static void main(String[] args) {
> Lock lock = new ReentrantLock();
> try {
> TimeUnit.MILLISECONDS.timedWait(lock, 1);
> } catch (InterruptedException e) {
> e.printStackTrace();
> }
> }
>
> }
>
> == PATCH ==
> diff -r 67aa34b019e1
> src/java.base/share/classes/java/util/concurrent/TimeUnit.java
> --- a/src/java.base/share/classes/java/util/concurrent/TimeUnit.java
> Mon
> Nov 06 17:48:00 2017 -0800
> +++ b/src/java.base/share/classes/java/util/concurrent/TimeUnit.java
> Tue
> Nov 07 23:09:24 2017 +0100
> @@ -356,6 +356,8 @@
>   * @param timeout the maximum time to wait. If less than
>   * or equal to zero, do not wait at all.
>   * @throws InterruptedException if interrupted while waiting
> + * @throws IllegalMonitorStateException if not executed inside a
> + * synchronized block
>   */
>  public void timedWait(Object obj, long timeout)
>  throws InterruptedException {
>
>

Re: JDK-8067661: transferTo proposal for Appendable

2017-11-07 Thread Brian Burkhalter

Hi Patrick,

On Nov 7, 2017, at 3:01 PM, Patrick Reinhart  wrote:

> I integrated your changes into my current version which I will look into 
> tomorrow with Alan at Devoxx. I think adding some implementation note on the 
> default method implementation will be needed to review later on…

I concur.

> I also made an override on the CharBuffer, where we need to add a enhanced 
> documentation for the specialties around the CharBuffer implementations 
> (BufferOverflowException/ReadOnlyBufferException), where a review of some 
> native english speakers would help :-)

Sounds good!

Thanks,

Brian

Re: RFR 8190884: java/lang/Runtime/exec/LotsOfOutput fails intermittently

2017-11-07 Thread mandy chung


+1

Mandy

On 11/7/17 2:55 PM, Roger Riggs wrote:

Please review a test update to run LotsOfOutput with 'othervm';
running under the default batch testing mode of agentvm may cause 
instability.


--- old/test/jdk/java/lang/Runtime/exec/LotsOfOutput.java 2017-11-07 
17:48:38.398907014 -0500
+++ new/test/jdk/java/lang/Runtime/exec/LotsOfOutput.java 2017-11-07 
17:48:37.846921042 -0500

@@ -23,7 +23,8 @@

 /**
  * @test
- * @bug 4369826 8078582
+ * @bug 4369826 8078582 8190884
+ * @run main/othervm LotsOfOutput
  * @summary Process with lots of output should not crash VM
  * @author kladko
  */

Thanks, Roger

Re: JDK-8067661: transferTo proposal for Appendable

2017-11-07 Thread Patrick Reinhart

Hi Brian,

I integrated your changes into my current version which I will look into 
tomorrow with Alan at Devoxx. I think adding some implementation note on the 
default method implementation will be needed to review later on…

I also made an override on the CharBuffer, where we need to add a enhanced 
documentation for the specialties around the CharBuffer implementations 
(BufferOverflowException/ReadOnlyBufferException), where a review of some 
native english speakers would help :-)

-Patrick


> Am 07.11.2017 um 23:38 schrieb Brian Burkhalter :
> 
> I am a little late to this thread, but some alternative verbiage to consider 
> is included below. This wording however diverges from that of [1] so if we 
> want them to remain similar then it’s probably not worth it to change from 
> what has been agreed upon already.
> 
> Thanks,
> 
> Brian
> 
> [1] 
> https://docs.oracle.com/javase/9/docs/api/java/io/InputStream.html#transferTo-java.io.OutputStream-
>  
> 
> 
>   57  * Reads all characters from this source and writes the characters to
>   58  * the given appendable in the order that they are read. On return, 
> the
>   59  * source of characters will be at its end.
> 
> Reads all characters from this source and appends them to a destination in
> the order in which they are read. On return, ...
> 
>   60  * 
>   61  * This method may block indefinitely reading from the readable, or
>   62  * writing to the appendable. Where this source or the appendable is
>   63  * {@link AutoCloseable closeable}, then the behavior when either are
>   64  * asynchronously closed, or the thread interrupted during 
> the transfer,
>   65  * is highly readable and appendable specific, and therefore not 
> specified.
> 
> This method may block indefinitely while reading from the source or writing 
> to the destination.
> If the source or destination is {@link AutoCloseable closeable}, then the 
> behavior when either
> is asynchronously closed, or the thread is interrupted during the 
> transfer, is highly
> implementation-dependent and hence unspecified.
> 
>   66  * 
>   67  * If an I/O error occurs reading from the readable or writing to the
>   68  * appendable, then it may do so after some characters have been 
> read or
>   69  * written. Consequently the readable may not be at end of its data 
> and
>   70  * one, or both participants may be in an inconsistent state. That 
> in mind
>   71  * all additional measures required by one or both participants in 
> order to
>   72  * eventually free their internal resources have to be taken by the 
> caller
>   73  * of this method.
> 
> If an I/O error occurs during the operation, then not all characters might 
> have been transferred
> and the source or destination could be left in an inconsistent state. The 
> caller of this method
> should therefore ensure in such a case that measures are taken to release any 
> resources held
> by the source and destination.
> 
> On Nov 3, 2017, at 5:49 AM, Alan Bateman  > wrote:
> 
>> On 02/11/2017 19:55, Patrick Reinhart wrote:
>>> :
>>> If we are all happy with the API so far, I could start adding an initial 
>>> implementation and test…
>>> 
>> I think the API looks fine so go ahead.
>

Re: [Patch][JDK10] Use Class.getPackageName() where possible

2017-11-07 Thread mandy chung




On 11/7/17 6:48 AM, Christoph Dreis wrote:

=== PATCH ===
diff -r 67aa34b019e1 src/java.base/share/classes/java/io/ObjectInputFilter.java
--- a/src/java.base/share/classes/java/io/ObjectInputFilter.javaMon Nov 
06 17:48:00 2017 -0800
+++ b/src/java.base/share/classes/java/io/ObjectInputFilter.javaTue Nov 
07 15:44:36 2017 +0100
@@ -656,8 +656,8 @@
   * otherwise {@code false}
   */
  private static boolean matchesPackage(Class c, String pkg) {
-String n = c.getName();
-return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() 
- 1;
+String n = c.getPackageName();
+return n.length() == pkg.length() - 1 && n.startsWith(pkg);
  }
  


This is correct but we could simplify this.  We can modify the callers 
to drop a trailing "." from the pkg parameter.  I took the liberty to 
revise it a little.


http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8190733/webrev.00/

Roger - can you take a look at the change in ObjectInputFilter?

Mandy

Re: RFR 8190884: java/lang/Runtime/exec/LotsOfOutput fails intermittently

2017-11-07 Thread Lance Andersen

+1

> On Nov 7, 2017, at 5:55 PM, Roger Riggs  wrote:
> 
> Please review a test update to run LotsOfOutput with 'othervm';
> running under the default batch testing mode of agentvm may cause instability.
> 
> --- old/test/jdk/java/lang/Runtime/exec/LotsOfOutput.java 2017-11-07 
> 17:48:38.398907014 -0500
> +++ new/test/jdk/java/lang/Runtime/exec/LotsOfOutput.java 2017-11-07 
> 17:48:37.846921042 -0500
> @@ -23,7 +23,8 @@
> 
>  /**
>   * @test
> - * @bug 4369826 8078582
> + * @bug 4369826 8078582 8190884
> + * @run main/othervm LotsOfOutput
>   * @summary Process with lots of output should not crash VM
>   * @author kladko
>   */
> 
> Thanks, Roger
> 
> 
> 

 
  

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

RFR 8190884: java/lang/Runtime/exec/LotsOfOutput fails intermittently

2017-11-07 Thread Roger Riggs


Please review a test update to run LotsOfOutput with 'othervm';
running under the default batch testing mode of agentvm may cause 
instability.


--- old/test/jdk/java/lang/Runtime/exec/LotsOfOutput.java 2017-11-07 
17:48:38.398907014 -0500
+++ new/test/jdk/java/lang/Runtime/exec/LotsOfOutput.java 2017-11-07 
17:48:37.846921042 -0500

@@ -23,7 +23,8 @@

 /**
  * @test
- * @bug 4369826 8078582
+ * @bug 4369826 8078582 8190884
+ * @run main/othervm LotsOfOutput
  * @summary Process with lots of output should not crash VM
  * @author kladko
  */

Thanks, Roger

Re: [10] RFR 8186046 Minimal ConstantDynamic support

2017-11-07 Thread Paul Sandoz


> On 7 Nov 2017, at 12:04, Karen Kinnear  wrote:
> 
> Paul,
> 
> Thank you for the explanations. Asking for your help in some test case 
> construction at the end here:
> 
>>> 3. java/lang/invoke/package-info.java 128-134
>>> Error handling could be clearer.
>>> My understanding is that if a LinkageError or subclass is thrown, this will 
>>> be rethrown
>>> for all subsequent attempts. Other errors, e.g. VMError may retry resolution
> I was WRONG here - this does match the JVMS. Apologies for the confusion.
>>> 
>>> 9. 
>>> test/jdk/java/lang/invoke/common/test/java/lang/invoke/lib/InstructionHelper.java
>>> How would I write an ldc CONSTANT_Dynamic which referred to a bootstrap 
>>> method that
>>> was not ACC_STATIC?
>> 
>> https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.23
>> 
>> See the note under bootstrap_method_ref. The kind of method handle is 
>> constrained because of the arguments that are pushed on the stack before 
>> invocation. I believe it’s possible to have a constructor, but the declaring 
>> class would need to be a subtype of CallSite in the case of indy, and a 
>> subtype of the constant value type in the case of condy.
>> 
>> 
>>> Or was not ACC_PUBLIC?
>> 
>> That’s dependent on the accessibility between the lookup class the the BSM 
>> declaring class.
>> 
>> 
>>> Or was 
>>> Or did I read the invoke dynamic method incorrectly?
>>> 
>> 
>> By default, and for convenience, the InstructionHelper assumes the BSM is 
>> declared by the lookup class. I recently modified that to support the BSM 
>> being declared on another class, to test the minimal set of BSMs for condy 
>> (separate issue [1]). Note it’s always possible to use the bytecode API more 
>> directly.
>> 
>> So we can easily add more -ve tests for non-accessible or non-static BSMs.

I am wrong, i forgot i updated the API to remove the declaration of the BSM 
kind, and it only supports static BSMs.


> Thank you - that is what we were trying to do - test BSM declared in another 
> class, test in-accessible
> BSM and test non static method.
> 

The latter (access to a BSM in another class) is easy now using the high-level 
construct of InstructionHelper.ldcDynamicConstant (see patch for the condy 
BSMs, which is also in the amber repo in the condy branch).


> Could you help us figure out how to
> 1) invoke a constructor? 
> 2) invoke a virtual method? How do you pass the receiver? 
> 

I think for these you will need to use ASM tools to massage the MH for the BSM 
e.g. develop for a class with a static BSM then adjust the constant pool entry 
for the associated MH.

Paul.

Re: JDK-8067661: transferTo proposal for Appendable

2017-11-07 Thread Brian Burkhalter

I am a little late to this thread, but some alternative verbiage to consider is 
included below. This wording however diverges from that of [1] so if we want 
them to remain similar then it’s probably not worth it to change from what has 
been agreed upon already.

Thanks,

Brian

[1] 
https://docs.oracle.com/javase/9/docs/api/java/io/InputStream.html#transferTo-java.io.OutputStream-

  57  * Reads all characters from this source and writes the characters to
  58  * the given appendable in the order that they are read. On return, the
  59  * source of characters will be at its end.

Reads all characters from this source and appends them to a destination in
the order in which they are read. On return, ...

  60  * 
  61  * This method may block indefinitely reading from the readable, or
  62  * writing to the appendable. Where this source or the appendable is
  63  * {@link AutoCloseable closeable}, then the behavior when either are
  64  * asynchronously closed, or the thread interrupted during the 
transfer,
  65  * is highly readable and appendable specific, and therefore not 
specified.

This method may block indefinitely while reading from the source or writing to 
the destination.
If the source or destination is {@link AutoCloseable closeable}, then the 
behavior when either
is asynchronously closed, or the thread is interrupted during the 
transfer, is highly
implementation-dependent and hence unspecified.

  66  * 
  67  * If an I/O error occurs reading from the readable or writing to the
  68  * appendable, then it may do so after some characters have been read 
or
  69  * written. Consequently the readable may not be at end of its data and
  70  * one, or both participants may be in an inconsistent state. That in 
mind
  71  * all additional measures required by one or both participants in 
order to
  72  * eventually free their internal resources have to be taken by the 
caller
  73  * of this method.

If an I/O error occurs during the operation, then not all characters might have 
been transferred
and the source or destination could be left in an inconsistent state. The 
caller of this method
should therefore ensure in such a case that measures are taken to release any 
resources held
by the source and destination.

On Nov 3, 2017, at 5:49 AM, Alan Bateman  wrote:

> On 02/11/2017 19:55, Patrick Reinhart wrote:
>> :
>> If we are all happy with the API so far, I could start adding an initial 
>> implementation and test…
>> 
> I think the API looks fine so go ahead.

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread Paul Sandoz


> On 7 Nov 2017, at 14:18, fo...@univ-mlv.fr wrote:
>> 
>> Try passing Class.class to it. To be honest this is somewhat motivated by
>> testing when called explicitly.
> 
> see my answer to John.
> 

Fair point, i’ll go with Class. It’s the less smelly option.

Updated along with other changes:

  
http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/ 

 


>> 
>> 
>>> If you have invoke(), you do not need enumConstant because you can cook a
>>> constant method handle Enum.valueOf and send it to invoke.
>> 
>> It’s also possible to support via getStatic as well, as is the case for
>> primitive class.
>> 
>> We went back and forth on the generic and specific axis. For cases where we
>> considered constants are “honorary" members of the constant pool we provide
>> explicit BSMs.
>> 
>> 
>>> The methods that returns VarHandles are trickier because you need a Lookup
>>> object.
>>> 
>>> getstatic should be renamed to getConstant because this is what it does.
>> 
>> No, we want to stick closely with the notion of what the BSM does, there is a
>> close connection also with MethodHandle getStatic, it’s performing a static
>> field access. “getConstant” is too vague, notionally all the BSMs return
>> constants.
>> 
>> 
>>> wrapping the Throwable in a LinkageError seems wrong to me given that a 
>>> calls to
>>> a BSM already does that, so getstatic can just declare "throws Throawble" 
>>> like
>>> invoke and you have the same semantics.
>>> 
>> 
>> We don’t want to declare Throwable for the API in this case. This try/catch 
>> is
>> just ceremony since a getstatic MH in the implementation can only throw a VM
>> error e.g. related to out of memory or stack overflow. I can add some 
>> comments
>> in the code.
> 
> but you do that in invoke().
> 

There is no choice when Invoking a method handle that is passed to us.

In the case of getStatic the use of a MH can be considered an implementation 
detail (we could use reflection) and we know what the j.l.invoke implementation 
does. In this case i believe only VM errors can be produced.

Paul.

[NEW BUG][JDK10] TimeUnit.timedWait(Object obj, long timeout) doc should emphasize possibility of IllegalMonitorStateException

2017-11-07 Thread Christoph Dreis

Hi,

I don't know if this is considered a real bug or just a lack of
documentation, but I found that although the example in the documentation of
TimeUnit.timedWait(Object obj, long timeout) shows the correct usage of the
method, I think at least the documentation should emphasize the possibility
of an IllegalMonitorStateException in case it isn't executed inside a
synchronized block.

The method seems to be not used inside the JDK itself after all, but people
might use it.

Please find a minimal reproduction and a possible documentation patch below.
In case it is considered worthwhile I'd be happy if this is sponsored.

Cheers,
Christoph

= MINIMAL REPRO ==
public class Main {

public static void main(String[] args) {
Lock lock = new ReentrantLock();
try {
TimeUnit.MILLISECONDS.timedWait(lock, 1);
} catch (InterruptedException e) {
e.printStackTrace();
}
}

}

== PATCH ==
diff -r 67aa34b019e1
src/java.base/share/classes/java/util/concurrent/TimeUnit.java
--- a/src/java.base/share/classes/java/util/concurrent/TimeUnit.javaMon
Nov 06 17:48:00 2017 -0800
+++ b/src/java.base/share/classes/java/util/concurrent/TimeUnit.javaTue
Nov 07 23:09:24 2017 +0100
@@ -356,6 +356,8 @@
  * @param timeout the maximum time to wait. If less than
  * or equal to zero, do not wait at all.
  * @throws InterruptedException if interrupted while waiting
+ * @throws IllegalMonitorStateException if not executed inside a
+ * synchronized block
  */
 public void timedWait(Object obj, long timeout)
 throws InterruptedException {

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread forax

> De: "John Rose" 
> À: "Rémi Forax" 
> Cc: "Paul Sandoz" , "hotspot-dev"
> , "core-libs-dev"
> 
> Envoyé: Mardi 7 Novembre 2017 23:20:01
> Objet: Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

> On Nov 7, 2017, at 2:18 PM, [ mailto:fo...@univ-mlv.fr | fo...@univ-mlv.fr ]
> wrote:

 Also, why it's not called in invoke ?

>>> What “it” are you referring to?

>> validateClassAccess.

> Because having a MH means you have already been granted
> access to invoke it. Having a Class does *not* mean you have
> been granted access to its members; that is a further step.

but here you are using the class to change the mh, so you can have access to 
the mh and still have no access to the Class if you do not call the BSM with a 
condy, which is the purpose of validateClassAccess, no ? 

> — John

Rémi

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread John Rose

On Nov 7, 2017, at 2:18 PM, fo...@univ-mlv.fr wrote:
> 
>>> 
>>> Also, why it's not called in invoke ?
>>> 
>> 
>> What “it” are you referring to?
> 
> validateClassAccess.

Because having a MH means you have already been granted
access to invoke it.  Having a Class does *not* mean you have
been granted access to its members; that is a further step.

— John

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread forax



- Mail original -
> De: "Paul Sandoz" 
> À: "Remi Forax" 
> Cc: "core-libs-dev" , "hotspot-dev" 
> 
> Envoyé: Mardi 7 Novembre 2017 21:54:06
> Objet: Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

> Hi Remi,
> 
> You are asking a lot of the same questions we went through a number of times
> before landing where we are :-)
> 
> 
>> On 7 Nov 2017, at 11:33, Remi Forax  wrote:
>> 
>> Hi Paul,
>> 
>> You have an import static requireNonNull but it is only used once in
>> nullConstant, all other methods use Objects.requireNonNull.
>> 
> 
> Fixed.
> 
> 
>> The test that checks that lookup, name and type are null or not is different 
>> in
>> each method,
>> so by example in primitiveClass, if type equals null, you get an IAE.
>> 
> 
> Fixed.
> 
> 
>> I fail to see the point of using validateClassAccess(), if the BSM is used
>> through an indy, the type is created only if the caller class can create it, 
>> so
>> the test seems redundant. If it's not used by an indy, why do we need to test
>> that ?
> 
> We are being deliberately conservative ensuring the lookup is consistent with
> the type in case nefarious byte code spinners punch a hole (not proven a hole
> can be punched, just being conservative).
> 
> 
>> Also, why it's not called in invoke ?
>> 
> 
> What “it” are you referring to?

validateClassAccess.

> 
> 
>> There is also no reason to validate that the parameter type is the right one
>> because the VM will validate the return type of the BSM is asTypable to the
>> type sent as argument.
>> 
> 
> Yes, but we prefer that the BSM reject thereby better signalling the source of
> the error.

the method returns a Class in its signature, so it can not be something else if 
not call by a condy and if the type is wrong in the bytecode, the VM will throw 
a CCE with the same info than your IAE.
i still think it's unnecessary. 

> 
> 
>> Some methods use generics, so other don't. Given it's not useful to use 
>> generics
>> if the methods is used as a BSM, i think the generics signature should be
>> removed, otherwise, getstatic and invoke should use generics.
>> 
> 
> I updated the nullConstant method and removed the type variable.
> 
> In general we try to be consistent with the explicit erasure unless it causes
> some contortion in the implementation as is the case for Enum.
> 
> 
>> In nullConstant, you do not need a requireNonNull here, getting you call
>> isPrimitive() just after.
>> 
> 
> We decided to be explicit rather than implicit for all null checks.
> 
> 
>> In primitiveClass, the second test is equivalent to name.length() != 1, to
>> remove the @SuppressWarnings, type should be declared as a Class. 
>> Why
>> not using getstatic to retrieve the field Type of the wrapper instead ?
>> 
> 
> Try passing Class.class to it. To be honest this is somewhat motivated by
> testing when called explicitly.

see my answer to John.

> 
> 
>> If you have invoke(), you do not need enumConstant because you can cook a
>> constant method handle Enum.valueOf and send it to invoke.
> 
> It’s also possible to support via getStatic as well, as is the case for
> primitive class.
> 
> We went back and forth on the generic and specific axis. For cases where we
> considered constants are “honorary" members of the constant pool we provide
> explicit BSMs.
> 
> 
>> The methods that returns VarHandles are trickier because you need a Lookup
>> object.
>> 
>> getstatic should be renamed to getConstant because this is what it does.
> 
> No, we want to stick closely with the notion of what the BSM does, there is a
> close connection also with MethodHandle getStatic, it’s performing a static
> field access. “getConstant” is too vague, notionally all the BSMs return
> constants.
> 
> 
>> wrapping the Throwable in a LinkageError seems wrong to me given that a 
>> calls to
>> a BSM already does that, so getstatic can just declare "throws Throawble" 
>> like
>> invoke and you have the same semantics.
>> 
> 
> We don’t want to declare Throwable for the API in this case. This try/catch is
> just ceremony since a getstatic MH in the implementation can only throw a VM
> error e.g. related to out of memory or stack overflow. I can add some comments
> in the code.

but you do that in invoke().

> 
> Note that reflective exceptions are mapped to their equivalent errors. 
> Although
> the BSM has been linked it is being asked to perform what can be considered
> further linkage.
> 
> 
>> in arrayVarHandle, the doc said that lookup is not used but lookup is used.
>> 
> 
> Fixed.
> 
> 
>> in validateClassAccess, the return can be moved at the end of the method.
>> 
>> and as a minor issue, all the ifs should be followed by curly braces per the
>> coding convention.
>> 
> 
> I expanded to curly braces.
> 
> Paul.

Rémi

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread Paul Sandoz


> On 7 Nov 2017, at 13:54, Paul Sandoz  wrote:
>> 
>>> If it's not used by an indy, why do we need to test that ? Also, why it's 
>>> not called in invoke ?
>> 
>> …Enum.valueOf doesn't do a security check; that is its choice.
>> This means that if you pass it an enum type that is not public
>> or not in a package exported to you, you can still peek at its
>> enum values.  Meanwhile, when javac emits a reference to
>> an enum, it does so with getstatic.  The getstatic bytecode
>> *does* perform access checks.  The call to validateClassAccess
>> performs those checks, for alignment with the semantics
>> of getstatic.  The internal use of Enum.valueOf is just a detail
>> of the emulation of getstatic in the case of an enum.
>> 
>> (Note to self:  Never use enums to implement a shared
>> secrets pattern.)
>> 
>> For bootstrap methods I prefer to use the most restrictive
>> set of applicable access rules, handshaking with the lookup.
>> 
>> In the case of enums it doesn't matter much, as you say,
>> because Enum.valueOf leaves the door open.
>> 
> 
> Yes, Brian and I noticed that so we punted on the access control.
> 

Hold on… no we didn’t, we included the explicit access control check.

Paul.

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread Paul Sandoz


> On 7 Nov 2017, at 12:59, John Rose  wrote:
> 
> Good comments!  I can handle a couple of them…
> 
> On Nov 7, 2017, at 11:33 AM, Remi Forax  wrote:
>> 
>> I fail to see the point of using validateClassAccess(), if the BSM is used 
>> through an indy, the type is created only if the caller class can create it, 
>> so the test seems redundant.
> 
> That's true, but… 
> 
>> If it's not used by an indy, why do we need to test that ? Also, why it's 
>> not called in invoke ?
> 
> …Enum.valueOf doesn't do a security check; that is its choice.
> This means that if you pass it an enum type that is not public
> or not in a package exported to you, you can still peek at its
> enum values.  Meanwhile, when javac emits a reference to
> an enum, it does so with getstatic.  The getstatic bytecode
> *does* perform access checks.  The call to validateClassAccess
> performs those checks, for alignment with the semantics
> of getstatic.  The internal use of Enum.valueOf is just a detail
> of the emulation of getstatic in the case of an enum.
> 
> (Note to self:  Never use enums to implement a shared
> secrets pattern.)
> 
> For bootstrap methods I prefer to use the most restrictive
> set of applicable access rules, handshaking with the lookup.
> 
> In the case of enums it doesn't matter much, as you say,
> because Enum.valueOf leaves the door open.
> 

Yes, Brian and I noticed that so we punted on the access control.


> I could go either way on this one.
> 

For consistency with our conservative position on other BSMs we should probably 
perform the checks. I’ll update.


>> There is also no reason to validate that the parameter type is the right one 
>> because the VM will validate the return type of the BSM is asTypable to the 
>> type sent as argument.
> 
> For condy, having the BSM validate types is a code smell
> for the reason you mention.

If the BSM should only accept specific types it seems within its remit to 
reject thereby better associating the error with the dynamic constant entry. We 
are dealing here with known fixed types (Class and VarHandle).

 
>  Also, when primitives (and value
> types) are in the mix, people usually code the validation
> incorrectly, since Class.isInstance is the wrong tool, and
> the right tool is non-obvious (MHs.identity.asType).
> 
> Are you commenting on the return type adjustment in invoke?
> That's just a minor optimization to avoid (un)boxing, which
> should have no semantic effect.
> 
>> Some methods use generics, so other don't. Given it's not useful to use 
>> generics if the methods is used as a BSM, i think the generics signature 
>> should be removed, otherwise, getstatic and invoke should use generics.
> 
> The generics are present to make normal calls from source
> code type check.  Use cases:  Combinators, test code.
> 
>> to remove the @SuppressWarnings, type should be declared as a 
>> Class.
> 
> Paul explained this one to me when I made the same objection.  Turns
> out that the class literal "Class.class" (which is required to call that
> BSM from source code) can only have a raw type.  Yuck.  So we
> declare the formal with a matching amount of raw-ness.
> 
>> Why not using getstatic to retrieve the field Type of the wrapper instead ?
> 
> Because the descriptor is a more compact notation for a primitive type.
> Primitive type references will be common and we want them to be small.
> 

Yes, i forgot to mention the size aspect.


>> If you have invoke(), you do not need enumConstant because you can cook a 
>> constant method handle Enum.valueOf and send it to invoke. The methods that 
>> returns VarHandles are trickier because you need a Lookup object. 
> 
> The set is minimal, but not that minimal.  We *could* express *everything*
> using invoke but that would be grotesquely verbose and opaque.
> The "extra" API points are thought to be helpful in reducing class file size,
> and also clarifying the intentions of the affected constants.
> 

Yes.


>> getstatic should be renamed to getConstant because this is what it does.
> 
> (I could go either way; since the JVM has ConstantValue attrs, "Constant"
> is a Thing.  If it's getstatic I prefer getStatic, for alignment with 
> pre-existing
> names in jli.)
> 

+1 on getStatic alignment with jli.

getConstant is too general for this case IMO.

The naming convention we chose was to append Constant if the prefix was a 
keyword such as null or enum.

Paul.


>> wrapping the Throwable in a LinkageError seems wrong to me given that a 
>> calls to a BSM already does that, so getstatic can just declare "throws 
>> Throawble" like invoke and you have the same semantics.
> 
> The point is that we are emulating the semantics of a getstatic instruction,
> which only throws LEs.  We don't want other stuff to leak out of the
> implementation.  Remember that "bytecode behavior" is a normative
> specification, to use when applicable.  It's applicable here.
> 
> Again, I could go

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread forax



- Mail original -
> De: "John Rose" 
> À: "Rémi Forax" 
> Cc: "Paul Sandoz" , "hotspot-dev" 
> , "core-libs-dev"
> 
> Envoyé: Mardi 7 Novembre 2017 21:59:41
> Objet: Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

> Good comments!  I can handle a couple of them…
> 
> On Nov 7, 2017, at 11:33 AM, Remi Forax  wrote:
>> 
>> I fail to see the point of using validateClassAccess(), if the BSM is used
>> through an indy, the type is created only if the caller class can create it, 
>> so
>> the test seems redundant.
> 
> That's true, but…
> 
>> If it's not used by an indy, why do we need to test that ? Also, why it's not
>> called in invoke ?
> 
> …Enum.valueOf doesn't do a security check; that is its choice.
> This means that if you pass it an enum type that is not public
> or not in a package exported to you, you can still peek at its
> enum values.  Meanwhile, when javac emits a reference to
> an enum, it does so with getstatic.  The getstatic bytecode
> *does* perform access checks.  The call to validateClassAccess
> performs those checks, for alignment with the semantics
> of getstatic.  The internal use of Enum.valueOf is just a detail
> of the emulation of getstatic in the case of an enum.
> 
> (Note to self:  Never use enums to implement a shared
> secrets pattern.)
> 
> For bootstrap methods I prefer to use the most restrictive
> set of applicable access rules, handshaking with the lookup.
> 
> In the case of enums it doesn't matter much, as you say,
> because Enum.valueOf leaves the door open.
> 
> I could go either way on this one.

As you said, javac use getstatic (the bytecode), why not reuse getstatic (the 
BSM) in that case, being a field of an enum doesn't seem important and at least 
you are sure that you have the semantics of getstatic (you miss the cache in 
java.lang.Class but i'm not sure it worth it).

> 
>> There is also no reason to validate that the parameter type is the right one
>> because the VM will validate the return type of the BSM is asTypable to the
>> type sent as argument.
> 
> For condy, having the BSM validate types is a code smell
> for the reason you mention.  Also, when primitives (and value
> types) are in the mix, people usually code the validation
> incorrectly, since Class.isInstance is the wrong tool, and
> the right tool is non-obvious (MHs.identity.asType).
> 
> Are you commenting on the return type adjustment in invoke?
> That's just a minor optimization to avoid (un)boxing, which
> should have no semantic effect.

no, the return type adjustment is ok for me, it avoid unboxing + boxing if the 
original handle returns a primitive.

> 
>> Some methods use generics, so other don't. Given it's not useful to use 
>> generics
>> if the methods is used as a BSM, i think the generics signature should be
>> removed, otherwise, getstatic and invoke should use generics.
> 
> The generics are present to make normal calls from source
> code type check.  Use cases:  Combinators, test code.

so getstatic and invoke should have generics in their signature too.

> 
>> to remove the @SuppressWarnings, type should be declared as a 
>> Class.
> 
> Paul explained this one to me when I made the same objection.  Turns
> out that the class literal "Class.class" (which is required to call that
> BSM from source code) can only have a raw type.  Yuck.  So we
> declare the formal with a matching amount of raw-ness.

yes, right X.class and x.getClass() are the two rules in the JLS that can 
introduce raw types.
I think i prefer 'type' to be typed as a Class in that case, it's not a raw 
type and you can assign Class> to it. 

> 
>> Why not using getstatic to retrieve the field Type of the wrapper instead ?
> 
> Because the descriptor is a more compact notation for a primitive type.
> Primitive type references will be common and we want them to be small.

Ok, you have to send the wrapper type if you use getstatic, so you are saving a 
reference to the wrapper type.

> 
>> If you have invoke(), you do not need enumConstant because you can cook a
>> constant method handle Enum.valueOf and send it to invoke. The methods that
>> returns VarHandles are trickier because you need a Lookup object.
> 
> The set is minimal, but not that minimal.  We *could* express *everything*
> using invoke but that would be grotesquely verbose and opaque.
> The "extra" API points are thought to be helpful in reducing class file size,
> and also clarifying the intentions of the affected constants.
> 
>> getstatic should be renamed to getConstant because this is what it does.
> 
> (I could go either way; since the JVM has ConstantValue attrs, "Constant"
> is a Thing.  If it's getstatic I prefer getStatic, for alignment with
> pre-existing
> names in jli.)

perhaps getFinalStatic, because it's restricted to final field.

> 
>> wrapping

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

2017-11-07 Thread Vitaly Davidovich

On Tue, Nov 7, 2017 at 3:45 PM Remi Forax  wrote:

>
>
> On November 7, 2017 8:48:43 PM GMT+01:00, Vitaly Davidovich <
> vita...@gmail.com> wrote:
> >On Tue, Nov 7, 2017 at 1:25 PM Remi Forax  wrote:
> >
> >> My bad,
> >> I've calculated that the header was 8 bytes instead of 12, so I've
> >> supposed that the boolean was not stored in a byte.
> >>
> >> For my culture, why the header is 12 bytes, the pointer to the vtable
> >is
> >> on 64bits and can not be compressed like the other oops ?
> >
> >It’s 12 bytes (on x64) with compressed klass ptr; 8 bytes for the mark
> >word
> >and a 4 byte compressed klass ptr.  It’s 16 bytes without compressed
> >klass
> >ptr.  Or are you asking something else Remi?
>
> My question is more given you have compressed oops  (and compressed klass)
> why the header is 12 bytes and not 8 bytes (a two words header).
>
> so why the  mark word is 8 bytes and not 4 ? At worst it's a pointer and
> again you can use the compressed oops trick ?

http://hg.openjdk.java.net/jdk9/hs/hotspot/file/c68024d52834/src/share/vm/oops/markOop.hpp
has
a description of the layout - it’s not a pointer per se (although some
states do encode one) but a bunch of metadata.

>
>
> >
> >I think many folks would be upset if a boolean wasn’t stored as a byte
> >:).
>
> yes, right.
>
> Remi
>
> >
> >>
> >>
> >> Rémi
> >>
> >>
> >> On November 7, 2017 5:42:33 PM GMT+01:00, Brent Christian <
> >> brent.christ...@oracle.com> wrote:
> >> >Hi, Remi
> >> >
> >> >Thanks for the idea.  From my reading of the jol ouput:
> >> >
> >> >
> >>
> >
> http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt
> >> >
> >> >
> >> >
> >> >I believe retainClassRef is already being stored in a single byte
> >(i.e.
> >> >
> >> >SIZE of 1):
> >> >
> >> >  OFFSET  SIZE TYPE DESCRIPTION
> >> >...
> >> >  10 1  boolean StackFrameInfo.retainClassRef
> >> >
> >> >Thanks,
> >> >-Brent
> >> >
> >> >On 11/6/17 11:45 PM, Remi Forax wrote:
> >> >> Hi Brent,
> >> >> if you declare retainClassRef as a byte, you can even compress the
> >> >data structure a little bit more, no ?
> >> >> a boolean (as a field) uses 4 bytes while a byte uses welll 1
> >byte.
> >> >> Given that bci is a short, the VM will put both bci and
> >> >retainClassRef on the same 32 bits slot.
> >> >>
> >> >> cheers,
> >> >> Rémi
> >> >>
> >> >> - Mail original -
> >> >>> De: "Brent Christian" 
> >> >>> À: "core-libs-dev" ,
> >"hotspot-dev"
> >> >
> >> >>> Envoyé: Mardi 7 Novembre 2017 01:23:16
> >> >>> Objet: RFR: 8185925 & 8153682 (footprint reduction of
> >> >java.lang.StackFrameInfo)
> >> >>
> >> >>> Hi,
> >> >>>
> >> >>> There are a couple opportunities to reduce the memory footprint
> >of
> >> >>> java.lang.StackFrameInfo (the internal implementation of
> >> >>> java.lang.StackWalker.StackFrame):
> >> >>>
> >> >>> 8153682[1] : StackFrameInfo.declaringClass could be removed
> >> >>> 8185925[2] : StackFrameInfo::walker field can be replaced with
> >> >bitmap to
> >> >>> save footprint
> >> >>>
> >> >>> I had a look using jol[3].  Removing only 'walker' helps only
> >under
> >> >32-
> >> >>> and 64-bit, but not with compressed oops.  Removing both 'walker'
> >> >and
> >> >>> 'declaringClass' brings a benefit to compressed oops as well
> >(though
> >> >not
> >> >>> for 16-byte aligned).
> >> >>>
> >> >>> The size change, in bytes, for each execution mode is as follows:
> >> >>>
> >> >>>   32-bit: 32->24
> >> >>>   64-bit: 56->40
> >> >>>   64/compressed oops: 32->24
> >> >>> 64/compressed oops, 16-byte aligned: 32->32
> >> >>>
> >> >>> (For reference, the jol reports for the baseline and specimen are
> >at
> >> >[4]
> >> >>> and [5], respectively.)
> >> >>>
> >> >>> Please review my code change for this.  The webrev is here:
> >> >>> http://cr.openjdk.java.net/~bchristi/8185925/webrev.03/
> >> >>>
> >> >>> An automated test run is in progress.
> >> >>>
> >> >>> Thanks!
> >> >>> -Brent
> >> >>>
> >> >>> --
> >> >>> 1. https://bugs.openjdk.java.net/browse/JDK-8153682
> >> >>> 2. https://bugs.openjdk.java.net/browse/JDK-8185925
> >> >>> 3. http://openjdk.java.net/projects/code-tools/jol/
> >> >>> 4.
> >> >>>
> >> >
> >>
> >
> http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.baseline.txt
> >> >>> 5.
> >> >>>
> >> >
> >>
> >
> http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt
> >>
> >> --
> >> Sent from my Android device with K-9 Mail. Please excuse my brevity.
> >>
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
>
-- 
Sent from my phone

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread John Rose

Good comments!  I can handle a couple of them…

On Nov 7, 2017, at 11:33 AM, Remi Forax  wrote:
> 
> I fail to see the point of using validateClassAccess(), if the BSM is used 
> through an indy, the type is created only if the caller class can create it, 
> so the test seems redundant.

That's true, but… 

> If it's not used by an indy, why do we need to test that ? Also, why it's not 
> called in invoke ?

…Enum.valueOf doesn't do a security check; that is its choice.
This means that if you pass it an enum type that is not public
or not in a package exported to you, you can still peek at its
enum values.  Meanwhile, when javac emits a reference to
an enum, it does so with getstatic.  The getstatic bytecode
*does* perform access checks.  The call to validateClassAccess
performs those checks, for alignment with the semantics
of getstatic.  The internal use of Enum.valueOf is just a detail
of the emulation of getstatic in the case of an enum.

(Note to self:  Never use enums to implement a shared
secrets pattern.)

For bootstrap methods I prefer to use the most restrictive
set of applicable access rules, handshaking with the lookup.

In the case of enums it doesn't matter much, as you say,
because Enum.valueOf leaves the door open.

I could go either way on this one.

> There is also no reason to validate that the parameter type is the right one 
> because the VM will validate the return type of the BSM is asTypable to the 
> type sent as argument.

For condy, having the BSM validate types is a code smell
for the reason you mention.  Also, when primitives (and value
types) are in the mix, people usually code the validation
incorrectly, since Class.isInstance is the wrong tool, and
the right tool is non-obvious (MHs.identity.asType).

Are you commenting on the return type adjustment in invoke?
That's just a minor optimization to avoid (un)boxing, which
should have no semantic effect.

> Some methods use generics, so other don't. Given it's not useful to use 
> generics if the methods is used as a BSM, i think the generics signature 
> should be removed, otherwise, getstatic and invoke should use generics.

The generics are present to make normal calls from source
code type check.  Use cases:  Combinators, test code.

> to remove the @SuppressWarnings, type should be declared as a Class.

Paul explained this one to me when I made the same objection.  Turns
out that the class literal "Class.class" (which is required to call that
BSM from source code) can only have a raw type.  Yuck.  So we
declare the formal with a matching amount of raw-ness.

> Why not using getstatic to retrieve the field Type of the wrapper instead ?

Because the descriptor is a more compact notation for a primitive type.
Primitive type references will be common and we want them to be small.

> If you have invoke(), you do not need enumConstant because you can cook a 
> constant method handle Enum.valueOf and send it to invoke. The methods that 
> returns VarHandles are trickier because you need a Lookup object. 

The set is minimal, but not that minimal.  We *could* express *everything*
using invoke but that would be grotesquely verbose and opaque.
The "extra" API points are thought to be helpful in reducing class file size,
and also clarifying the intentions of the affected constants.

> getstatic should be renamed to getConstant because this is what it does.

(I could go either way; since the JVM has ConstantValue attrs, "Constant"
is a Thing.  If it's getstatic I prefer getStatic, for alignment with 
pre-existing
names in jli.)

> wrapping the Throwable in a LinkageError seems wrong to me given that a calls 
> to a BSM already does that, so getstatic can just declare "throws Throawble" 
> like invoke and you have the same semantics.

The point is that we are emulating the semantics of a getstatic instruction,
which only throws LEs.  We don't want other stuff to leak out of the
implementation.  Remember that "bytecode behavior" is a normative
specification, to use when applicable.  It's applicable here.

Again, I could go either way on this one.

Thanks,
— John

Re: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread Paul Sandoz

Hi Remi,

You are asking a lot of the same questions we went through a number of times 
before landing where we are :-)

> On 7 Nov 2017, at 11:33, Remi Forax  wrote:
> 
> Hi Paul,
> 
> You have an import static requireNonNull but it is only used once in 
> nullConstant, all other methods use Objects.requireNonNull.
> 

Fixed.

> The test that checks that lookup, name and type are null or not is different 
> in each method,
> so by example in primitiveClass, if type equals null, you get an IAE. 
> 

Fixed.

> I fail to see the point of using validateClassAccess(), if the BSM is used 
> through an indy, the type is created only if the caller class can create it, 
> so the test seems redundant. If it's not used by an indy, why do we need to 
> test that ?

We are being deliberately conservative ensuring the lookup is consistent with 
the type in case nefarious byte code spinners punch a hole (not proven a hole 
can be punched, just being conservative).

> Also, why it's not called in invoke ?
> 

What “it” are you referring to?

> There is also no reason to validate that the parameter type is the right one 
> because the VM will validate the return type of the BSM is asTypable to the 
> type sent as argument.
> 

Yes, but we prefer that the BSM reject thereby better signalling the source of 
the error.

> Some methods use generics, so other don't. Given it's not useful to use 
> generics if the methods is used as a BSM, i think the generics signature 
> should be removed, otherwise, getstatic and invoke should use generics.
> 

I updated the nullConstant method and removed the type variable.

In general we try to be consistent with the explicit erasure unless it causes 
some contortion in the implementation as is the case for Enum.

> In nullConstant, you do not need a requireNonNull here, getting you call 
> isPrimitive() just after.
> 

We decided to be explicit rather than implicit for all null checks.

> In primitiveClass, the second test is equivalent to name.length() != 1, to 
> remove the @SuppressWarnings, type should be declared as a Class. 
> Why not using getstatic to retrieve the field Type of the wrapper instead ?
> 

Try passing Class.class to it. To be honest this is somewhat motivated by 
testing when called explicitly.

> If you have invoke(), you do not need enumConstant because you can cook a 
> constant method handle Enum.valueOf and send it to invoke.

It’s also possible to support via getStatic as well, as is the case for 
primitive class. 

We went back and forth on the generic and specific axis. For cases where we 
considered constants are “honorary" members of the constant pool we provide 
explicit BSMs.

> The methods that returns VarHandles are trickier because you need a Lookup 
> object. 
> 
> getstatic should be renamed to getConstant because this is what it does.

No, we want to stick closely with the notion of what the BSM does, there is a 
close connection also with MethodHandle getStatic, it’s performing a static 
field access. “getConstant” is too vague, notionally all the BSMs return 
constants.

> wrapping the Throwable in a LinkageError seems wrong to me given that a calls 
> to a BSM already does that, so getstatic can just declare "throws Throawble" 
> like invoke and you have the same semantics.
> 

We don’t want to declare Throwable for the API in this case. This try/catch is 
just ceremony since a getstatic MH in the implementation can only throw a VM 
error e.g. related to out of memory or stack overflow. I can add some comments 
in the code.

Note that reflective exceptions are mapped to their equivalent errors. Although 
the BSM has been linked it is being asked to perform what can be considered 
further linkage.

> in arrayVarHandle, the doc said that lookup is not used but lookup is used.
> 

Fixed.

> in validateClassAccess, the return can be moved at the end of the method.
> 
> and as a minor issue, all the ifs should be followed by curly braces per the 
> coding convention.
> 

I expanded to curly braces.

Paul.

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

2017-11-07 Thread Remi Forax



On November 7, 2017 8:48:43 PM GMT+01:00, Vitaly Davidovich  
wrote:
>On Tue, Nov 7, 2017 at 1:25 PM Remi Forax  wrote:
>
>> My bad,
>> I've calculated that the header was 8 bytes instead of 12, so I've
>> supposed that the boolean was not stored in a byte.
>>
>> For my culture, why the header is 12 bytes, the pointer to the vtable
>is
>> on 64bits and can not be compressed like the other oops ?
>
>It’s 12 bytes (on x64) with compressed klass ptr; 8 bytes for the mark
>word
>and a 4 byte compressed klass ptr.  It’s 16 bytes without compressed
>klass
>ptr.  Or are you asking something else Remi?

My question is more given you have compressed oops  (and compressed klass) why 
the header is 12 bytes and not 8 bytes (a two words header).

so why the  mark word is 8 bytes and not 4 ? At worst it's a pointer and again 
you can use the compressed oops trick ?

>
>I think many folks would be upset if a boolean wasn’t stored as a byte
>:).

yes, right.

Remi

>
>>
>>
>> Rémi
>>
>>
>> On November 7, 2017 5:42:33 PM GMT+01:00, Brent Christian <
>> brent.christ...@oracle.com> wrote:
>> >Hi, Remi
>> >
>> >Thanks for the idea.  From my reading of the jol ouput:
>> >
>> >
>>
>http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt
>> >
>> >
>> >
>> >I believe retainClassRef is already being stored in a single byte
>(i.e.
>> >
>> >SIZE of 1):
>> >
>> >  OFFSET  SIZE TYPE DESCRIPTION
>> >...
>> >  10 1  boolean StackFrameInfo.retainClassRef
>> >
>> >Thanks,
>> >-Brent
>> >
>> >On 11/6/17 11:45 PM, Remi Forax wrote:
>> >> Hi Brent,
>> >> if you declare retainClassRef as a byte, you can even compress the
>> >data structure a little bit more, no ?
>> >> a boolean (as a field) uses 4 bytes while a byte uses welll 1
>byte.
>> >> Given that bci is a short, the VM will put both bci and
>> >retainClassRef on the same 32 bits slot.
>> >>
>> >> cheers,
>> >> Rémi
>> >>
>> >> - Mail original -
>> >>> De: "Brent Christian" 
>> >>> À: "core-libs-dev" ,
>"hotspot-dev"
>> >
>> >>> Envoyé: Mardi 7 Novembre 2017 01:23:16
>> >>> Objet: RFR: 8185925 & 8153682 (footprint reduction of
>> >java.lang.StackFrameInfo)
>> >>
>> >>> Hi,
>> >>>
>> >>> There are a couple opportunities to reduce the memory footprint
>of
>> >>> java.lang.StackFrameInfo (the internal implementation of
>> >>> java.lang.StackWalker.StackFrame):
>> >>>
>> >>> 8153682[1] : StackFrameInfo.declaringClass could be removed
>> >>> 8185925[2] : StackFrameInfo::walker field can be replaced with
>> >bitmap to
>> >>> save footprint
>> >>>
>> >>> I had a look using jol[3].  Removing only 'walker' helps only
>under
>> >32-
>> >>> and 64-bit, but not with compressed oops.  Removing both 'walker'
>> >and
>> >>> 'declaringClass' brings a benefit to compressed oops as well
>(though
>> >not
>> >>> for 16-byte aligned).
>> >>>
>> >>> The size change, in bytes, for each execution mode is as follows:
>> >>>
>> >>>   32-bit: 32->24
>> >>>   64-bit: 56->40
>> >>>   64/compressed oops: 32->24
>> >>> 64/compressed oops, 16-byte aligned: 32->32
>> >>>
>> >>> (For reference, the jol reports for the baseline and specimen are
>at
>> >[4]
>> >>> and [5], respectively.)
>> >>>
>> >>> Please review my code change for this.  The webrev is here:
>> >>> http://cr.openjdk.java.net/~bchristi/8185925/webrev.03/
>> >>>
>> >>> An automated test run is in progress.
>> >>>
>> >>> Thanks!
>> >>> -Brent
>> >>>
>> >>> --
>> >>> 1. https://bugs.openjdk.java.net/browse/JDK-8153682
>> >>> 2. https://bugs.openjdk.java.net/browse/JDK-8185925
>> >>> 3. http://openjdk.java.net/projects/code-tools/jol/
>> >>> 4.
>> >>>
>> >
>>
>http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.baseline.txt
>> >>> 5.
>> >>>
>> >
>>
>http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt
>>
>> --
>> Sent from my Android device with K-9 Mail. Please excuse my brevity.
>>

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

2017-11-07 Thread mandy chung




On 11/7/17 12:15 PM, Brent Christian wrote:

Updated webrev here:
http://cr.openjdk.java.net/~bchristi/8185925/webrev.04/


Nit: FLAGS should be lower-case (not a constant)

Otherwise looks good.   No need for a new webrev.

Mandy

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

2017-11-07 Thread Brent Christian


On 11/07/2017 09:45 AM, mandy chung wrote:


StackFrameInfo.java

  38 // Footprint improvement: MemberName::clazz can replace
  39 // StackFrameInfo::declaringClass.

The above comment can be removed.


Whoops - thanks.


41 private final boolean retainClassRef;

JVMS [1] has a note about Hotspot implementation of boolean array that
is encoded as a byte array.  That explains JOL output that this boolean
field is 8-bit in our implementation.  This field could be changed to a
byte to hold additional flags, if any in the future.  It may be good to
change this to a byte making the field size explicit.


Thanks for the link and explanation.  I agree with making the change now.

Updated webrev here:
http://cr.openjdk.java.net/~bchristi/8185925/webrev.04/

-Brent

Re: RFR (JDK10/JAXP) 8181151: Fix lint warnings in JAXP repo: cast

2017-11-07 Thread huizhe wang

Thanks Lance, Roger!  After this, will do a final check, a few JDK 9 
deprecations, we'll be completely free from warnings!


-Joe

On 11/7/2017 12:04 PM, Roger Riggs wrote:

+1

On 11/7/2017 2:47 PM, Lance Andersen wrote:

Looks OK Joe

On Nov 7, 2017, at 2:12 PM, Joe Wang  wrote:

Hi,

This change fixes about 300 [cast] warnings in the JAXP sources. 
Changes are basically removing the redundant cast, a bit more than 
that only in one case: in XSDUniqueOrKeyTraverser at line 92, 
uniqueOrKey was assigned to itself when it meant to be "idc". The 
change didn't affect the operation since idc is a result of the get 
operation with uniqueOrKey's name.


JAXP tests passed.

JBS: https://bugs.openjdk.java.net/browse/JDK-8181151
webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181151/webrev/

Thanks,
Joe


 

Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

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

Review request: JDK-8157246 MHs.arrayLength, arrayElementGetter/Setter, arrayConstructor need to specify invocation-time behavior

2017-11-07 Thread mandy chung


http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8157246/webrev.00/index.html

This fixes the spec of MethodHandles::arrayLength, arrayConstructor, 
arrayElementGetter/Setter to specify the behavior if the returned method 
handle is invoked with null array or invalid index; same runtime 
exception thrown by the bytecode behavior.  MethodHandle::asSpreader 
spec is also clarified to throw NPE and IAE except when it spreads with 
no argument and the returned method handle accepts a zero-length array 
or null array.


Thanks
Mandy

Re: [10] RFR 8186046 Minimal ConstantDynamic support

2017-11-07 Thread Karen Kinnear

Paul,

Thank you for the explanations. Asking for your help in some test case 
construction at the end here:

>> 3. java/lang/invoke/package-info.java 128-134
>> Error handling could be clearer.
>> My understanding is that if a LinkageError or subclass is thrown, this will 
>> be rethrown
>> for all subsequent attempts. Other errors, e.g. VMError may retry resolution
I was WRONG here - this does match the JVMS. Apologies for the confusion.
>> 
>> 9. 
>> test/jdk/java/lang/invoke/common/test/java/lang/invoke/lib/InstructionHelper.java
>> How would I write an ldc CONSTANT_Dynamic which referred to a bootstrap 
>> method that
>> was not ACC_STATIC?
> 
> https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-4.html#jvms-4.7.23 
> 
> 
> See the note under bootstrap_method_ref. The kind of method handle is 
> constrained because of the arguments that are pushed on the stack before 
> invocation. I believe it’s possible to have a constructor, but the declaring 
> class would need to be a subtype of CallSite in the case of indy, and a 
> subtype of the constant value type in the case of condy.
> 
> 
>> Or was not ACC_PUBLIC?
> 
> That’s dependent on the accessibility between the lookup class the the BSM 
> declaring class.
> 
> 
>> Or was 
>> Or did I read the invoke dynamic method incorrectly?
>> 
> 
> By default, and for convenience, the InstructionHelper assumes the BSM is 
> declared by the lookup class. I recently modified that to support the BSM 
> being declared on another class, to test the minimal set of BSMs for condy 
> (separate issue [1]). Note it’s always possible to use the bytecode API more 
> directly.
> 
> So we can easily add more -ve tests for non-accessible or non-static BSMs.
Thank you - that is what we were trying to do - test BSM declared in another 
class, test in-accessible
BSM and test non static method.

Could you help us figure out how to
1) invoke a constructor? 
2) invoke a virtual method? How do you pass the receiver? 

thanks,
Karen

> 
> Paul.
> 
> [1] 
> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/
>  
>

Re: RFR (JDK10/JAXP) 8181151: Fix lint warnings in JAXP repo: cast

2017-11-07 Thread Roger Riggs


+1

On 11/7/2017 2:47 PM, Lance Andersen wrote:

Looks OK Joe

On Nov 7, 2017, at 2:12 PM, Joe Wang  wrote:

Hi,

This change fixes about 300 [cast] warnings in the JAXP sources. Changes are basically 
removing the redundant cast, a bit more than that only in one case: in 
XSDUniqueOrKeyTraverser at line 92, uniqueOrKey was assigned to itself when it meant to 
be "idc". The change didn't affect the operation since idc is a result of the 
get operation with uniqueOrKey's name.

JAXP tests passed.

JBS: https://bugs.openjdk.java.net/browse/JDK-8181151
webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181151/webrev/

Thanks,
Joe

  
   

  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: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

2017-11-07 Thread Vitaly Davidovich

On Tue, Nov 7, 2017 at 1:25 PM Remi Forax  wrote:

> My bad,
> I've calculated that the header was 8 bytes instead of 12, so I've
> supposed that the boolean was not stored in a byte.
>
> For my culture, why the header is 12 bytes, the pointer to the vtable is
> on 64bits and can not be compressed like the other oops ?

It’s 12 bytes (on x64) with compressed klass ptr; 8 bytes for the mark word
and a 4 byte compressed klass ptr.  It’s 16 bytes without compressed klass
ptr.  Or are you asking something else Remi?

I think many folks would be upset if a boolean wasn’t stored as a byte :).

>
>
> Rémi
>
>
> On November 7, 2017 5:42:33 PM GMT+01:00, Brent Christian <
> brent.christ...@oracle.com> wrote:
> >Hi, Remi
> >
> >Thanks for the idea.  From my reading of the jol ouput:
> >
> >
> http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt
> >
> >
> >
> >I believe retainClassRef is already being stored in a single byte (i.e.
> >
> >SIZE of 1):
> >
> >  OFFSET  SIZE TYPE DESCRIPTION
> >...
> >  10 1  boolean StackFrameInfo.retainClassRef
> >
> >Thanks,
> >-Brent
> >
> >On 11/6/17 11:45 PM, Remi Forax wrote:
> >> Hi Brent,
> >> if you declare retainClassRef as a byte, you can even compress the
> >data structure a little bit more, no ?
> >> a boolean (as a field) uses 4 bytes while a byte uses welll 1 byte.
> >> Given that bci is a short, the VM will put both bci and
> >retainClassRef on the same 32 bits slot.
> >>
> >> cheers,
> >> Rémi
> >>
> >> - Mail original -
> >>> De: "Brent Christian" 
> >>> À: "core-libs-dev" , "hotspot-dev"
> >
> >>> Envoyé: Mardi 7 Novembre 2017 01:23:16
> >>> Objet: RFR: 8185925 & 8153682 (footprint reduction of
> >java.lang.StackFrameInfo)
> >>
> >>> Hi,
> >>>
> >>> There are a couple opportunities to reduce the memory footprint of
> >>> java.lang.StackFrameInfo (the internal implementation of
> >>> java.lang.StackWalker.StackFrame):
> >>>
> >>> 8153682[1] : StackFrameInfo.declaringClass could be removed
> >>> 8185925[2] : StackFrameInfo::walker field can be replaced with
> >bitmap to
> >>> save footprint
> >>>
> >>> I had a look using jol[3].  Removing only 'walker' helps only under
> >32-
> >>> and 64-bit, but not with compressed oops.  Removing both 'walker'
> >and
> >>> 'declaringClass' brings a benefit to compressed oops as well (though
> >not
> >>> for 16-byte aligned).
> >>>
> >>> The size change, in bytes, for each execution mode is as follows:
> >>>
> >>>   32-bit: 32->24
> >>>   64-bit: 56->40
> >>>   64/compressed oops: 32->24
> >>> 64/compressed oops, 16-byte aligned: 32->32
> >>>
> >>> (For reference, the jol reports for the baseline and specimen are at
> >[4]
> >>> and [5], respectively.)
> >>>
> >>> Please review my code change for this.  The webrev is here:
> >>> http://cr.openjdk.java.net/~bchristi/8185925/webrev.03/
> >>>
> >>> An automated test run is in progress.
> >>>
> >>> Thanks!
> >>> -Brent
> >>>
> >>> --
> >>> 1. https://bugs.openjdk.java.net/browse/JDK-8153682
> >>> 2. https://bugs.openjdk.java.net/browse/JDK-8185925
> >>> 3. http://openjdk.java.net/projects/code-tools/jol/
> >>> 4.
> >>>
> >
> http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.baseline.txt
> >>> 5.
> >>>
> >
> http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
>
-- 
Sent from my phone

Re: RFR (JDK10/JAXP) 8181151: Fix lint warnings in JAXP repo: cast

2017-11-07 Thread Lance Andersen

Looks OK Joe
> On Nov 7, 2017, at 2:12 PM, Joe Wang  wrote:
> 
> Hi,
> 
> This change fixes about 300 [cast] warnings in the JAXP sources. Changes are 
> basically removing the redundant cast, a bit more than that only in one case: 
> in XSDUniqueOrKeyTraverser at line 92, uniqueOrKey was assigned to itself 
> when it meant to be "idc". The change didn't affect the operation since idc 
> is a result of the get operation with uniqueOrKey's name.
> 
> JAXP tests passed.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8181151
> webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181151/webrev/
> 
> Thanks,
> Joe

 
  

 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 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread Remi Forax

Hi Paul,

You have an import static requireNonNull but it is only used once in 
nullConstant, all other methods use Objects.requireNonNull.

The test that checks that lookup, name and type are null or not is different in 
each method,
so by example in primitiveClass, if type equals null, you get an IAE. 

I fail to see the point of using validateClassAccess(), if the BSM is used 
through an indy, the type is created only if the caller class can create it, so 
the test seems redundant. If it's not used by an indy, why do we need to test 
that ? Also, why it's not called in invoke ?

There is also no reason to validate that the parameter type is the right one 
because the VM will validate the return type of the BSM is asTypable to the 
type sent as argument.

Some methods use generics, so other don't. Given it's not useful to use 
generics if the methods is used as a BSM, i think the generics signature should 
be removed, otherwise, getstatic and invoke should use generics.

In nullConstant, you do not need a requireNonNull here, getting you call 
isPrimitive() just after.

In primitiveClass, the second test is equivalent to name.length() != 1, to 
remove the @SuppressWarnings, type should be declared as a Class. Why 
not using getstatic to retrieve the field Type of the wrapper instead ?

If you have invoke(), you do not need enumConstant because you can cook a 
constant method handle Enum.valueOf and send it to invoke. The methods that 
returns VarHandles are trickier because you need a Lookup object. 

getstatic should be renamed to getConstant because this is what it does.
wrapping the Throwable in a LinkageError seems wrong to me given that a calls 
to a BSM already does that, so getstatic can just declare "throws Throawble" 
like invoke and you have the same semantics.

in arrayVarHandle, the doc said that lookup is not used but lookup is used.

in validateClassAccess, the return can be moved at the end of the method.

and as a minor issue, all the ifs should be followed by curly braces per the 
coding convention.

cheers,
Rémi

- Mail original -
> De: "Paul Sandoz" 
> À: "core-libs-dev" , "hotspot-dev" 
> 
> Envoyé: Mardi 7 Novembre 2017 19:38:57
> Objet: RFR 8187742 Minimal set of bootstrap methods for dynamic constants

> Hi,
> 
> Please review the patch that adds a minimal set of bootstrap methods can be be
> used for producing dynamic constants:
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/
>  
> 
> 
> The aim is to provide a small but common set of BSMs that are likely useful 
> with
> ldc or as BSM arguments, filling in the gaps for constants that cannot be
> currently represented, such as null, primitive classes and VarHandles. It’s
> possible to get more sophisticated regarding say factories but that is
> something to consider later on.
> 
> This patch is based off the minimal dynamic constant support (still in 
> review):
> 
>  http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049603.html
>  
> 
> 
> Paul.

RFR (JDK10/JAXP) 8181151: Fix lint warnings in JAXP repo: cast

2017-11-07 Thread Joe Wang


Hi,

This change fixes about 300 [cast] warnings in the JAXP sources. Changes 
are basically removing the redundant cast, a bit more than that only in 
one case: in XSDUniqueOrKeyTraverser at line 92, uniqueOrKey was 
assigned to itself when it meant to be "idc". The change didn't affect 
the operation since idc is a result of the get operation with 
uniqueOrKey's name.


JAXP tests passed.

JBS: https://bugs.openjdk.java.net/browse/JDK-8181151
webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181151/webrev/

Thanks,
Joe

Re: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c

2017-11-07 Thread Martin Buchholz

Looks good to me too.

I've always wondered about how the corresponding java source files are
maintained.  Is it all manual or do y'all have some magic script to help
keep IANA data and java data aligned?  Do we need more testing that
mistakes don't creep in?

On Tue, Nov 7, 2017 at 3:41 AM, Ramanand Patil 
wrote:

> Hi Naoto,
> Thank you for pointing that. I have modified both the affected
> files(ZoneName.java from src and test).
> Here is the updated Webrev: http://cr.openjdk.java.net/~
> rpatil/8190258+8190259/webrev.01/
>
> Regards,
> Ramanand.
>
> > -Original Message-
> > From: Naoto Sato
> > Sent: Tuesday, November 07, 2017 5:18 AM
> > To: Ramanand Patil ; core-libs-dev
>  > d...@openjdk.java.net>; i18n-...@openjdk.java.net
> > Subject: Re:  JDK10 RFR of JDK-8190258: (tz) Support
> tzdata2017c
> > and 8190259: test tck.java.time.zone.TCKZoneRules is broken by
> tzdata2017c
> >
> > Hi Ramanand,
> >
> > In java/time/format/ZoneName.java, should the zid for Africa_Central map
> > to "Africa/Maputo"?
> >
> > Naoto
> >
> > On 11/6/17 6:06 AM, Ramanand Patil wrote:
> > > Hi all,
> > > Please review the latest TZDATA integration (tzdata2017c) to JDK10.
> > > Bugs:
> > > https://bugs.openjdk.java.net/browse/JDK-8190258
> > > https://bugs.openjdk.java.net/browse/JDK-8190259
> > >
> > > Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.00/
> > >
> > > All the TimeZone related tests are passed after integration.
> > >
> > > Regards,
> > > Ramanand.
> > >
>

Re: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c

2017-11-07 Thread Naoto Sato


Looks good.

Naoto

On 11/7/17 3:41 AM, Ramanand Patil wrote:

Hi Naoto,
Thank you for pointing that. I have modified both the affected 
files(ZoneName.java from src and test).
Here is the updated Webrev: 
http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.01/

Regards,
Ramanand.


-Original Message-
From: Naoto Sato
Sent: Tuesday, November 07, 2017 5:18 AM
To: Ramanand Patil ; core-libs-dev ; i18n-...@openjdk.java.net
Subject: Re:  JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c
and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c

Hi Ramanand,

In java/time/format/ZoneName.java, should the zid for Africa_Central map
to "Africa/Maputo"?

Naoto

On 11/6/17 6:06 AM, Ramanand Patil wrote:

Hi all,
Please review the latest TZDATA integration (tzdata2017c) to JDK10.
Bugs:
https://bugs.openjdk.java.net/browse/JDK-8190258
https://bugs.openjdk.java.net/browse/JDK-8190259

Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.00/

All the TimeZone related tests are passed after integration.

Regards,
Ramanand.

RFR 8187742 Minimal set of bootstrap methods for dynamic constants

2017-11-07 Thread Paul Sandoz

Hi,

Please review the patch that adds a minimal set of bootstrap methods can be be 
used for producing dynamic constants:

  
http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8187742-condy-bootstraps/webrev/ 


The aim is to provide a small but common set of BSMs that are likely useful 
with ldc or as BSM arguments, filling in the gaps for constants that cannot be 
currently represented, such as null, primitive classes and VarHandles. It’s 
possible to get more sophisticated regarding say factories but that is 
something to consider later on.

This patch is based off the minimal dynamic constant support (still in review):

  http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049603.html 


Paul.

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

2017-11-07 Thread Remi Forax

My bad,
I've calculated that the header was 8 bytes instead of 12, so I've supposed 
that the boolean was not stored in a byte.

For my culture, why the header is 12 bytes, the pointer to the vtable is on 
64bits and can not be compressed like the other oops ?

Rémi


On November 7, 2017 5:42:33 PM GMT+01:00, Brent Christian 
 wrote:
>Hi, Remi
>
>Thanks for the idea.  From my reading of the jol ouput:
>
>http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt
>
>
>
>I believe retainClassRef is already being stored in a single byte (i.e.
>
>SIZE of 1):
>
>  OFFSET  SIZE TYPE DESCRIPTION
>...
>  10 1  boolean StackFrameInfo.retainClassRef
>
>Thanks,
>-Brent
>
>On 11/6/17 11:45 PM, Remi Forax wrote:
>> Hi Brent,
>> if you declare retainClassRef as a byte, you can even compress the
>data structure a little bit more, no ?
>> a boolean (as a field) uses 4 bytes while a byte uses welll 1 byte.
>> Given that bci is a short, the VM will put both bci and
>retainClassRef on the same 32 bits slot.
>> 
>> cheers,
>> Rémi
>> 
>> - Mail original -
>>> De: "Brent Christian" 
>>> À: "core-libs-dev" , "hotspot-dev"
>
>>> Envoyé: Mardi 7 Novembre 2017 01:23:16
>>> Objet: RFR: 8185925 & 8153682 (footprint reduction of
>java.lang.StackFrameInfo)
>> 
>>> Hi,
>>>
>>> There are a couple opportunities to reduce the memory footprint of
>>> java.lang.StackFrameInfo (the internal implementation of
>>> java.lang.StackWalker.StackFrame):
>>>
>>> 8153682[1] : StackFrameInfo.declaringClass could be removed
>>> 8185925[2] : StackFrameInfo::walker field can be replaced with
>bitmap to
>>> save footprint
>>>
>>> I had a look using jol[3].  Removing only 'walker' helps only under
>32-
>>> and 64-bit, but not with compressed oops.  Removing both 'walker'
>and
>>> 'declaringClass' brings a benefit to compressed oops as well (though
>not
>>> for 16-byte aligned).
>>>
>>> The size change, in bytes, for each execution mode is as follows:
>>>
>>>   32-bit: 32->24
>>>   64-bit: 56->40
>>>   64/compressed oops: 32->24
>>> 64/compressed oops, 16-byte aligned: 32->32
>>>
>>> (For reference, the jol reports for the baseline and specimen are at
>[4]
>>> and [5], respectively.)
>>>
>>> Please review my code change for this.  The webrev is here:
>>> http://cr.openjdk.java.net/~bchristi/8185925/webrev.03/
>>>
>>> An automated test run is in progress.
>>>
>>> Thanks!
>>> -Brent
>>>
>>> --
>>> 1. https://bugs.openjdk.java.net/browse/JDK-8153682
>>> 2. https://bugs.openjdk.java.net/browse/JDK-8185925
>>> 3. http://openjdk.java.net/projects/code-tools/jol/
>>> 4.
>>>
>http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.baseline.txt
>>> 5.
>>>
>http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

2017-11-07 Thread mandy chung




On 11/6/17 4:23 PM, Brent Christian wrote:

Please review my code change for this.  The webrev is here:
http://cr.openjdk.java.net/~bchristi/8185925/webrev.03/



It's a good footprint improvement.  Thanks for doing this.

StackFrameInfo.java

  38 // Footprint improvement: MemberName::clazz can replace
  39 // StackFrameInfo::declaringClass.


The above comment can be removed.

41 private final boolean retainClassRef;

JVMS [1] has a note about Hotspot implementation of boolean array that 
is encoded as a byte array. That explains JOL output that this boolean 
field is 8-bit in our implementation.  This field could be changed to a 
byte to hold additional flags, if any in the future.  It may be good to 
change this to a byte making the field size explicit.


Otherwise looks good.

Mandy
[1] 
https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-2.html#jvms-2.3.4

An automated test run is in progress.

Thanks!
-Brent

--
1. https://bugs.openjdk.java.net/browse/JDK-8153682
2. https://bugs.openjdk.java.net/browse/JDK-8185925
3. http://openjdk.java.net/projects/code-tools/jol/
4. 
http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.baseline.txt
5. 
http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt

Re: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)

2017-11-07 Thread Brent Christian


Hi, Remi

Thanks for the idea.  From my reading of the jol ouput:

http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt 



I believe retainClassRef is already being stored in a single byte (i.e. 
SIZE of 1):


 OFFSET  SIZE TYPE DESCRIPTION
...
 10 1  boolean StackFrameInfo.retainClassRef

Thanks,
-Brent

On 11/6/17 11:45 PM, Remi Forax wrote:

Hi Brent,
if you declare retainClassRef as a byte, you can even compress the data 
structure a little bit more, no ?
a boolean (as a field) uses 4 bytes while a byte uses welll 1 byte.
Given that bci is a short, the VM will put both bci and retainClassRef on the 
same 32 bits slot.

cheers,
Rémi

- Mail original -

De: "Brent Christian" 
À: "core-libs-dev" , "hotspot-dev" 

Envoyé: Mardi 7 Novembre 2017 01:23:16
Objet: RFR: 8185925 & 8153682 (footprint reduction of java.lang.StackFrameInfo)



Hi,

There are a couple opportunities to reduce the memory footprint of
java.lang.StackFrameInfo (the internal implementation of
java.lang.StackWalker.StackFrame):

8153682[1] : StackFrameInfo.declaringClass could be removed
8185925[2] : StackFrameInfo::walker field can be replaced with bitmap to
save footprint

I had a look using jol[3].  Removing only 'walker' helps only under 32-
and 64-bit, but not with compressed oops.  Removing both 'walker' and
'declaringClass' brings a benefit to compressed oops as well (though not
for 16-byte aligned).

The size change, in bytes, for each execution mode is as follows:

  32-bit: 32->24
  64-bit: 56->40
  64/compressed oops: 32->24
64/compressed oops, 16-byte aligned: 32->32

(For reference, the jol reports for the baseline and specimen are at [4]
and [5], respectively.)

Please review my code change for this.  The webrev is here:
http://cr.openjdk.java.net/~bchristi/8185925/webrev.03/

An automated test run is in progress.

Thanks!
-Brent

--
1. https://bugs.openjdk.java.net/browse/JDK-8153682
2. https://bugs.openjdk.java.net/browse/JDK-8185925
3. http://openjdk.java.net/projects/code-tools/jol/
4.
http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.baseline.txt
5.
http://cr.openjdk.java.net/~bchristi/8185925/StackFrameInfo.jol.rmDeclClass.txt

Re: [10] RFR 8186046 Minimal ConstantDynamic support

2017-11-07 Thread Paul Sandoz


> On 5 Nov 2017, at 16:55, David Holmes  wrote:
> 
> On 4/11/2017 7:28 AM, Paul Sandoz wrote:
>>> On 3 Nov 2017, at 11:14, Karen Kinnear  wrote:
>>> 6. SD::find_java_mirror_for_type
>>> You have resolve_or_null/fail doing CHECK_(empty) which should
>>> check for a NULL constant_type_klass. This is followed by an explicit if
>>> (constant_type_klass == NULL) — is that needed?
>>> 
>> Can SD:resolve_or_null return a null value when HAS_PENDING_EXCEPTION=false?
> 
> I don't believe it actually can - the only reason you would get NULL is if 
> something went wrong, for which an exception must be pending. However, even 
> the internal implementation underlying this seems unclear on that point

Right, i am gonna leave things as they are for now unless we come up with a 
more definitive answer.

Paul.

> e.g in SystemDictionary::resolve_instance_class_or_null we have:
> 
>  if (HAS_PENDING_EXCEPTION || k == NULL) {
>return NULL;
>  }
> 
> David
> ——

RE: [Patch][JDK10] Use Class.getPackageName() where possible

2017-11-07 Thread Christoph Dreis

Hi,

> On 11/3/17 6:52 PM, Bernd Eckenfels wrote:
>> The private static helper in ObjectStreamClass became a oneliner and can be 
>> removed and the callsites can transform from getPackageName(c) to 
>> c.getPackageName().
> Good catch.   we should clean that up.
> Christoph - can you send a updated patch to remove 
> ObjectStreamClass::getPackageName and replace the calls with 
> Class::getPackageName and also remove VerifyAccess::getPackageName?

Please find the updated patch below.

Cheers,
Christoph


=== PATCH ===
diff -r 67aa34b019e1 src/java.base/share/classes/java/io/ObjectInputFilter.java
--- a/src/java.base/share/classes/java/io/ObjectInputFilter.javaMon Nov 
06 17:48:00 2017 -0800
+++ b/src/java.base/share/classes/java/io/ObjectInputFilter.javaTue Nov 
07 15:44:36 2017 +0100
@@ -656,8 +656,8 @@
  * otherwise {@code false}
  */
 private static boolean matchesPackage(Class c, String pkg) {
-String n = c.getName();
-return n.startsWith(pkg) && n.lastIndexOf('.') == pkg.length() 
- 1;
+String n = c.getPackageName();
+return n.length() == pkg.length() - 1 && n.startsWith(pkg);
 }
 
 /**
diff -r 67aa34b019e1 src/java.base/share/classes/java/io/ObjectStreamClass.java
--- a/src/java.base/share/classes/java/io/ObjectStreamClass.javaMon Nov 
06 17:48:00 2017 -0800
+++ b/src/java.base/share/classes/java/io/ObjectStreamClass.javaTue Nov 
07 15:44:36 2017 +0100
@@ -1580,18 +1580,7 @@
  */
 private static boolean packageEquals(Class cl1, Class cl2) {
 return (cl1.getClassLoader() == cl2.getClassLoader() &&
-getPackageName(cl1).equals(getPackageName(cl2)));
-}
-
-/**
- * Returns package name of given class.
- */
-private static String getPackageName(Class cl) {
-String s = cl.getName();
-int i = s.lastIndexOf('[');
-i = (i < 0) ? 0 : i + 2;
-int j = s.lastIndexOf('.');
-return (i < j) ? s.substring(i, j) : "";
+cl1.getPackageName().equals(cl2.getPackageName()));
 }
 
 /**
diff -r 67aa34b019e1 src/java.base/share/classes/java/lang/ClassLoader.java
--- a/src/java.base/share/classes/java/lang/ClassLoader.javaMon Nov 06 
17:48:00 2017 -0800
+++ b/src/java.base/share/classes/java/lang/ClassLoader.javaTue Nov 07 
15:44:36 2017 +0100
@@ -675,12 +675,11 @@
 return;
 }
 
-final String name = cls.getName();
-final int i = name.lastIndexOf('.');
-if (i != -1) {
+final String packageName = cls.getPackageName();
+if (!packageName.isEmpty()) {
 AccessController.doPrivileged(new PrivilegedAction<>() {
 public Void run() {
-sm.checkPackageAccess(name.substring(0, i));
+sm.checkPackageAccess(packageName);
 return null;
 }
 }, new AccessControlContext(new ProtectionDomain[] {pd}));
diff -r 67aa34b019e1 src/java.base/share/classes/java/lang/reflect/Proxy.java
--- a/src/java.base/share/classes/java/lang/reflect/Proxy.java  Mon Nov 06 
17:48:00 2017 -0800
+++ b/src/java.base/share/classes/java/lang/reflect/Proxy.java  Tue Nov 07 
15:44:36 2017 +0100
@@ -1034,11 +1034,8 @@
 
 // do permission check if the caller is in a different runtime 
package
 // of the proxy class
-int n = proxyClass.getName().lastIndexOf('.');
-String pkg = (n == -1) ? "" : 
proxyClass.getName().substring(0, n);
-
-n = caller.getName().lastIndexOf('.');
-String callerPkg = (n == -1) ? "" : 
caller.getName().substring(0, n);
+String pkg = proxyClass.getPackageName();
+String callerPkg = caller.getPackageName();
 
 if (pcl != ccl || !pkg.equals(callerPkg)) {
 sm.checkPermission(new 
ReflectPermission("newProxyInPackage." + pkg));
diff -r 67aa34b019e1 
src/java.base/share/classes/sun/invoke/util/VerifyAccess.java
--- a/src/java.base/share/classes/sun/invoke/util/VerifyAccess.java Mon Nov 
06 17:48:00 2017 -0800
+++ b/src/java.base/share/classes/sun/invoke/util/VerifyAccess.java Tue Nov 
07 15:44:36 2017 +0100
@@ -332,16 +332,6 @@
 return Objects.equals(class1.getPackageName(), 
class2.getPackageName());
 }
 
-/** Return the package name for this class.
- */
-public static String getPackageName(Class cls) {
-assert (!cls.isArray());
-String name = cls.getName();
-int dot = name.lastIndexOf('.');
-if (dot < 0) return "";
-return name.substring(0, dot);
-}
-
 /**
  * Test if two classes are defined as part of the same package member 
(top-level class).
  * If this is true, they can share private access with

RE: JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c

2017-11-07 Thread Ramanand Patil

Hi Naoto,
Thank you for pointing that. I have modified both the affected 
files(ZoneName.java from src and test).
Here is the updated Webrev: 
http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.01/

Regards,
Ramanand.

> -Original Message-
> From: Naoto Sato
> Sent: Tuesday, November 07, 2017 5:18 AM
> To: Ramanand Patil ; core-libs-dev  d...@openjdk.java.net>; i18n-...@openjdk.java.net
> Subject: Re:  JDK10 RFR of JDK-8190258: (tz) Support tzdata2017c
> and 8190259: test tck.java.time.zone.TCKZoneRules is broken by tzdata2017c
> 
> Hi Ramanand,
> 
> In java/time/format/ZoneName.java, should the zid for Africa_Central map
> to "Africa/Maputo"?
> 
> Naoto
> 
> On 11/6/17 6:06 AM, Ramanand Patil wrote:
> > Hi all,
> > Please review the latest TZDATA integration (tzdata2017c) to JDK10.
> > Bugs:
> > https://bugs.openjdk.java.net/browse/JDK-8190258
> > https://bugs.openjdk.java.net/browse/JDK-8190259
> >
> > Webrev: http://cr.openjdk.java.net/~rpatil/8190258+8190259/webrev.00/
> >
> > All the TimeZone related tests are passed after integration.
> >
> > Regards,
> > Ramanand.
> >

42 matches

Mail list logo