Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive

2020-08-18 Thread Yumin Qi 

HI, Sundar, David

  Thanks.

  I have update the webrev at the same link: 
http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-02/ according Sundar's 
suggestion.


  Thanks.

  Yumin


On 8/17/20 7:39 PM, sundararajan.athijegannat...@oracle.com wrote:

Hi David.

Thanks.

-Sundar

On 18/08/20 8:04 am, David Holmes wrote:

Hi Sundar,

On 18/08/2020 12:25 pm, sundararajan.athijegannat...@oracle.com wrote:

Not a full review of fresh changes. But couple of comments:

* src/hotspot/share/memory/lambdaFormInvokers.cpp and 
src/hotspot/share/memory/lambdaFormInvokers.hpp miss "Classpath exception" 
clause in the copyright header


Hotspot files do not use the Classpath exception. That is for .java sources that will 
be"linked against" by applications.

Cheers,
David


* I had suggested a change GenerateJLIClassesPlugin.java in last round of 
review. That's still applicable.

https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068160.html

-Sundar

On 18/08/20 1:07 am, Yumin Qi wrote:

Hi, Ioi

  Thanks for review/suggestion. I have updated the webrev at the following link:

http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-02/

  Following changes done in this patch:

  1) move regenerating holder classes into new added file: 
lambdaFormInvokers.[ch]pp

  "@lambda-form-invoker" tag only observed at jvm side.

  2) remove InvokerBytecodeGeneratorHelper.java, move related functions to 
existing class, GenerateJLIClassesHelper.java

  3) add tracing for SPECIES_RESOLVE, so archive more classes.

  4) remove InvokerGenerateBytesException.java, using RuntimeException instead.

  5) Changed make file to exclude the new added lamdaFormInvokers.cpp from the 
non-cds building list.

  6) There is a bug in previous patch, jobject (typeArrayOop) should not be 
used directly for loading class since GC will move the java object. Fixed by 
copying the bytes from handle to C heap.

  7) Tested with tier1,2


Thanks

Yumin



On 8/15/20 6:27 PM, Ioi Lam wrote:

On 8/15/20 6:19 PM, Ioi Lam wrote:

To better capture what we're trying to do in this RFE, I've changed the RFE 
title (and the subject of this email thread) to

https://bugs.openjdk.java.net/browse/JDK-8247536
Support for pre-generated java.lang.invoke classes in CDS static archive

I also update the RFE Description to give an overview of the problem and the 
solution.

The design document is also updated to reflect Yumin's latest implementation



Oops, sent too quickly. The design doc's title is also updated:

https://wiki.openjdk.java.net/display/HotSpot/Support+for+pre-generated+java.lang.invoke+classes+in+CDS+archive


Thanks
- Ioi

On 8/12/20 11:54 AM, calvin.che...@oracle.com wrote:

Hi Yumin,

I reviewed mostly the native code. Below are my comments:

1) classListParser.hpp

71   bool    _lambda_format;

The above name is too generic. How about _lambda_form or _is_lambda_form?
If you rename the above, please also rename the function which returns the 
above bool.

2) jvm.cpp

3850 JVM_ENTRY(void, JVM_CDSTraceResolve(JNIEnv* env, jclass ignore, jstring 
line))

ignore -> ignored

3) jvm.hpp

210 JVM_CDSTraceResolve(JNIEnv* env, jclass ignore, jstring line);

Same comment as for jvm.cpp

4) metaspaceShared.cpp

2017   size_t i = 0;
2018   while (i < size) {
2019 full_name[i++] = *start++;
2020   }

Could the above be simplified to the following?

    strncpy(full_name, start, size - 1);

2029   char* class_name = get_full_class_name(path_name);

Should os::free(class_name) be called before the function returns?

1870 static GrowableArray* lambda_list = NULL;

The name lambda_list is a bit generic, how about lambda_form_list?

2112 lambda_list->append(parser.current_line());

Since parser.current_line() does a strdup, do those buffer need to be freed 
after its use? (i.e. after the call to regenerate_holder_classes()?)

In MetaspaceShared::regenerate_holder_classes, before calling up to java, I think it's 
better to strip the prefix "@lambda-form-invoker" from the strings. So that the 
java InvokerBytecodeGeneratorHelper.readTraceConfig method doesn't need to handle it:

 143 case "[LF_RESOLVE]":
 144 case InvokerBytecodeGenerator.CDS_LOG_PREFIX:

5) DumpSymbolAndStringTable.java

37 private static final String my_string = "DumpSymbolAndStringTable";

Unused variable?

thanks,

Calvin

On 8/11/20 10:36 AM, Yumin Qi wrote:

Forget to send to @core-lib-dev, the patch changed jdk code.


Thanks

Yumin



 Forwarded Message 
Subject: RFR: 8247536: Support pre-generated MethodHandle lambda forms in 
CDS
Date: Tue, 11 Aug 2020 07:44:34 -0700
From: Yumin Qi 
To: hotspot-runtime-...@openjdk.java.net



Hi, Please reivew

  bug: https://bugs.openjdk.java.net/browse/JDK-8247536
  Webrev: http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-01/

  Summary: CDS does not archive pre-generated lambda form classes for method 
handles:

[0.142s][info][class,load]

Re: Fwd: RFR: 8247536: Support pre-generated MethodHandle lambda forms in CDS

2020-08-18 Thread Yumin Qi 

Hi, Calvin

  I have updated the webrev at 
http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-02/


On 8/12/20 11:54 AM, calvin.che...@oracle.com wrote:

Hi Yumin,

I reviewed mostly the native code. Below are my comments:

1) classListParser.hpp

71   bool    _lambda_format;


No longer  used in new patch.

The above name is too generic. How about _lambda_form or _is_lambda_form?
If you rename the above, please also rename the function which returns the 
above bool.

2) jvm.cpp

3850 JVM_ENTRY(void, JVM_CDSTraceResolve(JNIEnv* env, jclass ignore, jstring 
line))

ignore -> ignored

done.


3) jvm.hpp

210 JVM_CDSTraceResolve(JNIEnv* env, jclass ignore, jstring line);

Same comment as for jvm.cpp


done

4) metaspaceShared.cpp

2017   size_t i = 0;
2018   while (i < size) {
2019 full_name[i++] = *start++;
2020   }

Could the above be simplified to the following?

    strncpy(full_name, start, size - 1);


I could use strncpy, but think it is not as efficient as this version since it 
does the same thing, and call c library function:

The call chain for strcpy -> memcpy -> above code.


2029 char* class_name = get_full_class_name(path_name);

Should os::free(class_name) be called before the function returns?


In fact it does not matter, since we exit after dump.

1870 static GrowableArray* lambda_list = NULL;

The name lambda_list is a bit generic, how about lambda_form_list?

2112   lambda_list->append(parser.current_line());

Since parser.current_line() does a strdup, do those buffer need to be freed 
after its use? (i.e. after the call to regenerate_holder_classes()?)

In MetaspaceShared::regenerate_holder_classes, before calling up to java, I think it's 
better to strip the prefix "@lambda-form-invoker" from the strings. So that the 
java InvokerBytecodeGeneratorHelper.readTraceConfig method doesn't need to handle it:

 143 case "[LF_RESOLVE]":
 144 case InvokerBytecodeGenerator.CDS_LOG_PREFIX:


See new version in the new patch. No call for free due to the same reason: 
there about 100+ allocations, not a big allocation, and after dump we exit.



5) DumpSymbolAndStringTable.java

37 private static final String my_string = "DumpSymbolAndStringTable";

Unused variable?


It is used, the string will be in stringTable and output should contain it.


Thanks

Yumin


thanks,

Calvin

On 8/11/20 10:36 AM, Yumin Qi wrote:

Forget to send to @core-lib-dev, the patch changed jdk code.


Thanks

Yumin



 Forwarded Message 
Subject: RFR: 8247536: Support pre-generated MethodHandle lambda forms in 
CDS
Date: Tue, 11 Aug 2020 07:44:34 -0700
From: Yumin Qi 
To: hotspot-runtime-...@openjdk.java.net



Hi, Please reivew

  bug: https://bugs.openjdk.java.net/browse/JDK-8247536
  Webrev: http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-01/

  Summary: CDS does not archive pre-generated lambda form classes for method 
handles:

[0.142s][info][class,load] java.lang.invoke.LambdaForm$MH/0x000800066c40 
source: __JVM_LookupDefineClass__

The method handle resolution if not found in the holder class, a class with the 
method will be generated and loaded as vm internal created class and not 
archived like above. Those class names are mangled as combination of the class 
name and the class instance address.

In this patch, collect those method holder class names and their associated methods' signatures when user specify DumpLoadedClassList, and record them in the log file in a format similar to the output format from traced by -Djava.lang.invoke.MethodHandle.TRACE_RESOLVE=true, but here use another name for CDS: -Djava.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE=true instead. The line prefix also changed from "[LF_RESOLVE]" to "@lambda-invoke-handle".  At dump time, regenerate the holder class with those methods and replace the existing holder class and archived it. At runtime, the resolution of the holder class which contains those methods are loaded from CDS archive so avoid regenerating them again. The patch reorganized the code for Jlink plugin, so the new added InvokerBytecodeGeneratorHelper can be shared both by JLink plugin and CDS code. Also change made to the mangled generated class name at static dump, the class name combining the class name and a sequentially ordered 
number instead of the class instance address, which looks like a random.



To use this feature, one can do the dump/run just like done for a static 
dump/run, so no extra steps required.

  1) java -XX:DumpLoadedClassList=myclasslist JavaApp

  DumpLoadedClassList will turn on 
-Djava.lang.invoke.MethodHandle.CDS_TRACE_RESOLVE=true

  2) java -Xshare:dump -XX:SharedClassListFile=myclasslist 
-XX:SharedArchiveFile=my.jsa JavaApp

  3) java -Xshare:on -XX:SharedArchiveFile=my.jsa JavaApp

  The performance on javac HelloWorld.java (javac-benchmark), no patch vs patch:

   1:   2689285002  2641821474 (-47463528)   391.720

Re: RFR: 8251499: no-placeholder compact number patterns throw IllegalArgumentException

2020-08-18 Thread Joe Wang 

Hi Naoto,

That's nice!  The change looks good to me.

Regards,
Joe

On 8/18/20 11:37 AM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for your comment. I consolidated those duplicated pieces 
into one piece. Did not make it a private method, though, as it would 
need to return two results from the method (cnfMultiplier and whether 
to return immediately for no-placeholder cases).


https://cr.openjdk.java.net/~naoto/8251499/webrev.02/

Naoto

On 8/17/20 11:52 PM, Joe Wang wrote:

Hi Naoto,

Looks good overall. One nit, blocks 1633-1639 and 1642-1649 may share 
a common private method with a parameter that takes either 
matchedPosIndex or matchedNegIndex, if you want.


Best,
Joe

On 8/17/20 4:42 PM, naoto.s...@oracle.com wrote:

Hi Joe,

It turned out that the previous fix did not address plural format 
cases. That means that just making the divisor negative to indicate 
non-placeholder cannot distinguish multiple plural cases with the 
same divisor. Instead, I created a list of placeholders (minimum 
digits) for each index and count. Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8251499/webrev.01/

I added a new test case (COMPACT_PATTERN14), which actually is 
extracted from CLDR 38 Somali locale that demonstrates the issue. 
I'd appreciate your further review.


Naoto

On 8/14/20 6:21 PM, Joe Wang wrote:

Hi Naoto,

Looks good to me.

While a negative divisor representing no zeros is newly introduced, 
the "divisor > 0" checks seem to have always been beneficial.  I 
had to count the number of ""s in COMPACT_PATTERN13 :-)


Have a great weekend!
Joe

On 8/14/2020 3:20 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8251499

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8251499/webrev.00/

The current implementation of CompactNumberFormat assumes that 
there is always the number placeholder part in compact patterns. 
This is not always true. In fact, upcoming CLDR 38 resurrects such 
patterns, so this fix is a precursor to support CLDR 38.


Naoto

Re: RFR: 8251499: no-placeholder compact number patterns throw IllegalArgumentException

2020-08-18 Thread naoto . sato 

Hi Joe,

Thank you for your comment. I consolidated those duplicated pieces into 
one piece. Did not make it a private method, though, as it would need to 
return two results from the method (cnfMultiplier and whether to return 
immediately for no-placeholder cases).


https://cr.openjdk.java.net/~naoto/8251499/webrev.02/

Naoto

On 8/17/20 11:52 PM, Joe Wang wrote:

Hi Naoto,

Looks good overall. One nit, blocks 1633-1639 and 1642-1649 may share a 
common private method with a parameter that takes either matchedPosIndex 
or matchedNegIndex, if you want.


Best,
Joe

On 8/17/20 4:42 PM, naoto.s...@oracle.com wrote:

Hi Joe,

It turned out that the previous fix did not address plural format 
cases. That means that just making the divisor negative to indicate 
non-placeholder cannot distinguish multiple plural cases with the same 
divisor. Instead, I created a list of placeholders (minimum digits) 
for each index and count. Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8251499/webrev.01/

I added a new test case (COMPACT_PATTERN14), which actually is 
extracted from CLDR 38 Somali locale that demonstrates the issue. I'd 
appreciate your further review.


Naoto

On 8/14/20 6:21 PM, Joe Wang wrote:

Hi Naoto,

Looks good to me.

While a negative divisor representing no zeros is newly introduced, 
the "divisor > 0" checks seem to have always been beneficial.  I had 
to count the number of ""s in COMPACT_PATTERN13 :-)


Have a great weekend!
Joe

On 8/14/2020 3:20 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8251499

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8251499/webrev.00/

The current implementation of CompactNumberFormat assumes that there 
is always the number placeholder part in compact patterns. This is 
not always true. In fact, upcoming CLDR 38 resurrects such patterns, 
so this fix is a precursor to support CLDR 38.


Naoto

Re: RFR: 8251499: no-placeholder compact number patterns throw IllegalArgumentException

2020-08-18 Thread naoto . sato 

Hi Roger,

Thank you for your comment. I added a brief comment in the issue on how 
the implementation behaves in the problem case.


Naoto

On 8/18/20 8:19 AM, Roger Riggs wrote:

Hi Naoto,

I think the issue would benefit from a comment describing the solution.
Its not clear how the code addresses the issue.

Thanks, Roger


On 8/17/20 7:42 PM, naoto.s...@oracle.com wrote:

Hi Joe,

It turned out that the previous fix did not address plural format 
cases. That means that just making the divisor negative to indicate 
non-placeholder cannot distinguish multiple plural cases with the same 
divisor. Instead, I created a list of placeholders (minimum digits) 
for each index and count. Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8251499/webrev.01/

I added a new test case (COMPACT_PATTERN14), which actually is 
extracted from CLDR 38 Somali locale that demonstrates the issue. I'd 
appreciate your further review.


Naoto

On 8/14/20 6:21 PM, Joe Wang wrote:

Hi Naoto,

Looks good to me.

While a negative divisor representing no zeros is newly introduced, 
the "divisor > 0" checks seem to have always been beneficial.  I had 
to count the number of ""s in COMPACT_PATTERN13 :-)


Have a great weekend!
Joe

On 8/14/2020 3:20 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8251499

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8251499/webrev.00/

The current implementation of CompactNumberFormat assumes that there 
is always the number placeholder part in compact patterns. This is 
not always true. In fact, upcoming CLDR 38 resurrects such patterns, 
so this fix is a precursor to support CLDR 38.


Naoto

Re: Fix for Javadoc errors in java.base

2020-08-18 Thread Mandy Chung 

Looks fine.

Thanks
Mandy

On 8/18/20 10:02 AM, Julia Boes wrote:


Hi,

The two changes below still need to be reviewed. Any takers?

Cheers,

Julia

--- 
old/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java2020-08-14 
23:55:41.953638446 +0530
+++ 
new/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java2020-08-14 
23:55:41.445619497 +0530

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All 
rights reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -208,7 +208,7 @@
      *
      * @return a CallSite, which, when invoked, will return an 
instance of the

      * functional interface
-     * @throws ReflectiveOperationException
+     * @throws LambdaConversionException
      */
     abstract CallSite buildCallSite()
             throws LambdaConversionException;
--- 
old/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java2020-08-14 
23:55:42.977677096 +0530
+++ 
new/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java2020-08-14 
23:55:42.473658062 +0530

@@ -200,7 +200,6 @@
      *
      * @return a CallSite, which, when invoked, will return an 
instance of the

      * functional interface
-     * @throws ReflectiveOperationException
      * @throws LambdaConversionException If properly formed 
functional interface

      * is not found
      */

Re: Fix for Javadoc errors in java.base

2020-08-18 Thread Julia Boes 

Hi,

The two changes below still need to be reviewed. Any takers?

Cheers,

Julia

--- 
old/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java2020-08-14 
23:55:41.953638446 +0530
+++ 
new/src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java2020-08-14 
23:55:41.445619497 +0530

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -208,7 +208,7 @@
      *
      * @return a CallSite, which, when invoked, will return an 
instance of the

      * functional interface
-     * @throws ReflectiveOperationException
+     * @throws LambdaConversionException
      */
     abstract CallSite buildCallSite()
             throws LambdaConversionException;
--- 
old/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java2020-08-14 
23:55:42.977677096 +0530
+++ 
new/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java2020-08-14 
23:55:42.473658062 +0530

@@ -200,7 +200,6 @@
      *
      * @return a CallSite, which, when invoked, will return an 
instance of the

      * functional interface
-     * @throws ReflectiveOperationException
      * @throws LambdaConversionException If properly formed 
functional interface

      * is not found
      */

Re: RFR[8238286]: 'Add new flatMap stream operation that is more amenable to pushing’

2020-08-18 Thread Paul Sandoz 
Hi Patrick,

This looks good. To resolve the ambiguity of when results are undefined I 
suggest we tweak the docs at the various locations, see below. No need for 
another round of review.

I can understand the desire to place the primitive functional interfaces in 
j.u.functions, but for reasons previously stated they are fine in the current 
location.

Paul.



  * accepts replacing elements. The mapping function operates on the 
consumer,
  * zero or more times, for acceptance of replacing elements.
  *
- * The results of this method are undefined if the {@link Consumer}
- * argument is called outside the scope of the mapper function.
- *
  * This is an intermediate
  * operation.
+ * The results of this intermediate operation are undefined if the
+ * {@code consumer} argument is operated on outside the scope of
+ * its application to the mapping function.
  *
  * @implSpec
  * The default implementation accumulates accepted elements into an 
internal

> On Aug 11, 2020, at 11:11 AM, Patrick Concannon 
>  wrote:
> 
> Hi,
> 
> Please find the next iteration of mapMulti in the new webrev below. 
> 
> In this iteration the following changes have been made:
> The API note for mapMulti has been updated.
> Sub-interfaces for {Int, Double, Long}Stream have been refactored to be more 
> specific to mapMulti.
> The examples have been changed, and now include a reference to how an 
> explicit type parameter can be used in conjunction with mapMulti.
> 
> The CSR and specdiff have also been updated to reflect these changes.
> 
> webrev: http://cr.openjdk.java.net/~pconcannon/8238286/webrevs/webrev.05/
> specdiff: http://cr.openjdk.java.net/~pconcannon/8238286/specdiff/specout.02/
> CSR: https://bugs.openjdk.java.net/browse/JDK-8248166
> 
> Kind regards,
> Patrick

Re: RFR: 8251499: no-placeholder compact number patterns throw IllegalArgumentException

2020-08-18 Thread Roger Riggs 

Hi Naoto,

I think the issue would benefit from a comment describing the solution.
Its not clear how the code addresses the issue.

Thanks, Roger


On 8/17/20 7:42 PM, naoto.s...@oracle.com wrote:

Hi Joe,

It turned out that the previous fix did not address plural format 
cases. That means that just making the divisor negative to indicate 
non-placeholder cannot distinguish multiple plural cases with the same 
divisor. Instead, I created a list of placeholders (minimum digits) 
for each index and count. Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8251499/webrev.01/

I added a new test case (COMPACT_PATTERN14), which actually is 
extracted from CLDR 38 Somali locale that demonstrates the issue. I'd 
appreciate your further review.


Naoto

On 8/14/20 6:21 PM, Joe Wang wrote:

Hi Naoto,

Looks good to me.

While a negative divisor representing no zeros is newly introduced, 
the "divisor > 0" checks seem to have always been beneficial.  I had 
to count the number of ""s in COMPACT_PATTERN13 :-)


Have a great weekend!
Joe

On 8/14/2020 3:20 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8251499

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8251499/webrev.00/

The current implementation of CompactNumberFormat assumes that there 
is always the number placeholder part in compact patterns. This is 
not always true. In fact, upcoming CLDR 38 resurrects such patterns, 
so this fix is a precursor to support CLDR 38.


Naoto

Re: Possible subtle memory model error in ClassValue

2020-08-18 Thread Andrew Haley 
On 17/08/2020 15:24, Peter Levart wrote:
>
> On 8/16/20 7:35 PM, Andrew Haley wrote:
>> On 15/08/2020 10:13, Peter Levart wrote:
>>> https://github.com/openjdk/jdk/pull/9
>>>
>>>
>>> Sorry for abusing GitHub pull request mechanism but I don't have
>>> bandwidth currently to clone the mercurial repository ;-)
>> That's a lot of work to avoid a simple fence.
>>
> Two fences, mind you (the read fence is no-op only on Intel). So take
> half of that work for each fence ;-) Still a lot?

OK, but in HotSpot (and GraalVM AFAICR, but I'd need to check) finals
only require a StoreStore fence and the address dependency does the
rest, so if we can get away with using a final field then we can also
get away with just using a StoreStore fence at the end of the
constructor. I understand that this (as Hans pointed out) isn't strict
Java but it depends on how much we care about strict Java inside the
core libraries.

> No, really it was not much work to make the patch. The real work is yet
> to come - checking that it is correct.

Indeed.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: JDK-8249691: jdk/lambda/vm/StrictfpDefault.java file can be removed

2020-08-18 Thread Seán Coffey 

Looks fine to me Evan.

regards,
Sean.

On 17/08/2020 16:25, Evan Whelan wrote:

Hi all,

  


This is a small fix that helps with some test cleanup. One redundant test file 
has been removed.

  


Webrev found at: http://cr.openjdk.java.net/~kravikumar/8249691/webrev/

  


Link to JBS issue: https://bugs.openjdk.java.net/browse/JDK-8249691

  


Any follow up questions are welcomed.

  


Thanks,

Evan

Re: RFR: 8251499: no-placeholder compact number patterns throw IllegalArgumentException

2020-08-18 Thread Joe Wang 

Hi Naoto,

Looks good overall. One nit, blocks 1633-1639 and 1642-1649 may share a 
common private method with a parameter that takes either matchedPosIndex 
or matchedNegIndex, if you want.


Best,
Joe

On 8/17/20 4:42 PM, naoto.s...@oracle.com wrote:

Hi Joe,

It turned out that the previous fix did not address plural format 
cases. That means that just making the divisor negative to indicate 
non-placeholder cannot distinguish multiple plural cases with the same 
divisor. Instead, I created a list of placeholders (minimum digits) 
for each index and count. Here is the updated webrev:


http://cr.openjdk.java.net/~naoto/8251499/webrev.01/

I added a new test case (COMPACT_PATTERN14), which actually is 
extracted from CLDR 38 Somali locale that demonstrates the issue. I'd 
appreciate your further review.


Naoto

On 8/14/20 6:21 PM, Joe Wang wrote:

Hi Naoto,

Looks good to me.

While a negative divisor representing no zeros is newly introduced, 
the "divisor > 0" checks seem to have always been beneficial.  I had 
to count the number of ""s in COMPACT_PATTERN13 :-)


Have a great weekend!
Joe

On 8/14/2020 3:20 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix for the following issue:

https://bugs.openjdk.java.net/browse/JDK-8251499

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8251499/webrev.00/

The current implementation of CompactNumberFormat assumes that there 
is always the number placeholder part in compact patterns. This is 
not always true. In fact, upcoming CLDR 38 resurrects such patterns, 
so this fix is a precursor to support CLDR 38.


Naoto