[JDK 11] RFR 8196759: Move two java/text/Normalizer tests into OpenJDK

[Chris Yin]

Please review the changes to move two java/text/Normalizer tests to OpenJDK 
jdk_text group, thanks

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

webrev: http://cr.openjdk.java.net/~xiaofeya/8196759/webrev.00/ 


Regards,
Chris

Re: RFR(XXS) : 8190679 : java/util/Arrays/TimSortStackSize2.java fails with "Initial heap size set to a larger value than the maximum heap size"

[David Holmes]

Hi Igor,

On 27/02/2018 11:25 AM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8190679/webrev.00/index.html

9 lines changed: 2 ins; 0 del; 7 mod;


Hi all,

could you please review the patch for TimSortStackSize2 test?

the test failed when externally passed (via -javaoption or -vmoption) -Xmx 
value is less than 770m or 385m, depending on UseCompressedOops. it happened 
because the test explicitly set Xms value, but didn't set Xmx.
now, the test sets Xmx as Xms times 2.


I'm not happy with setting Xmx at 2 times Xms - that seems to be setting 
ourselves up for another case where we can't set -Xmx at startup. This 
test has encountered problems in the past with external flag settings - 
see in particular the review thread for JDK-8075071:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/032316.html

Will the test pass if we simply set -Xmx and -Xms to the same? Or 
(equivalently based on on previous review discussions) just set -Xmx 
instead of -Xms?


Thanks,
David


PS as it mostly affects hotspot testing, the patch will be pushed to jdk/hs.

webrev: http://cr.openjdk.java.net/~iignatyev//8190679/webrev.00/index.html
testing: java/util/Arrays/TimSortStackSize2.java  w/ and w/o externally 
provided Xmx value
JBS: https://bugs.openjdk.java.net/browse/JDK-8190679

Thanks,
-- Igor

Re: [PATCH] Reduce Chance Of Mistakenly Early Backing Memory Cleanup

[Paul Sandoz]

Hi Ben,

Here is the webrev online:

  
http://cr.openjdk.java.net/~psandoz/jdk/buffer-reachability-fence/webrev/index.html

(I don’t know if you have any colleagues with author or above roles in OpenJDK 
to upload for you, it might be faster.)

Reference.java
—

 423 @ForceInline
 424 public static void reachabilityFence(Object ref) {
 425 // Does nothing, because this method is annotated with @DontInline
 426 // HotSpot needs to retain the ref and not GC it before a call to 
this
 427 // method
 428 }

We need to update the comment, preferably using a summary of Vladimir’s 
analysis.


Direct-X-Buffer-bin.java.template
—

  34 private $type$ get$Type$(long a) {
  35 $memtype$ x = UNSAFE.get$Memtype$Unaligned(null, a, bigEndian);
  36 $type$ y = $fromBits$(x);
  37 Reference.reachabilityFence(this);
  38 return y;
  39 }

It’s overkill in the above case but for good practice reasons i recommend for 
all usages using a try/finally block as suggested in the JavaDoc for 
Reference.reachabilityFence.


Direct-X-Buffer.java.template
—

 260 public $type$ get() {
 261 return 
$fromBits$($swap$(UNSAFE.get$Swaptype$(ix(nextGetIndex();
 262 }
 263 
 264 public $type$ get(int i) {
 265 return $fromBits$($swap$(UNSAFE.get$Swaptype$(ix(checkIndex(i);
 266 }
 267 
 268 #if[streamableType]
 269 $type$ getUnchecked(int i) {
 270 return $fromBits$($swap$(UNSAFE.get$Swaptype$(ix(i;
 271 }
 272 #end[streamableType]

Missing fences. We also need to look carefully at the bulk operations as well, 
you have a fence for the bulk put accepting a ByteBuffer although a fence is 
likely required on the src as well.


 506 byte _get(int i) {  // package-private
 507 return UNSAFE.getByte(address + i);
 508 }

AFAICT the _get and _put methods are no longer used and the code could be 
deleted (left over from other refactoring to the view classes).

Thanks,
Paul.


 

> On Feb 19, 2018, at 8:37 AM, Ben Walsh  wrote:
> 
> As requested, here are the results with modifications to the annotations 
> on Reference.reachabilityFence. Much more promising ...
> 
> 
> * Benchmark 1 *
> 
> Test Code :
> 
> package org.sample;
> 
> import org.openjdk.jmh.annotations.Benchmark;
> import org.openjdk.jmh.annotations.Level;
> import org.openjdk.jmh.annotations.Scope;
> import org.openjdk.jmh.annotations.Setup;
> import org.openjdk.jmh.annotations.State;
> 
> import java.nio.ByteBuffer;
> 
> public class ByteBufferBenchmark {
> 
>@State(Scope.Benchmark)
>public static class ByteBufferContainer {
> 
>ByteBuffer bb;
> 
>@Setup(Level.Invocation)
>public void initByteBuffer() {
>bb = ByteBuffer.allocateDirect(1);
>}
> 
>ByteBuffer getByteBuffer() {
>return bb;
>}
>}
> 
>@Benchmark
>public void benchmark_byte_buffer_put(ByteBufferContainer bbC) {
> 
>bbC.getByteBuffer().put((byte)42);
>}
> 
> }
> 
> Results :
> 
> - Unmodified Build -
> 
> Benchmark   Mode  Cnt Score   
> Error  Units
> ByteBufferBenchmark.benchmark_byte_buffer_put  thrpt  200  35604933.518 ± 
> 654975.515  ops/s
> 
> - Build With Reference.reachabilityFences Added -
> 
> Benchmark   Mode  Cnt Score   
> Error  Units  Impact
> ByteBufferBenchmark.benchmark_byte_buffer_put  thrpt  200  33100911.857 ± 
> 747461.951  ops/s  -7.033%
> 
> - Build With Reference.reachabilityFences Added And DontInline Replaced 
> With ForceInline -
> 
> Benchmark   Mode  Cnt Score   
> Error  Units  Impact
> ByteBufferBenchmark.benchmark_byte_buffer_put  thrpt  200  34836320.294 ± 
> 640188.408  ops/s  -2.159%
> 
> - Build With Reference.reachabilityFences Added And DontInline Removed -
> 
> Benchmark   Mode  Cnt Score   
> Error  Units  Impact
> ByteBufferBenchmark.benchmark_byte_buffer_put  thrpt  200  34740015.332 ± 
> 556578.542  ops/s  -2.429%
> 
> 
> * Benchmark 2 *
> 
> Test Code :
> 
> package org.sample;
> 
> import org.openjdk.jmh.annotations.Benchmark;
> import org.openjdk.jmh.annotations.Level;
> import org.openjdk.jmh.annotations.Param;
> import org.openjdk.jmh.annotations.Scope;
> import org.openjdk.jmh.annotations.Setup;
> import org.openjdk.jmh.annotations.State;
> 
> import java.nio.ByteBuffer;
> 
> @State(Scope.Benchmark)
> public class ByteBufferBenchmark {
> 
>@Param({"1", "10", "100", "1000", "1"})
>public int L;
> 
>@State(Scope.Benchmark)
>public static class ByteBufferContainer {
> 
>ByteBuffer bb;
> 
>@Setup(Level.Invocation)
>public void initByteBuffer() {
>bb = ByteBuffer.allocateDirect(1);
>}
> 
>ByteBuffer getByteBuffer() {
>

RFR(XXS) : 8190679 : java/util/Arrays/TimSortStackSize2.java fails with "Initial heap size set to a larger value than the maximum heap size"

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8190679/webrev.00/index.html
> 9 lines changed: 2 ins; 0 del; 7 mod; 

Hi all,

could you please review the patch for TimSortStackSize2 test?

the test failed when externally passed (via -javaoption or -vmoption) -Xmx 
value is less than 770m or 385m, depending on UseCompressedOops. it happened 
because the test explicitly set Xms value, but didn't set Xmx.
now, the test sets Xmx as Xms times 2.

PS as it mostly affects hotspot testing, the patch will be pushed to jdk/hs.

webrev: http://cr.openjdk.java.net/~iignatyev//8190679/webrev.00/index.html
testing: java/util/Arrays/TimSortStackSize2.java  w/ and w/o externally 
provided Xmx value
JBS: https://bugs.openjdk.java.net/browse/JDK-8190679

Thanks,
-- Igor

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Mon, Feb 26, 2018 at 3:38 PM, mandy chung  wrote:

>
>
> On 2/26/18 3:26 PM, Martin Buchholz wrote:
>
>
> Looks okay.  I also think no need to have a separate copyToArrayDeque
>> method and just inline in the constructor.
>>
>
> It's used twice.  Also, it's likely to be replaced someday when we decide
> what to do with lambdas, so good to keep as a separate method.
>
>
> The first use can be replaced with very simple code:
>
> ArrayList path = new ArrayList<>(urls.length);
> ArrayDeque unopenedUrls = new ArrayDeque<>(urls.length);
> for (URL url : urls) {
> path.add(url);
> unopenedUrls.add(url);
> }
>
> I had trouble deciding which way was better, so let's do it your way!
toArrayDeque is gone.

Re: RFR: 8197594 - String and character repeat

[Stuart Marks]

On 2/18/18 1:37 AM, James Laskey wrote:

Didn’t I hear someone mentioning “\U1D11A” at some point?


On 2/19/18 7:55 AM, Martin Buchholz wrote:
Oops, I already got it wrong - it's already at 6 hex digits because there are 17 
planes, not 16.  MAX_CODE_POINT is U+10.

Yes, we need a variable width syntax like regex \x{h...h}


Yeah, there are a bunch of syntactic alternatives to consider. An "obvious" 
alternative to "\u" is "\Uxx" which works if you're always willing to 
specify six digits (or to have some weird non-delimited but variable-length 
sequence, such as the existing octal escapes for chars (does anybody use those 
(see JLS 3.10.6)?)) The difference between \u and \U is rather subtle, though. 
Or a delimited sequence such as used by regex might be an alternative.



And java regex also supports
   \N{name}The character with Unicode character name 'name'
so we could do the same for the java language.
Although it would be a little weird to have every Unicode update make some 
previously invalid source files valid.


We could also say "It's 2018 and UTF-8 has won" and simply use UTF-8 in source 
files directly.   No Unicode escapes needed.


Even if one is willing to have a source file in UTF-8 (as opposed to say, ASCII) 
there are things in Unicode that are really hard to edit. For example, there are 
zero-width spaces, joiners, non-joiners, directionality markers, etc. I think 
escapes are the bare minimum. Some kind of name-based interpolation would be 
useful, but the actual Unicode names are rather unwieldy. Maybe something like 
HTML entities would be worthwhile to investigate, though probably with a 
different syntax.


s'marks

Re: RFR: 8197594 - String and character repeat

[Stuart Marks]

To close the loop on this, I've reopened JDK-4993841, which requests adding an 
API Character.toString(int) which converts an int codepoint value to a String.


(This seems like the obvious API, in parallel with Character.toString(char), but 
of course alternatives could be considered.)


s'marks

On 2/20/18 11:46 AM, Louis Wasserman wrote:

I'm with Brian: adding a separate API to make it easier to get from a
codepoint to a String seems independently merited, and makes the single
repeat API work well for that case.  A very quick regex-powered search
comes up with 183 hits in Google for (new
String|String.copyValueOf|String.valueOf)(Character.toChars(..)).

I do, however, recommend a separate thread for discussing that API :)

On Tue, Feb 20, 2018 at 11:33 AM Kevin Bourrillion 
wrote:


Just to add another dimension to this data:  most of the usages of our
repeat method (~75%) are in test code. These tests usually just want any
old test string of a certain length. Repeating a single character is the
obvious way to get that.

Among production code usages (~25%), there are a few roughly equal use
cases: ascii indentation/alignment, redaction, and Martin's expected case
of "drawing" with ASCII symbols, and "other".



On Thu, Feb 15, 2018 at 12:52 PM, Louis Wasserman 
wrote:


I don't think there's a case for demand to merit having a
repeat(CharSequence, int) at all.

I did an analysis of usages of Guava's Strings.repeat on Google's
codebase.  Users might be rolling their own implementations, too, but this
should be a very good proxy for demand.

StringRepeat_SingleConstantChar = 4.475 K // strings with .length() ==  1
StringRepeat_SingleConstantCodePoint = 28 // strings with
.codePointCount(...) == 1
StringRepeat_MultiCodePointConstant = 1.156 K // constant strings neither
of the above
StringRepeat_CharSequenceToString = 2 //
Strings.repeat(CharSequence.toString(), n)
StringRepeat_NoneOfTheAbove = 248

Notably, it seems like basically nobody needs to repeat a CharSequence --
definitely not enough demand to merit the awkwardness of e.g.
Rope.repeat(n) inheriting a repeat returning a String.

Based on this data, I'd recommend providing one and only one method of
this
type: String.repeat(int).  There's no real advantage to a static
repeat(char, int) method when the overwhelming majority of these are
constants: e.g. compare SomeUtilClass.repeat('*', n) versus "*".repeat(n).
Character.toString(c).repeat(n) isn't a bad workaround if you don't have a
constant char.  There also isn't much demand for dealing with the code
point case specially; the String.repeat(int) method seems like it'd handle
that just fine.

On Thu, Feb 15, 2018 at 11:44 AM Jim Laskey 
wrote:





On Feb 15, 2018, at 3:36 PM, Ivan Gerasimov <

ivan.gerasi...@oracle.com>

wrote:


Hello!

The link with the webrev returned 404, but I could find it at this

location: http://cr.openjdk.java.net/~jlaskey/8197594/webrev-00/


A few minor comments:

1)

This check:

2992 final long limit = (long)count * 2L;
2993 if ((long)Integer.MAX_VALUE < limit) {

can be possibly simplified as
if (count > Integer.MAX_VALUE - count) {


Good.



2)
Should String repeat(final int codepoint, final int count) be

optimized

for codepoints that can be represented with a single char?


E.g. like this:

public static String repeat(final int codepoint, final int count) {
return Character.isBmpCodePoint(codepoint))
? repeat((char) codepoint, count)
: (new String(Character.toChars(codepoint))).repeat(count);
}


Yes, avoid array allocation.



3)
Using long arithmetic can possibly be avoided in the common path of

repeat(final int count):


E.g. like this:

 if (count < 0) {
 throw new IllegalArgumentException("count is negative, " +

count);

 } else if (count == 1) {
 return this;
 } else if (count == 0) {
 return "";
}
 final int len = value.length;
 if (Integer.MAX_VALUE / count < len) {
 throw new IllegalArgumentException(
 "Resulting string exceeds maximum string length:

" +

((long)len * (long)count));

 }
 final int limit = count * len;


Good.

Thank you.



With kind regards,
Ivan

On 2/15/18 9:20 AM, Jim Laskey wrote:

This is a pre-CSR code review [1] for String repeat methods

(Enhancement).


The proposal is to introduce four new methods;

1. public String repeat(final int count)
2. public static String repeat(final char ch, final int count)
3. public static String repeat(final int codepoint, final int count)
4. public static String repeat(final CharSequence seq, final int

count)


For the sake of transparency, only 1 is necessary, 2-4 are

convenience

methods.

In the case of 2, “*”.repeat(10) performs as well as

String.repeat(‘*’,

10).

3 and 4 convert to String before calling 1.

Performance runs with jmh (results as comment 

Re: RFR: Here are some URLClassPath patches

[mandy chung]

On 2/26/18 3:26 PM, Martin Buchholz wrote:



Looks okay.  I also think no need to have a separate
copyToArrayDeque method and just inline in the constructor.


It's used twice.  Also, it's likely to be replaced someday when we 
decide what to do with lambdas, so good to keep as a separate method.


The first use can be replaced with very simple code:

ArrayList path = new ArrayList<>(urls.length);
ArrayDeque unopenedUrls = new ArrayDeque<>(urls.length);
for (URL url : urls) {
path.add(url);
unopenedUrls.add(url);
}

Mandy

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Mon, Feb 26, 2018 at 3:06 PM, mandy chung  wrote:

>
>
> On 2/21/18 12:30 PM, Martin Buchholz wrote:
> line 63-64: ident further to the right as line 62 is the condition.
>
>
Whitespace rejiggered.


> Looks okay.  I also think no need to have a separate copyToArrayDeque
> method and just inline in the constructor.
>

It's used twice.  Also, it's likely to be replaced someday when we decide
what to do with lambdas, so good to keep as a separate method.

Re: RFR: Here are some URLClassPath patches

[mandy chung]

On 2/26/18 3:10 PM, Martin Buchholz wrote:



On Mon, Feb 26, 2018 at 3:06 PM, mandy chung > 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




Can you rename initialModuleName to mainModule as Alan suggests
(also in the comment)?


It's the other way around - I renamed it from mainModuleName to 
initialModuleName based on Alan's comment.  (and the code elsewhere 
continues to be inconsistent on this)


OK.   I agree we should follow up and make them consistent.

Mandy

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Mon, Feb 26, 2018 at 3:06 PM, mandy chung  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
>
>
> Can you rename initialModuleName to mainModule as Alan suggests (also in
> the comment)?
>

It's the other way around - I renamed it from mainModuleName to
initialModuleName based on Alan's comment.  (and the code elsewhere
continues to be inconsistent on this)

Re: RFR: Here are some URLClassPath patches

[mandy chung]

On 2/21/18 12:30 PM, Martin Buchholz wrote:

OK, we have a reworked set of patches.

(In my corner of openjdk we generally use real javadoc on private 
elements.)


I reverted private doc comment style to the current maddening 
inconsistency, except I couldn't restrain myself from fixing


-    // ACC used when loading classes and resources */
+    // ACC used when loading classes and resources

I changed ArrayDeque.push to addFirst, as Peter suggested.


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


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



Can you rename initialModuleName to mainModule as Alan suggests (also in 
the comment)?

line 63-64: ident further to the right as line 62 is the condition.

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



+1


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



Looks okay.  I also think no need to have a separate copyToArrayDeque 
method and just inline in the constructor.



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


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



+1

Mandy

Re: RFR: Here are some URLClassPath patches

[Paul Sandoz]

> On Feb 26, 2018, at 1:25 PM, Martin Buchholz  wrote:
> 
> 
> 
> On Mon, Feb 26, 2018 at 9:28 AM, Paul Sandoz  > wrote:
> 
> 
> > On Feb 23, 2018, at 2:09 PM, Martin Buchholz  > > wrote:
> >
> > [+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?
> >
> 
> 
> > On Feb 23, 2018, at 2:09 PM, Martin Buchholz  > > wrote:
> 
> > 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).
> >
> 
> Some data on how many lambdas/methods refs are used by the core collection 
> classes could help. I would be wary of going on a lambda purge right now and 
> biasing certain collection classes towards their use at startup if we can 
> avoid that with careful management.
> 
> I would be tempted to drop the method copyToArrayDeque and just rely on the 
> push method (even though it uses synchronized), then add a comment to the 
> unopenedUrls field and/or in the constructor.
> 
> 
> I prefer to keep things as I have them for now.  Calling push requires an 
> extra copy to create the array,

Ok, fair point,
Paul.

> and even though that overhead is swamped by other inefficiencies, it's a tax 
> on every single java program (and the tax is higher at Google, where we have 
> trouble fitting the classpath into Linux' 128 kb per-arg command line length 
> limit).
> 

Re: RFR: Here are some URLClassPath patches

[Martin Buchholz]

On Mon, Feb 26, 2018 at 9:28 AM, Paul Sandoz  wrote:

>
>
> > On Feb 23, 2018, at 2:09 PM, Martin Buchholz 
> wrote:
> >
> > [+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?
> >
>
>
> > On Feb 23, 2018, at 2:09 PM, Martin Buchholz 
> wrote:
>
> > 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).
> >
>
> Some data on how many lambdas/methods refs are used by the core collection
> classes could help. I would be wary of going on a lambda purge right now
> and biasing certain collection classes towards their use at startup if we
> can avoid that with careful management.
>
> I would be tempted to drop the method copyToArrayDeque and just rely on
> the push method (even though it uses synchronized), then add a comment to
> the unopenedUrls field and/or in the constructor.
>
>
I prefer to keep things as I have them for now.  Calling push requires an
extra copy to create the array, and even though that overhead is swamped by
other inefficiencies, it's a tax on every single java program (and the tax
is higher at Google, where we have trouble fitting the classpath into
Linux' 128 kb per-arg command line length limit).

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

[Adam Petcher]

Thanks for the initial feedback. Here is the latest webrev:

http://cr.openjdk.java.net/~apetcher/8181594/webrev.01/

See inline below.

On 2/23/2018 12:46 PM, Xuelei Fan wrote:


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.


It is also used by XDHPublicKeyImpl (in the XDH code review). XDH public 
keys are represented as BigInteger, and I use the array reverse method 
to convert encoded keys to BigInteger.




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?


Good idea. I moved it into the same directory as TestIntegerModuloP.java.



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


In ECC, we also have curves over binary fields, and some methods in 
these interfaces/classes will not work correctly if the number of field 
elements is not prime (e.g. Fermat's little theorem is used to calculate 
a multiplicative inverse). So I would prefer that the names of these 
interfaces contain some string (e.g. "Prime" or "P") to make it clear 
that they only represent prime fields.


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



I'm not fond of the current names, either. My goal with the naming was 
to indicate that "IntegerModuloP_Base" shouldn't be used in most cases, 
because you don't know whether it is mutable or not. So the typical 
interface to use is the immutable one, and the mutable one should only 
be used when mutability is necessary. Your suggested naming is more 
conventional, but I think it loses this aspect of my naming system.


It's only an internal API, so I'm not too picky about the names, and I 
have no problem changing it as you suggest. But I'd like to see if 
anyone else has any suggestions about the names, first.




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


I couldn't come up with a way to implement this without branching when 
the swap parameter is boolean. See IntegerPolynomial.conditionalSwap to 
see how this is implemented in arithmetic with an int swap argument. If 
you (or anyone) can think of a way to do this with boolean, let me know.


I added a sentence to the comment above conditionalSwapWith that 
describes why it is an int instead of a boolean.




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?


The comment above the class definition has this sentence:

"This interface can be used to improve performance and avoid the 
allocation of a large number of temporary objects."


Do you need more information than this in the comments? The performance 
motivation is so that a.add(b).multiply(c)... can be done without 
allocating a new buffer for each operation. For example, without mutable 
field elements, an X25519 point multiplication would allocate around 
4,300 temporary arrays totaling 350,000 bytes. If I remember correctly, 
switching the X25519 implementation to mutable field elements reduced 
the point multiplication time by about half.





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


To be precise, it calculates "((this % p) + (b % p)) % 2^m" (where p is 
the prime that defines the field, and m is the desired length, in bits). 
Note that the addition here is normal integer addition (not addition in 
GF(p)).


This operation is not used in XDH, but it is used in Poly1305 to add the 
AES encryption of a nonce to a field element. So you can get more 
information about this operation by reading the Poly1305 paper/RFC.



Besides, what's the benefits of the two methods?  Could we just use:
  this.add(b).asByteArray()


No, because that would calculate "((this + b) mod p) mod 2^m". The value 
of (this + b) can be larger than p, so this would not produce the 
desired result.




I guess, but not very sure, it is for constant time calculation. If 
the 

Re: RFR: Here are some URLClassPath patches

[Paul Sandoz]

> On Feb 23, 2018, at 2:09 PM, Martin Buchholz  wrote:
> 
> [+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?
> 


> On Feb 23, 2018, at 2:09 PM, Martin Buchholz  wrote:

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

Some data on how many lambdas/methods refs are used by the core collection 
classes could help. I would be wary of going on a lambda purge right now and 
biasing certain collection classes towards their use at startup if we can avoid 
that with careful management.

I would be tempted to drop the method copyToArrayDeque and just rely on the 
push method (even though it uses synchronized), then add a comment to the 
unopenedUrls field and/or in the constructor.


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

No current plans to more formally attack this gnarly problem. A start would be 
to gather data on the VM boot code-paths to better understand the sensitive 
areas, from that we might consider black listing some classes so a compiler 
failure is produced. IIRC the circularity errors can often confusing, it might 
be possible to place a check for the required VM init level, thereby producing 
a clearer runtime error. Lazier initialization might help too (i hope with 
constant dynamic stuff, where even though it’s using j.l.invoke it will move 
stuff off the critical path, while also spreading the cost of startup).

Paul.

Re: [11] RFR: (JAXP) 8038043: Xerces Update: XInclude update

[Joe Wang]

Looks good. Thanks!

Best,
Joe

On 2/26/2018 7:34 AM, Aleks Efimov wrote:

Hi Joe,

Thank you for the review. I've updated XIncludeHandler and 
XIncludeTextReader files per your suggestions.
XIncludeTextReader:191 line is still shown in Sdiffs as one line, but 
patch contains correct change that splits it.


New webrev: http://cr.openjdk.java.net/~aefimov/8038043/11/01

Best,
Aleksei


On 02/22/2018 08:39 AM, Joe Wang wrote:

Hi Aleksei,

Thanks for taking the time to work on this!

Looks good overall.

XIncludeHandler: setupCurrentBaseURI method can be private.

XIncludeTextReader: there's a very long line at 191. It would be good 
to fix it so that Sdiffs looks better the next time.


As for the tests, I'm fine with the encoding tests plus passing all 
existing ones.


Best,
Joe

On 2/16/2018 9:53 AM, Aleks Efimov wrote:

Hi,

Please, help to review the update of XInclude related classes from 
the Apache Xerces 2.11.0 source.

JBS:
    https://bugs.openjdk.java.net/browse/JDK-8038043
The webrev:
    http://cr.openjdk.java.net/~aefimov/8038043/11/00/

New regression test has been added to check the updated reporting of 
invalid bytes encountered during the parsing and inclusion of XML 
documents. New test and other regression tests shows no failures.


With Best Regards,
Aleksei

Re: RFR: 8198492: java/lang/StackWalker/CallerFromMain.java failed timeout.

[mandy chung]

On 2/25/18 3:51 PM, Claes Redestad wrote:

Hi,

the JDK-8198418[1] improvements to lambda bootstrapping meant
initialization changed around to allow the possibility of a
bootstrap race, which made it possible to cause a class loading
deadlock when different threads try to initialize classes like
SimpleMethodHandle and SpeciesData at the same time.

Making sure the common ancestor, BoundMethodHandle, is initialized
using the same means before going into the synchronized block in
LambdaForm:createFormsFor seems to be enough to ensure this race
can always be resolved peacefully:

http://cr.openjdk.java.net/~redestad/8198492/jdk.00/


This looks okay.

Mandy

Re: Weird timezone issues with Timestamp.valueOf(String s)

[Stephen Colebourne]

Just to note that you really need someone from the JDBC maintenance
group to comment.

However, my understanding is that Timestamp conceptually represents a
date and time **without reference to a time-zone**, exactly the same
as LocalDateTime. But the implementation is poor, because it stores it
using epoch millis relative to the local time zone.

In general, I prefer to run server-side systems only in UTC to avoid
issues like this.

Stephen


On 9 February 2018 at 17:15, Martin Buchholz  wrote:
> [redirect to core-libs-dev]
>
> On Fri, Feb 9, 2018 at 7:08 AM,  wrote:
>
>> Not that I encourage using date/time classes from the packages of either
>> java.util or java.sql, but sometimes it happens under the hood.
>>
>>
>>
>> We found a weird issue with timestamps.
>>
>> In our (PostgreSQL) database we have a column of SQL type TIMESTAMP - no
>> timezone.
>>
>>
>>
>> When JPA fills our entity field with a java.sql.Timestamp value, it is
>> given
>> an inherent timezone of the system it's running on; in our case it was CET
>> (UTC +1).
>>
>> Now this timezone isn't immediately obvious, because if you print it to
>> system out, it seems to have the correct time. However when you convert it
>> with .toInstant() the timezone rears its ugly head.
>>
>>
>>
>> I digged deeper and found Timestamp.valueOf(String s), it does a lot of
>> magic, but in the end it calls its own deprecated constructor new
>> Timestamp(int year, int month, int date,  int hour, int minute, int second,
>> int nano).
>>
>> That constructor calls the deprecated constructor of java.util.Date(int
>> year, int month, int date,  int hour, int minute, int second) and that is
>> the one doing something with the current timezone on the system.
>>
>> The method Timestamp.valueOf(String s) itself however, is not deprecated
>> and
>> I find this odd.
>>
>> More odd is that Timestamp.valueOf(LocalDateTime dateTime) has a
>> @SuppresWarnings("deprecation") annotation.
>>
>>
>>
>> Thus I found that Timestamp.valueOf("2017-10-04 00:00:00") on a system
>> running in CET timezone yields a different result than one running in UTC
>> timezone.
>>
>> Here is some example code where I put the output of the println's in
>> comments.
>>
>>
>>
>>
>> TimeZone.setDefault(TimeZone.getTimeZone(ZoneId.of("Europe/Amsterdam")));
>>
>> Timestamp fromStringCet = Timestamp.valueOf("2017-10-04 00:00:00");
>>
>> System.out.println(fromStringCet); // 2017-10-04 00:00:00.0
>>
>> System.out.println(fromStringCet.toInstant()); // 2017-10-03T22:00:00Z
>>
>>
>>
>> TimeZone.setDefault(TimeZone.getTimeZone(ZoneOffset.UTC));
>>
>> Timestamp fromStringUtc = Timestamp.valueOf("2017-10-04 00:00:00");
>>
>> System.out.println(fromStringUtc); // 2017-10-04 00:00:00.0
>>
>> System.out.println(fromStringUtc.toInstant()); // 2017-10-04T00:00:00Z
>>
>>
>>
>> System.out.println(fromStringCet.equals(fromStringUtc)); // false
>>
>>
>>
>> LocalDateTime localDateTime = LocalDateTime.of(2017, 10, 4, 0, 0, 0);
>>
>>
>>
>>
>> TimeZone.setDefault(TimeZone.getTimeZone(ZoneId.of("Europe/Amsterdam")));
>>
>> Timestamp fromLocalDateTimeCet = Timestamp.valueOf(localDateTime);
>>
>> System.out.println(fromLocalDateTimeCet.toInstant()); //
>> 2017-10-03T22:00:00Z
>>
>>
>>
>> TimeZone.setDefault(TimeZone.getTimeZone(ZoneOffset.UTC));
>>
>> Timestamp fromLocalDateTimeUtc = Timestamp.valueOf(localDateTime);
>>
>> System.out.println(fromLocalDateTimeUtc.toInstant()); // 2017-10-04
>> 00:00:00.0
>>
>>
>>
>> System.out.println(fromLocalDateTimeCet.equals(fromLocalDateTimeUtc));
>> // false
>>
>>
>>
>> So what to do?
>>
>>
>>
>> Some options are:
>>
>> *   Make Timestamp.valueOf(String s) deprecated?
>> *   Always use UTC when doing the implicit conversion
>>
>>
>>
>> Thoughts?
>>
>>
>>
>> Dave Franken
>>
>>
>>
>>

Re: [11] RFR: (JAXP) 8038043: Xerces Update: XInclude update

[Aleks Efimov]

Hi Joe,

Thank you for the review. I've updated XIncludeHandler and 
XIncludeTextReader files per your suggestions.
XIncludeTextReader:191 line is still shown in Sdiffs as one line, but 
patch contains correct change that splits it.


New webrev: http://cr.openjdk.java.net/~aefimov/8038043/11/01

Best,
Aleksei


On 02/22/2018 08:39 AM, Joe Wang wrote:

Hi Aleksei,

Thanks for taking the time to work on this!

Looks good overall.

XIncludeHandler: setupCurrentBaseURI method can be private.

XIncludeTextReader: there's a very long line at 191. It would be good 
to fix it so that Sdiffs looks better the next time.


As for the tests, I'm fine with the encoding tests plus passing all 
existing ones.


Best,
Joe

On 2/16/2018 9:53 AM, Aleks Efimov wrote:

Hi,

Please, help to review the update of XInclude related classes from 
the Apache Xerces 2.11.0 source.

JBS:
    https://bugs.openjdk.java.net/browse/JDK-8038043
The webrev:
    http://cr.openjdk.java.net/~aefimov/8038043/11/00/

New regression test has been added to check the updated reporting of 
invalid bytes encountered during the parsing and inclusion of XML 
documents. New test and other regression tests shows no failures.


With Best Regards,
Aleksei

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

[Alan Bateman]

On 23/02/2018 19:58, Roger Riggs wrote:
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
I looked at the changes to the manifest code and I think they are okay. 
I don't have time to study the tests in detail.


I think it would be useful to copy the clarification note "(there must 
be a colon and a SPACE after the name)" from the JAR file spec to the 
equivalent sentence in the Attributes class description. That will help 
anyone wondering why the limit is 70 rather than 72. This change 
shouldn't need a CSR as it's not adding any new assertions.


-Alan

Re: RFR: 8198492: java/lang/StackWalker/CallerFromMain.java failed timeout.

[Claes Redestad]

On 2018-02-26 12:21, Alan Bateman wrote:


http://cr.openjdk.java.net/~redestad/8198492/jdk.00/
This looks okay to me to fix the current issue. There are several 
other bugs that look similar and I expect they will go away once you 
get this pushed. It might be useful to move JDK-8198492 to the 
java.lang.invoke subcomponent, and maybe change the summary so that 
it's clearer for anyone looking at in the future.


Thanks!

How about "Bootstrapping java.lang.invoke can cause deadlock after 
JDK-8198418"?


/Claes

Re: RFR: 8198492: java/lang/StackWalker/CallerFromMain.java failed timeout.

[Alan Bateman]

On 25/02/2018 23:51, Claes Redestad wrote:

Hi,

the JDK-8198418[1] improvements to lambda bootstrapping meant
initialization changed around to allow the possibility of a
bootstrap race, which made it possible to cause a class loading
deadlock when different threads try to initialize classes like
SimpleMethodHandle and SpeciesData at the same time.

Making sure the common ancestor, BoundMethodHandle, is initialized
using the same means before going into the synchronized block in
LambdaForm:createFormsFor seems to be enough to ensure this race
can always be resolved peacefully:

http://cr.openjdk.java.net/~redestad/8198492/jdk.00/
This looks okay to me to fix the current issue. There are several other 
bugs that look similar and I expect they will go away once you get this 
pushed. It might be useful to move JDK-8198492 to the java.lang.invoke 
subcomponent, and maybe change the summary so that it's clearer for 
anyone looking at in the future.


-Alan