Re: [11] RFR JDK-8198653: ClassLoader::getSystemClassLoader throws InternalError when called after shutdown

[mandy chung]

Yes, 
test/hotspot/jtreg/compiler/jvmci/events/JvmciShutdownEventTest.java. 
This test passes with the change.


Mandy

On 2/23/18 10:18 PM, David Holmes wrote:

Looks good.

Is there an existing test that caught this?

Thanks,
David

On 24/02/2018 7:57 AM, mandy chung wrote:
JDK-8198249 added a new shutdown VM initLevel.  
ClassLoader::getSystemClassLoader
should be updated to handle the new case.  I checked all other 
callers of

VM::initLevel and no other place needs update.

Thanks
Mandy

diff --git a/src/java.base/share/classes/java/lang/ClassLoader.java 
b/src/java.base/share/classes/java/lang/ClassLoader.java

--- a/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -1922,7 +1922,7 @@
  case 3:
  String msg = "getSystemClassLoader cannot be called 
during the system class loader instantiation";

  throw new IllegalStateException(msg);
-    case 4:
+    default:
  // system fully initialized
  assert VM.isBooted() && scl != null;
  SecurityManager sm = System.getSecurityManager();
@@ -1930,8 +1930,6 @@
  checkClassLoaderPermission(scl, 
Reflection.getCallerClass());

  }
  return scl;
-    default:
-    throw new InternalError("should not reach here");
  }
  }

Re: [11] RFR JDK-8198653: ClassLoader::getSystemClassLoader throws InternalError when called after shutdown

[David Holmes]

Looks good.

Is there an existing test that caught this?

Thanks,
David

On 24/02/2018 7:57 AM, mandy chung wrote:
JDK-8198249 added a new shutdown VM initLevel.  
ClassLoader::getSystemClassLoader

should be updated to handle the new case.  I checked all other callers of
VM::initLevel and no other place needs update.

Thanks
Mandy

diff --git a/src/java.base/share/classes/java/lang/ClassLoader.java 
b/src/java.base/share/classes/java/lang/ClassLoader.java

--- a/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -1922,7 +1922,7 @@
  case 3:
  String msg = "getSystemClassLoader cannot be called 
during the system class loader instantiation";

  throw new IllegalStateException(msg);
-    case 4:
+    default:
  // system fully initialized
  assert VM.isBooted() && scl != null;
  SecurityManager sm = System.getSecurityManager();
@@ -1930,8 +1930,6 @@
  checkClassLoaderPermission(scl, 
Reflection.getCallerClass());

  }
  return scl;
-    default:
-    throw new InternalError("should not reach here");
  }
  }

RFR JDK-8187653: Lock in CoderResult.Cache becomes performance bottleneck

[Xueming Shen]

Hi,

Please help review the proposed change to remove the potential performance
bottleneck in CoderResult caused by the "over" synchronization the cache
mechanism.

issue: https://bugs.openjdk.java.net/browse/JDK-8187653
webrev: http://cr.openjdk.java.net/~sherman/8187653/webrev

Notes:
While the original test case (new String/String.getBytes() with UTF8 as 
showed in the
stacktrace) described in the bug report might no longer be reproducible 
in jdk9, as
we have been optimizing lots of String related  char[]/byte[] coding 
path away from
the paths that use CoderResult. But it is still a concern/issue for the 
"general"
CharsetDe/Encoder.de/encode() operation, in which the malformed or 
unmappable

CoderResult object is returned.

As showed in the "[synchronized]" section in bm.scores [1], which is 
from the simple
jmh benchmark test CoderResultBM.java [2], the sores are getting 
significant worse

when the number of concurrent de/encoding threads gets bigger.

It appears the easy fix is to replace the sync mechanism from "method 
synchronized
+ HashMap" to  "ConcurrentHashMap" solves the problem, as showed in the 
same bm
result [1] in [ConcurrentHashMap] section, because most of the accesses 
to the caching
HashMap is read operation. The ConcurrentHahsMap's "almost non-block for 
retrieval

operation" really helps here.

There is another fact that might help us optimize further here. For most 
of our charsets
in JDK repository (with couple exceptions), the length of malformed and 
unmappable
CoderResult that these charsets might trigger actually is never longer 
than 4. So we might
not need to access the ConcurrentHashMap cache at all in normal use 
scenario. I'm putting
a CoderResult[4] on top the hashmap cache in proposed webrev. It does 
not improve the
performance significantly, but when the thread number gets bigger, a 
10%+ improvement
is observed. So I would assume it might be something worth doing, given 
its cost is really

low.

Thanks,
Sherman


[1] http://cr.openjdk.java.net/~sherman/8187653/bm.scores
[2] http://cr.openjdk.java.net/~sherman/8187653/CoderResultBM.java
[3] http://cr.openjdk.java.net/~sherman/8187653/bm.scores.ms932

Re: RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present

[Joseph D. Darcy]

Hello,

A few initial comments, not my final review.

Objects implementing the AnnotatedElement interface are also created in 
javac during annotation processing. As a secondary concern, it would be 
good to be have behavior between both javac and runtime annotations 
consistent when possible. (My own to-do list includes at least once such 
alignment JDK-8164819: "Make javac's toString() on annotation objects 
consistent with core reflection".)


Even in javac we've moved away from test and directory names based on 
bugid. I'd recommend incorporating these regression tests into the 
existing tests in


test/jdk/java/lang/annotation/Missing

or creating a subdirectory for conditions, etc..

It would be worth verifying whether or not this change also covers 
java.lang.reflect.Executable.getParameterAnnotations(), more 
specifically the implementation of that method in Method and 
Constructor. The method getParameterAnnotations() is much less used than 
getAnnotations and the other methods on the AnnotatedElement interface, 
but still part of the annotations feature.


(As a follow-up refactoring, it might be worthwhile to replace the 
interior of the three parseFooArray methods to a shared method that is 
passed a lambda for the "Object value = parseFooValue(/*args to get 
foo*/...);" logic.)


Thanks,

-Joe

On 2/23/2018 10:06 AM, joe darcy wrote:

On 2/23/2018 9:39 AM, Alan Bateman wrote:

On 22/02/2018 23:20, Liam Miller-Cushon wrote:

Hello,


Please consider this fix for JDK-7183985.


bug: https://bugs.openjdk.java.net/browse/JDK-7183985

webrev: http://cr.openjdk.java.net/~cushon/7183985/webrev.01/


I started a CSR for the change:
https://bugs.openjdk.java.net/browse/JDK-8198584

We have been using the fix at Google for about two years, and there has
been no compatibility impact. I found very few places 
ArrayStoreException
was being explicitly handled, and none that were depending on the 
current

behaviour of getAnnotation().


There was some previous discussion of the bug on core-libs-dev:

Yes, this one comes up every few years. I'm hoping Joe Darcy will 
reply to your review with any background or issues from when this 
came up in the past. From a distance then retrofitting 
AnnotatedElement getXXX methods to throw TypeNotPresentException 
seems reasonable, I'm less sure about the isAnnotationPresent method 
as it might be surprising for that to fail.




On my list!

Cheers,

-Joe

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

[+Paul]

On Fri, Feb 23, 2018 at 6:28 AM, Alan Bateman 
wrote:

>
> 8198484: URLClassPath should use an ArrayDeque instead of a Stack
> http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/
> https://bugs.openjdk.java.net/browse/JDK-8198484
>
> Can copyToArrayDeque use addAll?
>

Not directly, because addAll uses a lambda, and it's too early in the
bootstrap for lambdas.

We could delambdafy ArrayDeque, plausibly because ArrayDeque is a
super-core class, perhaps reengineering ArrayDeque(Collection) and/or
addAll(Collection).

Or perhaps lambda team has a plan to allow lambdas to be used in corest of
core java sometime soon?

[11] RFR JDK-8198653: ClassLoader::getSystemClassLoader throws InternalError when called after shutdown

[mandy chung]

JDK-8198249 added a new shutdown VM initLevel.  
ClassLoader::getSystemClassLoader
should be updated to handle the new case.  I checked all other callers of
VM::initLevel and no other place needs update.

Thanks
Mandy

diff --git a/src/java.base/share/classes/java/lang/ClassLoader.java 
b/src/java.base/share/classes/java/lang/ClassLoader.java
--- a/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -1922,7 +1922,7 @@
 case 3:
 String msg = "getSystemClassLoader cannot be called during the 
system class loader instantiation";
 throw new IllegalStateException(msg);
-case 4:
+default:
 // system fully initialized
 assert VM.isBooted() && scl != null;
 SecurityManager sm = System.getSecurityManager();
@@ -1930,8 +1930,6 @@
 checkClassLoaderPermission(scl, 
Reflection.getCallerClass());
 }
 return scl;
-default:
-throw new InternalError("should not reach here");
 }
 }

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Fri, Feb 23, 2018 at 6:28 AM, Alan Bateman 
wrote:

> Just getting to the updated webrev now.
>
> On 21/02/2018 20:30, Martin Buchholz wrote:
>
> :
>
>
> 8198480: Improve ClassLoaders static init block
> http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/
> https://bugs.openjdk.java.net/browse/JDK-8198480
>
> The value of jdk.module.main is the name of the "initial module" so better
> to use that instead term of "main module".
>
>
Done, but I see elsewhere the same convention, that jigsaw team might want
to make consistent:

---
String mainModule;

String mainModule = System.getProperty("jdk.module.main");

 * {@code jdk.module.main}
 * The module name of the initial/main module
 * {@code jdk.module.main.class}
 * The main class name of the initial module

SetMainModule(const char *s)
---

Re: RFR: 8197901: Crash during GC when logging level is debug

[Leonid Mesnik]

Thank you for review. I will add @bug info in the test.

Leonid

> On Feb 22, 2018, at 8:26 PM, David Holmes  wrote:
> 
> Hi Leonid,
> 
> Looks fine. Please also add this bug id to @bug in
> 
> test/jdk/java/lang/StackWalker/VerifyStackTrace.java
> 
> Thanks,
> David
> 
> On 23/02/2018 12:41 PM, Leonid Mesnik wrote:
>> Hi
>> Could you please review following fix which update implementation of 
>> Klass::external_name for anonymous classes.
>> Previously external_name tried to add hashcode of corresponding java_mirror 
>> for InstanceKlass if it exists. However the java_mirror could be incorrect 
>> during GC. Also external_name might tries to calculate hash_code if it was 
>> not “pre-calculated” during class verification. See JDK-8197442 
>>  [Graal] 
>> runtime/Metaspace/DefineClass.java crashes with "biases should not be seen 
>> by VM thread here"
>> The suggested fix is to  print address of corresponding InstanceKlass 
>> instead of hashcode. It allows to identify anonymous classes and allows to 
>> use external_name at any time. The hashcode for java_mirror is still 
>> pre-calculated in verifier.cpp since ik->java_mirror()->identity_hash()  
>> still might be used during safepoint. As a regression test I updated one of 
>> tests which redefine classes and easily reproduce problem when executed with 
>> full logging enabled.
>> Test java/lang/StackWalker/VerifyStackTrace.java is update to match new 
>> pattern.
>> webrev: http://cr.openjdk.java.net/~lmesnik/8197901/webrev.00/ 
>> 
>> bug: https://bugs.openjdk.java.net/browse/JDK-8197901 
>> 
>> Leonid

Re: RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Joe Wang]

Looks good to me too.

Would you want to update the Copyright year? For the apache file 
(ToStream.java), it would also be good to update the @LastModified tag.


Thanks,
Joe

On 2/23/2018 12:33 PM, Roger Riggs wrote:

There are two more changes in the java.xml package. (Thanks Joe)
Webrev updated in place.

http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

On 2/23/2018 2:50 PM, Xueming Shen wrote:

+1

On 02/23/2018 11:39 AM, Roger Riggs wrote:
Please review cleanup replacements of 
System.getProperty("line.separator") with System.lineSeparator().
It uses the line separator from System instead of looking it up in 
the properties each time.

Also fixed one javadoc @see reference.

The affected files are in several packages:

   com/sun/crypto/provider/
   java/util/regex/
   jdk/internal/util/xml/impl/

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

Re: RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Lance Andersen]

still looks good

> On Feb 23, 2018, at 3:33 PM, Roger Riggs  wrote:
> 
> There are two more changes in the java.xml package. (Thanks Joe)
> Webrev updated in place.
> 
>   http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/
> 
> Thanks, Roger
> 
> On 2/23/2018 2:50 PM, Xueming Shen wrote:
>> +1
>> 
>> On 02/23/2018 11:39 AM, Roger Riggs wrote:
>>> Please review cleanup replacements of System.getProperty("line.separator") 
>>> with System.lineSeparator().
>>> It uses the line separator from System instead of looking it up in the 
>>> properties each time.
>>> Also fixed one javadoc @see reference.
>>> 
>>> The affected files are in several packages:
>>> 
>>>com/sun/crypto/provider/
>>>java/util/regex/
>>>jdk/internal/util/xml/impl/
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/
>>> 
>>> 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 

Re: RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Roger Riggs]

There are two more changes in the java.xml package. (Thanks Joe)
Webrev updated in place.

  http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

On 2/23/2018 2:50 PM, Xueming Shen wrote:

+1

On 02/23/2018 11:39 AM, Roger Riggs wrote:
Please review cleanup replacements of 
System.getProperty("line.separator") with System.lineSeparator().
It uses the line separator from System instead of looking it up in 
the properties each time.

Also fixed one javadoc @see reference.

The affected files are in several packages:

   com/sun/crypto/provider/
   java/util/regex/
   jdk/internal/util/xml/impl/

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

JDK-6372077: JarFile.getManifest() should handle manifest attribute names up to 70 bytes

[Roger Riggs]

Please review this contribution from Philipp Kunz to handle manifest 
attribute names up to 70 bytes.


The change passes the available regression tests.
Manifest handling is somewhat sensitive so an additional review is 
appreciated.


Webrev: (rebased from the original patch of 2/22/18)
    http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/

Issue:
   https://bugs.openjdk.java.net/browse/JDK-6372077

Thanks, Roger

On 2/2/2018 1:52 PM, Philipp Kunz wrote:

Hi Roger

Glad to send the patch.
I also tried to write a meaningful and useful test. Please tell me 
ruthlessly if it makes sense or what not.
Looking forward to progress in a bug that has been open for more than 
10 years now.


Philipp


On 22.01.2018 21:03, Roger Riggs wrote:

Hi Philipp,

I'm tending to agree with the suggestion about line length 
interpretation.
To meet OpenJDK IP requirements, please attach the .patch file or 
include it in the body

of the message.

Thanks, Roger

On 12/18/2017 11:17 PM, Philipp Kunz wrote:

Hi Roger,

My suggested and also preferred approach is to read the manifest 
specification [1] in a way such that the line breaks are not 
included when counting the maximum line length. The specification 
does not state explicitly whether or not to include line breaks 
within the maximum line length limit but the following sentence from 
the specifications gives a hint:


    Because header names cannot be continued, the maximum length of a
    header name is 70 bytes (there must be a colon and a SPACE after 
the

    name).

Above statement can be true only if line breaks are not counted for 
the maximum line length limit. Assuming so would in my opinion allow 
to understand the complete manifest specification without a conflict 
and effectively result in wider manifest files (maximum each line), 
wider by two bytes of a line break.


In the meantime since the mail you replied to, I created a patch [3] 
mentioned in [2] which might be useful provided the manifest 
specification discussion is resolved.


Regards,
Philipp


[1] 
https://docs.oracle.com/javase/8/docs/technotes/guides/jar/jar.html#Notes_on_Manifest_and_Signature_Files 
/ 
https://docs.oracle.com/javase/9/docs/specs/jar/jar.html#Notes_on_Manifest_and_Signature_Files
[2] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050500.html

[3] http://files.paratix.ch/jdk/6372077/webrev.01/



On 18.12.2017 16:46, Roger Riggs wrote:

Hi Phillip,

Sorry for the silence...

I/we haven't had time to full understand the ramifications of the 
change you propose.
It seems there is a difficult/unresolvable conflict in the 
specifications between the line length

requirements and the header specs.

Regards, Roger

On 11/21/2017 1:18 AM, Philipp Kunz wrote:

Hi everyone,

I haven't got any reply now for around three weeks and now i start 
to wonder if I just missed it or if I should refine my approach to 
find a sponsor or if it helped if I presented a ready patch or if 
noone considers this important enough or anything else whatever. 
This is only my second contribution hence I don't know the 
procedure well.


One point maybe worth to point out again is that I don't want to 
support manifest headers longer than 70 character, just up to 70, 
which has always been the intention but has only worked up to 68. 
This might have been written confusingly in my last email.


As far as I understood, I should first get a sponsor. In any case, 
is there any suggestion for how to proceed?


Regards,
Philipp



On 03.11.2017 00:04, Philipp Kunz wrote:

Hi Sean and Max and all others,

Thank you Sean for the hint about the right mailing list. And 
thanks also for his hint to Max to make smaller portions of changes.


I would like to contribute a fix for JDK-6372077 which is about 
JarFile.getManifest() should handle manifest attribute name[s 
longer than] 70 bytes.


It looks like the bug is caused by Manifest.make72Safe breaking 
lines at 70 bytes instead of 72 despite its comment and name 
(http://hg.openjdk.java.net/jdk10/master/file/tip/src/java.base/share/classes/java/util/jar/Manifest.java#l176).The 
resulting StringBuffer has then lines of 72 bytes maximum each 
including the following line break. Without the line break that 
leaves 70 bytes of characters per line. On the other hand, header 
names can be up to 70 bytes (only single-byte utf-8 characters) 
and cannot be broken across a line break and need to be followed 
by a colon and a space which must be on the same line too 
according to the specification. When breaking at 70 bytes 
excluding the line break, however, long header names don't fit in 
one line together with the colon space delimiter because there is 
not sufficient space.
Manifests with names up to 70 bytes long can still be written 
without immediate exception but the resulting manifests are 
illegal in my opinion. When later reading such manifests 

Re: RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Xueming Shen]

+1

On 02/23/2018 11:39 AM, Roger Riggs wrote:

Please review cleanup replacements of System.getProperty("line.separator") with 
System.lineSeparator().
It uses the line separator from System instead of looking it up in the 
properties each time.
Also fixed one javadoc @see reference.

The affected files are in several packages:

   com/sun/crypto/provider/
   java/util/regex/
   jdk/internal/util/xml/impl/

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

Re: RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Xuelei Fan]

Looks fine to me.  Thanks!

Xuelei

On 2/23/2018 11:39 AM, Roger Riggs wrote:
Please review cleanup replacements of 
System.getProperty("line.separator") with System.lineSeparator().
It uses the line separator from System instead of looking it up in the 
properties each time.

Also fixed one javadoc @see reference.

The affected files are in several packages:

    com/sun/crypto/provider/
    java/util/regex/
    jdk/internal/util/xml/impl/

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

Re: RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Lance Andersen]

Looks good Roger


> On Feb 23, 2018, at 2:39 PM, Roger Riggs  wrote:
> 
> Please review cleanup replacements of System.getProperty("line.separator") 
> with System.lineSeparator().
> It uses the line separator from System instead of looking it up in the 
> properties each time.
> Also fixed one javadoc @see reference.
> 
> The affected files are in several packages:
> 
>   com/sun/crypto/provider/
>   java/util/regex/
>   jdk/internal/util/xml/impl/
> 
> Webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/
> 
> 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 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Roger Riggs]

Please review cleanup replacements of 
System.getProperty("line.separator") with System.lineSeparator().
It uses the line separator from System instead of looking it up in the 
properties each time.

Also fixed one javadoc @see reference.

The affected files are in several packages:

   com/sun/crypto/provider/
   java/util/regex/
   jdk/internal/util/xml/impl/

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

Re: RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present

[joe darcy]

On 2/23/2018 9:39 AM, Alan Bateman wrote:

On 22/02/2018 23:20, Liam Miller-Cushon wrote:

Hello,


Please consider this fix for JDK-7183985.


bug: https://bugs.openjdk.java.net/browse/JDK-7183985

webrev: http://cr.openjdk.java.net/~cushon/7183985/webrev.01/


I started a CSR for the change:
https://bugs.openjdk.java.net/browse/JDK-8198584

We have been using the fix at Google for about two years, and there has
been no compatibility impact. I found very few places 
ArrayStoreException
was being explicitly handled, and none that were depending on the 
current

behaviour of getAnnotation().


There was some previous discussion of the bug on core-libs-dev:

Yes, this one comes up every few years. I'm hoping Joe Darcy will 
reply to your review with any background or issues from when this came 
up in the past. From a distance then retrofitting AnnotatedElement 
getXXX methods to throw TypeNotPresentException seems reasonable, I'm 
less sure about the isAnnotationPresent method as it might be 
surprising for that to fail.




On my list!

Cheers,

-Joe

Re: [11] RFR JDK-8060094: java/util/Formatter/Basic.java failed in tr locale

[naoto . sato]

+1

Naoto

On 2/23/18 12:33 AM, Nishit Jain wrote:

Thanks Naoto,

Please check the updated webrev
http://cr.openjdk.java.net/~nishjain/8060094/webrev.04/

Edits made: In FormatLocale.java, clarified the exception messages about 
the locale used and removed an unused import.


Regards,
Nishit Jain
On 23-02-2018 00:56, Naoto Sato wrote:

Hi Nishit,

In the test case, the exception error messages may be more helpful if 
there is a distinction between the two (line 103 and 120) mentioning 
the formatter is using the default or specified locale.


Naoto

On 2/22/18 3:51 AM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-8060094.

Bug: https://bugs.openjdk.java.net/browse/JDK-8060094
Webrev: http://cr.openjdk.java.net/~nishjain/8060094/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8197916

Cause: The Formatter APIs were not using the correct locale for upper 
casing when specified during instance creation or during format() 
call. The default locale was getting used irrespective of whether a 
Locale is specified in the API.

Fix: Modified the APIs to use the locale specified.

Regards,
Nishit Jain

Re: RFR 8181594: Efficient and constant-time modular arithmetic

[Xuelei Fan]

ArrayUtil.java:
===
I'm not very sure how widely this utilities will be used in the future. 
Looks like only BigIntegerModuloP uses this classes.  I may prefer to 
define private methods for byte array swap in BigIntegerModuloP.



BigIntegerModuloP.java:
===
As this is a class for testing or ptototype purpose,  it might not be a 
part of JDK products, like JRE.  Would you mind move it to a test 
package if you want to keep it?



IntegerModuloP, IntegerModuloP_Base and MutableIntegerModuloP
=
In the security package/context, it may make sense to use 
"IntegerModulo" for the referring to "integers modulo a prime value". 
The class name of "IntegerModuloP_Base" is not a general Java coding 
style.  I may prefer a little bit name changes like:

 IntegerModuloP_Base   -> IntegerModulo
 IntegerModuloP-> ImmutableIntegerModulo
 MutableIntegerModuloP -> MutableIntegerModulo

 IntegerFieldModuloP   -> IntegerModuloField (?)


MutableIntegerModuloP.java
==
void conditionalSwapWith(MutableIntegerModuloP b, int swap);
As the 'swap' parameter can only be 0 or 1, could it be a boolean parameter?

Except the conditionalSwapWith() method, I did not get the points why we 
need a mutable version.  Would you please have more description of this 
requirement?



IntegerModuloP_Base.java

default byte[] addModPowerTwo(IntegerModuloP_Base b, int len)
void addModPowerTwo(IntegerModuloP_Base b, byte[] result);

For the first sign of the method names, I thought it is to calculate as 
"(this + b) ^ 2 mod m".  Besides, what's the benefits of the two 
methods?  Could we just use:

  this.add(b).asByteArray()

I guess, but not very sure, it is for constant time calculation.  If the 
function is required, could it be renamed as:


  // the result is inside of the size range
  IntegerModuloP addModSize(IntegerModuloP_Base b, int size)
Or
  // the result is wrapped if outside of the size range
  IntegerModuloP addOnWrap(IntegerModuloP_Base b, int size)

and the use may look like:
  this.addModSize(b, size).asByteArray()


Will review the rest when I understand more about the interfaces design.

Thanks,
Xuelei

On 1/30/2018 8:52 AM, Adam Petcher wrote:

+core-libs-dev


On 1/26/2018 4:06 PM, Adam Petcher wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8181594
Webrev: http://cr.openjdk.java.net/~apetcher/8181594/webrev.00/

This is a code review for the field arithmetic that will be used in 
implementations of X25519/X448 key agreement, the Poly1305 
authenticator, and EdDSA signatures. I believe that the library has 
all the features necessary for X25519/X448 and Poly1305, and I expect 
at most a couple of minor enhancements will be required to support 
EdDSA. There is no public API for this library, so we can change it in 
the future to suit the needs of new algorithms without breaking 
compatibility with external code. Still, I made an attempt to clearly 
structure and document the (internal) API, and I want to make sure it 
is understandable and easy to use.


This is not a general-purpose modular arithmetic library. It will only 
work well in circumstances where the sequence of operations is 
restricted, and where the prime that defines the field has some useful 
structure. Moreover, each new field will require some field-specific 
code that takes into account the structure of the prime and the way 
the field is used in the application. The initial implementation 
includes a field for Poly1305 and the fields for X25519/X448 which 
should also work for EdDSA.


The benefits of using this library are that it is much more efficient 
than using similar operations in BigInteger. Also, many operations are 
branch-free, making them suitable for use in a side-channel resistant 
implementation that does not branch on secrets.


To provide some context, I have attached a code snippet describing how 
this library can be used. The snippet is the constant-time Montgomery 
ladder from my X25519/X448 implementation, which I expect to be out 
for review soon. X25519/X448 only uses standard arithmetic operations, 
and the more unusual features (e.g. add modulo a power of 2) are 
needed by Poly1305.


The field arithmetic (for all fields) is implemented using a 32-bit 
representation similar to the one described in the Ed448 paper[1] (in 
the "Implementation on 32-bit platforms" section). Though my 
implementation uses signed limbs, and grade-school multiplication 
instead of Karatsuba. The argument for correctness is essentially the 
same for all three fields: the magnitude of each 64-bit limb is at 
most 2^(k-1) after reduction, except for the last limb which may have 
a magnitude of up to 2^k. The values of k are between 26 to 28 
(depending on the field), and we can calculate that the maximum 
magnitude for any limb during an add-multiply-carry-reduce 

Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Alan Bateman]

On 23/02/2018 15:28, Peter Levart wrote:

Hi Adam,

Did you know that native memory is already tracked on the Java side for
direct ByteBuffers? See class java.nio.Bits. Could you make use of it?

Right, these are the fields that are exposed at runtime via 
BufferPoolMXBean. A SA based tool could read from a core file. I can't 
tell if this is enough for Adam, it may be that the his tool reveals 
more details on the buffers in the pools.


-Alan

Re: RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present

[Alan Bateman]

On 22/02/2018 23:20, Liam Miller-Cushon wrote:

Hello,


Please consider this fix for JDK-7183985.


bug: https://bugs.openjdk.java.net/browse/JDK-7183985

webrev: http://cr.openjdk.java.net/~cushon/7183985/webrev.01/


I started a CSR for the change:
https://bugs.openjdk.java.net/browse/JDK-8198584

We have been using the fix at Google for about two years, and there has
been no compatibility impact. I found very few places ArrayStoreException
was being explicitly handled, and none that were depending on the current
behaviour of getAnnotation().


There was some previous discussion of the bug on core-libs-dev:

Yes, this one comes up every few years. I'm hoping Joe Darcy will reply 
to your review with any background or issues from when this came up in 
the past. From a distance then retrofitting AnnotatedElement getXXX 
methods to throw TypeNotPresentException seems reasonable, I'm less sure 
about the isAnnotationPresent method as it might be surprising for that 
to fail.


-Alan

Re: [PATCH] Optimization of AbstractStringBuilder.ensureCapacityInternal()

[Roman Leventov]

Hi Claes, indeed, seems that this change breaks the zeroing optimization,
so ensureCapacityInternal() becomes slower when the char count is
comparable with the array length. Thanks.

On 22 February 2018 at 18:35, Claes Redestad 
wrote:

> Hi,
>
> interesting - do you have any numbers showing a benefit from doing this
> (on various sets of input)?
>
> My concerns are that count might typically be close enough to value.length
> to make the difference small in practice, and that there are some (fragile)
> JIT optimizations to try and avoid zeroing out the newly allocated array
> that we have to take care not to break.
>
> /Claes
>
>
> On 2018-02-22 17:51, Roman Leventov wrote:
>
>> AbstractStringBuilder.ensureCapacityInternal() doesn't need to copy the
>> whole underlying array, only the part until the current count.
>>
>> diff --git
>> a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>> b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>> --- a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>> +++ b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
>> @@ -140,11 +140,12 @@
>>* overflow, this method throws {@code OutOfMemoryError}.
>>*/
>>   private void ensureCapacityInternal(int minimumCapacity) {
>> +byte[] oldValue = value;
>>   // overflow-conscious code
>> -int oldCapacity = value.length >> coder;
>> +int oldCapacity = oldValue.length >> coder;
>>   if (minimumCapacity - oldCapacity > 0) {
>> -value = Arrays.copyOf(value,
>> -newCapacity(minimumCapacity) << coder);
>> +value = new byte[newCapacity(minimumCapacity) << coder];
>> +System.arraycopy(oldValue, 0, value, 0, count << coder);
>>   }
>>   }
>>
>
>

Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Peter Levart]

Hi Adam,

Did you know that native memory is already tracked on the Java side for
direct ByteBuffers? See class java.nio.Bits. Could you make use of it?

Regards, Peter

On 23 Feb 2018 1:09 pm, "Adam Farley8"  wrote:

> Hi Paul,
>
> The larger picture for (read: effect of) these changes is best explained
> in my email here:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-
> February/051441.html
>
> See the hyperlink I posted, and the few lines before it.
>
> Unfortunately the only understanding I have regarding the workings of the
> native code
> is would be derived from the OpenJ9 implimentation. I figured I wouldn't
> be thanked for
> posting that code here, so I posted what code I could share, with the
> additional note
> that the Hotspot native side of this should be implimented by:
>
> 1) Turning those Unsafe.java methods into native methods, and make them
> abstract
> (descriptor only, for the uninitiated).
>
> 2) Find the Hotspot native code for native memory allocation,
> reallocation, and
> freeing. Basically create a method that stores the sum total amount of
> native memory
> used by the DBBs, and then calls the regular allocate/reallocate/free
> methods.
>
> E.g.
>
> NM Allocate (Java) - NM Allocate (Native)
>
> DBB NM Allocate (Java) - DBB NM Allocate (Native) - NM allocate (Native)
>
> 3) Find the code that prints the current native memory usage in core
> files, and
> add a similar bit to show the native memory usage for DBBs as a subset
> (see the
> aforementioned linked link for an example).
>
> This seems like a straightforward task, though that's easy for me to say.
> :)
>
> Does that answer your question?
>
> Also, I'm unfamiliar with Java Flight Recorder. Are other developers on
> the list
> familiar with JFR that can snwer this? I'll put the message in IRC as
> well, and
> update here if I get any answers.
>
> Best Regards
>
> Adam Farley
>
>
>
> From:   Paul Sandoz 
> To: Adam Farley8 
> Cc: core-libs-dev , hotspot-dev
> developers 
> Date:   22/02/2018 02:20
> Subject:Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track
> Native Memory Usage for Direct Byte Buffers
>
>
>
> Hi Adam,
>
> While the burden is minimal there is a principle here that i think we
> should adhere to regarding additions to the code base: additions should
> have value within OpenJDK itself otherwise it can become a thin end of the
> wedge to more stuff (“well you added these things, why not just add these
> too?”).
>
> So i would still be reluctant to add such methods without understanding
> the larger picture and what you have in mind.
>
> Can you send a pointer to your email referring in more detail to the
> larger change sets?
>
> This use-case might also apply in other related areas too with regards to
> logging/monitoring. I would be interested to understand what Java Flight
> Recorder (JFR) does in this regard (it being open sourced soon i believe)
> and how JFR might relate to what you are doing. Should we be adding JFR
> events to unsafe memory allocation? Can JFR efficiently access part of the
> Java call stack to determine the origin?
>
> Thanks,
> Paul.
>
> On Feb 19, 2018, at 5:08 AM, Adam Farley8  wrote:
>
> Hi Paul,
>
> > Hi Adam,
> >
> > From reading the thread i cannot tell if this is part of a wider
> solution including some yet to be proposed HotSpot changes.
>
> The wider solution would need to include some Hotspot changes, yes.
> I'm proposing raising a bug, committing the code we have here to
> "set the stage", and then we can invest more time later
> if the concept goes down well and the community agrees to pursue
> the full solution.
>
> As an aside, I tried submitting a big code set (including hotspot
> changes) months ago, and I'm *still* struggling to find someone to
> commit the thing, so I figured I'd try a more gradual, staged approach
> this time.
>
> >
> > As is i would be resistant to adding such standalone internal wrapper
> methods to Unsafe that have no apparent benefit within the OpenJDK itself
> since it's a maintenance burden.
>
> I'm hoping the fact that the methods are a single line (sans
> comments, descriptors and curly braces) will minimise this burden.
>
> >
> > Can you determine if the calls to UNSAFE.freeMemory/allocateMemory come
> from a DBB by looking at the call stack frame above the unsafe call?
> >
> > Thanks,
> > Paul.
>
> Yes that is possible, though I would advise against this because:
>
>  A) Checking the call stack is expensive, and doing this every time we
> allocate native memory is an easy way to slow down a program,
> or rack up mips.
> and
>  B) deciding which code path we're using based on the stack
> means the DBB class+method (and anything the parsing code
> mistakes for that class+method) can only ever allocate native
> memory for DBBs.
>
> 

Re: RFR: Here are some URLClassPath patches

[Alan Bateman]

Just getting to the updated webrev now.

On 21/02/2018 20:30, Martin Buchholz wrote:

:


8198480: Improve ClassLoaders static init block
http://cr.openjdk.java.net/~martin/webrevs/jdk/ClassLoaders-static/ 


https://bugs.openjdk.java.net/browse/JDK-8198480
The value of jdk.module.main is the name of the "initial module" so 
better to use that instead term of "main module".





8198481: Coding style cleanups for 
src/java.base/share/classes/jdk/internal/loader
http://cr.openjdk.java.net/~martin/webrevs/jdk/loader-style/ 


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

Looks okay.



8198482: The URLClassPath field "urls" should be renamed to "unopenedUrls"
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-rename-urls/ 


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

Looks okay.



8198484: URLClassPath should use an ArrayDeque instead of a Stack
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-ArrayDeque/ 


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

Can copyToArrayDeque use addAll?




8198485: Simplify a URLClassPath constructor
http://cr.openjdk.java.net/~martin/webrevs/jdk/URLClassPath-simplify-constructor/ 


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


Looks okay.

Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track Native Memory Usage for Direct Byte Buffers

[Adam Farley8]

Hi Paul,

The larger picture for (read: effect of) these changes is best explained 
in my email here:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051441.html

See the hyperlink I posted, and the few lines before it.

Unfortunately the only understanding I have regarding the workings of the 
native code
is would be derived from the OpenJ9 implimentation. I figured I wouldn't 
be thanked for 
posting that code here, so I posted what code I could share, with the 
additional note 
that the Hotspot native side of this should be implimented by:

1) Turning those Unsafe.java methods into native methods, and make them 
abstract
(descriptor only, for the uninitiated).

2) Find the Hotspot native code for native memory allocation, 
reallocation, and 
freeing. Basically create a method that stores the sum total amount of 
native memory
used by the DBBs, and then calls the regular allocate/reallocate/free 
methods.

E.g.

NM Allocate (Java) - NM Allocate (Native)

DBB NM Allocate (Java) - DBB NM Allocate (Native) - NM allocate (Native)

3) Find the code that prints the current native memory usage in core 
files, and 
add a similar bit to show the native memory usage for DBBs as a subset 
(see the
aforementioned linked link for an example).

This seems like a straightforward task, though that's easy for me to say. 
:)

Does that answer your question?

Also, I'm unfamiliar with Java Flight Recorder. Are other developers on 
the list
familiar with JFR that can snwer this? I'll put the message in IRC as 
well, and
update here if I get any answers.

Best Regards

Adam Farley



From:   Paul Sandoz 
To: Adam Farley8 
Cc: core-libs-dev , hotspot-dev 
developers 
Date:   22/02/2018 02:20
Subject:Re: [PATCH] RFR Bug-pending: Enable Hotspot to Track 
Native Memory Usage for Direct Byte Buffers



Hi Adam,

While the burden is minimal there is a principle here that i think we 
should adhere to regarding additions to the code base: additions should 
have value within OpenJDK itself otherwise it can become a thin end of the 
wedge to more stuff (“well you added these things, why not just add these 
too?”).

So i would still be reluctant to add such methods without understanding 
the larger picture and what you have in mind.

Can you send a pointer to your email referring in more detail to the 
larger change sets?

This use-case might also apply in other related areas too with regards to 
logging/monitoring. I would be interested to understand what Java Flight 
Recorder (JFR) does in this regard (it being open sourced soon i believe) 
and how JFR might relate to what you are doing. Should we be adding JFR 
events to unsafe memory allocation? Can JFR efficiently access part of the 
Java call stack to determine the origin?

Thanks,
Paul.

On Feb 19, 2018, at 5:08 AM, Adam Farley8  wrote:

Hi Paul, 

> Hi Adam, 
> 
> From reading the thread i cannot tell if this is part of a wider 
solution including some yet to be proposed HotSpot changes. 

The wider solution would need to include some Hotspot changes, yes. 
I'm proposing raising a bug, committing the code we have here to 
"set the stage", and then we can invest more time later 
if the concept goes down well and the community agrees to pursue 
the full solution. 

As an aside, I tried submitting a big code set (including hotspot 
changes) months ago, and I'm *still* struggling to find someone to 
commit the thing, so I figured I'd try a more gradual, staged approach 
this time. 

> 
> As is i would be resistant to adding such standalone internal wrapper 
methods to Unsafe that have no apparent benefit within the OpenJDK itself 
since it's a maintenance burden. 

I'm hoping the fact that the methods are a single line (sans 
comments, descriptors and curly braces) will minimise this burden. 

> 
> Can you determine if the calls to UNSAFE.freeMemory/allocateMemory come 
from a DBB by looking at the call stack frame above the unsafe call? 
> 
> Thanks, 
> Paul. 

Yes that is possible, though I would advise against this because: 

 A) Checking the call stack is expensive, and doing this every time we 
allocate native memory is an easy way to slow down a program, 
or rack up mips. 
and 
 B) deciding which code path we're using based on the stack 
means the DBB class+method (and anything the parsing code 
mistakes for that class+method) can only ever allocate native 
memory for DBBs. 

What do you think? 

Best Regards 

Adam Farley 

> 
>> On Feb 14, 2018, at 3:32 AM, Adam Farley8  
wrote: 
>> 
>> Hi All, 
>> 
>> Currently, diagnostic core files generated from OpenJDK seem to lump 
all 
>> of the 
>> native memory usages together, making it near-impossible for someone to 

>> figure 
>> out *what* is using all that memory in the event of a memory leak. 
>> 
>> The OpenJ9 VM 

RE: RFR (XS): 8198539: Cleanup of unused imports in java/util/jar/Attributes.java (java.base) and JdpController.java (jdk.management.agent)

[Langer, Christoph]

Thanks, Thomas.

From: Thomas Stüfe [mailto:thomas.stu...@gmail.com]
Sent: Donnerstag, 22. Februar 2018 09:54
To: Langer, Christoph 
Cc: Java Core Libs ; 
serviceability-...@openjdk.java.net; andrew_m_leon...@uk.ibm.com
Subject: Re: RFR (XS): 8198539: Cleanup of unused imports in 
java/util/jar/Attributes.java (java.base) and JdpController.java 
(jdk.management.agent)

Hi Christoph,

Looks fine.

.. Thomas

On Feb 22, 2018 09:42, "Langer, Christoph" 
> wrote:
Hi,

please review a simple import cleanup fix for java/util/jar/Attributes.java and 
sun/management/jdp/JdpController.java.

Bug: https://bugs.openjdk.java.net/browse/JDK-8198539
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8198539.0/

Thanks and best regards
Christoph

Re: [11] RFR JDK-8060094: java/util/Formatter/Basic.java failed in tr locale

[Nishit Jain]

Thanks Naoto,

Please check the updated webrev
http://cr.openjdk.java.net/~nishjain/8060094/webrev.04/

Edits made: In FormatLocale.java, clarified the exception messages about 
the locale used and removed an unused import.


Regards,
Nishit Jain
On 23-02-2018 00:56, Naoto Sato wrote:

Hi Nishit,

In the test case, the exception error messages may be more helpful if 
there is a distinction between the two (line 103 and 120) mentioning 
the formatter is using the default or specified locale.


Naoto

On 2/22/18 3:51 AM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-8060094.

Bug: https://bugs.openjdk.java.net/browse/JDK-8060094
Webrev: http://cr.openjdk.java.net/~nishjain/8060094/webrev.03/
CSR: https://bugs.openjdk.java.net/browse/JDK-8197916

Cause: The Formatter APIs were not using the correct locale for upper 
casing when specified during instance creation or during format() 
call. The default locale was getting used irrespective of whether a 
Locale is specified in the API.

Fix: Modified the APIs to use the locale specified.

Regards,
Nishit Jain