Re: RFR (JDK11/java.xml) 8199792: Wrong license header in XMLLimitAnalyzer.java

[Roger Riggs]

Hi Joe,

Looks fine

Roger

On 3/20/18 1:57 PM, Joe Wang wrote:

Hi,

Please help review a quick fix for a license header. Note that I 
didn't change the copyright year since there's no code change, and 
also I'd like to maintain the same copyright year as the other classes 
that were added together with this class.


JBS: https://bugs.openjdk.java.net/browse/JDK-8199792
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8199792/webrev/

Thanks,
Joe

Re: RFR 8199843 : Optimize Integer/Long.highestOneBit()

[Brian Burkhalter]

Hi Ivan,

On Mar 20, 2018, at 10:24 AM, Ivan Gerasimov  wrote:

> With non-zero values the new function performs 11-13% worse.
> I guess it's acceptable?

If we are mostly concerned about the intrinsified cases then I would think that 
this would be acceptable.

Brian

Re: RFR (JDK11/java.xml) 8199792: Wrong license header in XMLLimitAnalyzer.java

[Joe Wang]

Thanks!

On 3/20/2018 11:00 AM, Lance Andersen wrote:

+1
On Mar 20, 2018, at 1:57 PM, Joe Wang > wrote:


Hi,

Please help review a quick fix for a license header. Note that I 
didn't change the copyright year since there's no code change, and 
also I'd like to maintain the same copyright year as the other 
classes that were added together with this class.


JBS: https://bugs.openjdk.java.net/browse/JDK-8199792
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8199792/webrev/ 



Thanks,
Joe




Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

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

Re: [11] RFR JDK-8196399, JDK-8199672: Formatting a decimal using locale-specific grouping..., ClassCastException is thrown...

[Naoto Sato]

Looks good to me.

Naoto

On 3/20/18 10:37 AM, Nishit Jain wrote:

Hi,

Please review the fix for JDK-8196399 & JDK-8199672

Bug: https://bugs.openjdk.java.net/browse/JDK-8196399
https://bugs.openjdk.java.net/browse/JDK-8199672
Webrev: http://cr.openjdk.java.net/~nishjain/8196399_8199672/webrev.01/

Issue:
8196399: Locales such as hy_AM do not use grouping, but specify a 
grouping separator. Formatter throws ArithmeticException / by zero while 
identifying the position, as the grouping size is zero.

8199672: Unconditional casting of NumberFormat instance to DecimalFormat

Fix:
8196399: Reset the grouping separator to null character, so that 
grouping separator position identification is skipped.
8199672: Check if the instance returned is DecimalFormat; else, use 
DecimalFormat constructor to obtain the instance.



Regards,
Nishit Jain

Re: RFR (JDK11/java.xml) 8199792: Wrong license header in XMLLimitAnalyzer.java

[Lance Andersen]

+1
> On Mar 20, 2018, at 1:57 PM, Joe Wang  wrote:
> 
> Hi,
> 
> Please help review a quick fix for a license header. Note that I didn't 
> change the copyright year since there's no code change, and also I'd like to 
> maintain the same copyright year as the other classes that were added 
> together with this class.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8199792
> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8199792/webrev/
> 
> Thanks,
> Joe

 
  

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

RFR (JDK11/java.xml) 8199792: Wrong license header in XMLLimitAnalyzer.java

[Joe Wang]

Hi,

Please help review a quick fix for a license header. Note that I didn't 
change the copyright year since there's no code change, and also I'd 
like to maintain the same copyright year as the other classes that were 
added together with this class.


JBS: https://bugs.openjdk.java.net/browse/JDK-8199792
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8199792/webrev/

Thanks,
Joe

Re: RFR 8199773 (bf) XXXBuffer:compareTo method is not working as expected

[Paul Sandoz]

> On Mar 20, 2018, at 10:24 AM, Alan Bateman  wrote:
> 
> 
> 
> On 20/03/2018 17:12, Paul Sandoz wrote:
>> Hi,
>> 
>> Please review this simple fix for a silly mistake when calculating the 
>> result of comparing two buffers:
>> 
>>   
>> http://cr.openjdk.java.net/~psandoz/jdk/JDK-8199773-buffer-compare-value/webrev/
>>  
>> 
>> 
> Looks good to me, we probably should have caught this in the core review.
> 

Thanks. Hard to spot. I am glad the JCK tests found it. I should have realized 
the test needed to do pos/limit as well as pos/limit/slice (the latter caught 
bugs early in the development process).

Paul.

[11] RFR JDK-8196399, JDK-8199672: Formatting a decimal using locale-specific grouping..., ClassCastException is thrown...

[Nishit Jain]

Hi,

Please review the fix for JDK-8196399 & JDK-8199672

Bug: https://bugs.openjdk.java.net/browse/JDK-8196399
https://bugs.openjdk.java.net/browse/JDK-8199672
Webrev: http://cr.openjdk.java.net/~nishjain/8196399_8199672/webrev.01/

Issue:
8196399: Locales such as hy_AM do not use grouping, but specify a 
grouping separator. Formatter throws ArithmeticException / by zero while 
identifying the position, as the grouping size is zero.

8199672: Unconditional casting of NumberFormat instance to DecimalFormat

Fix:
8196399: Reset the grouping separator to null character, so that 
grouping separator position identification is skipped.
8199672: Check if the instance returned is DecimalFormat; else, use 
DecimalFormat constructor to obtain the instance.



Regards,
Nishit Jain

Re: RFR 8199773 (bf) XXXBuffer:compareTo method is not working as expected

[Alan Bateman]

On 20/03/2018 17:12, Paul Sandoz wrote:

Hi,

Please review this simple fix for a silly mistake when calculating the result 
of comparing two buffers:

   http://cr.openjdk.java.net/~psandoz/jdk/JDK-8199773-buffer-compare-value/webrev/ 



Looks good to me, we probably should have caught this in the core review.

-Alan

Re: RFR 8199843 : Optimize Integer/Long.highestOneBit()

[Ivan Gerasimov]

Hi Claes!


On 3/20/18 2:46 AM, Claes Redestad wrote:

Hi,

On 2018-03-20 09:58, Ivan Gerasimov wrote:

Hello!

The hightestOneBit function doesn't have an intrinsic and is 
currently implemented with a dozen of instructions.
Alternatively, it could be implemented as MIN_VALUE >>> 
numberOfLeadingZeros(i), which works for all integers but zero.
The former function gets intrisified by hotspot, which results in 
+27% of throughput (see the jmh results below).


Would you please help review this simple fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8199843
WEBREV: http://cr.openjdk.java.net/~igerasim/8199843/00/webrev/


nice optimization!

Benchmark: 
http://cr.openjdk.java.net/~igerasim/8199843/00/MyBenchmark.java


Benchmark results:

Benchmark(arg)   Mode  Cnt Score Error Units
MyBenchmark.int_testMethod_new   0  thrpt   35 323430664.593 ±  
7492044.171  ops/s
MyBenchmark.int_testMethod_new  42  thrpt   35 298526237.078 ±  
5978291.689  ops/s
MyBenchmark.int_testMethod_new -42  thrpt   35 302903562.073 ±  
7984723.721  ops/s
MyBenchmark.int_testMethod_org   0  thrpt   35 236245042.891 ±  
3635990.596  ops/s
MyBenchmark.int_testMethod_org  42  thrpt   35 237903410.753 ±  
3437684.390  ops/s
MyBenchmark.int_testMethod_org -42  thrpt   35 238472580.618 ±  
2654886.010  ops/s
MyBenchmark.long_testMethod_new  0  thrpt   35 282646114.501 ± 
48028366.305  ops/s
MyBenchmark.long_testMethod_new 42  thrpt   35 282382228.405 ±  
5781529.307  ops/s
MyBenchmark.long_testMethod_new-42  thrpt   35 276724858.286 ±  
6529561.227  ops/s
MyBenchmark.long_testMethod_org  0  thrpt   35 198500211.972 ± 
15096862.367  ops/s
MyBenchmark.long_testMethod_org 42  thrpt   35 215854630.194 ±  
3112930.563  ops/s
MyBenchmark.long_testMethod_org-42  thrpt   35 217992805.521 ±  
2622877.082  ops/s


To nitpick a bit:

Please run with some appropriate time unit, e.g., "-tu us" to make 
results more human readable.

And where are the baseline results? :-)

It'd also be nice to verify we don't regress too much in case there's 
no intrinsic, i.e., test with the

intrinsic disabled.


Good point!

Here are results for Integer.highestOneBit with the intrinsic of 
numberOfLeadingZeros being disabled:

Benchmark   (arg)   Mode  CntScore Error   Units
MyBenchmark.int_testMethod_00_base  0  thrpt   35 324.369 ± 15.437  
ops/us
MyBenchmark.int_testMethod_00_base 42  thrpt   35 307.741 ± 29.623  
ops/us
MyBenchmark.int_testMethod_00_base-42  thrpt   35 324.563 ± 25.039  
ops/us
MyBenchmark.int_testMethod_01_org   0  thrpt   35 231.276 ±  8.392  
ops/us
MyBenchmark.int_testMethod_01_org  42  thrpt   35 230.466 ± 10.557  
ops/us
MyBenchmark.int_testMethod_01_org -42  thrpt   35 238.579 ±  8.257  
ops/us
MyBenchmark.int_testMethod_02_new   0  thrpt   35 326.752 ± 18.400  
ops/us
MyBenchmark.int_testMethod_02_new  42  thrpt   35 200.604 ±  8.139  
ops/us
MyBenchmark.int_testMethod_02_new -42  thrpt   35 212.313 ± 21.284  
ops/us


Base case just returns the argument, thus shows the maximum possible 
upper bound of the throughput.


With non-zero values the new function performs 11-13% worse.
I guess it's acceptable?

With kind regards,
Ivan

Thanks!

/Claes



--
With kind regards,
Ivan Gerasimov

Re: RFR 8199843 : Optimize Integer/Long.highestOneBit()

[Ivan Gerasimov]

Hi Peter!


On 3/20/18 6:05 AM, Peter Levart wrote:

Hi Ivan,

What about branch-less variant?

public static int highestOneBit(int i) {
return i & (MIN_VALUE >>> numberOfLeadingZeros(i));
}


Nice variant!

I tried to run it, but the numbers are non-distinguishable for non-zero 
arguments.

And my variant performs slightly better with zero argument.
So, I think it's reasonable to keep the variant with the ternary operator.

Here are the results:
Benchmark   (arg)   Mode  CntScore Error   Units
MyBenchmark.int_testMethod_00_base  0  thrpt   35  352.509 ± 20.796  
ops/us
MyBenchmark.int_testMethod_00_base 42  thrpt   35 362.955 ±  7.278  
ops/us
MyBenchmark.int_testMethod_00_base-42  thrpt   35 366.084 ±  6.770  
ops/us
MyBenchmark.int_testMethod_01_org   0  thrpt   35 249.340 ±  1.981  
ops/us
MyBenchmark.int_testMethod_01_org  42  thrpt   35 249.007 ±  2.005  
ops/us
MyBenchmark.int_testMethod_01_org -42  thrpt   35 241.866 ±  4.759  
ops/us
MyBenchmark.int_testMethod_02_new   0  thrpt   35 328.389 ±  5.883  
ops/us
MyBenchmark.int_testMethod_02_new  42  thrpt   35 306.381 ±  5.505  
ops/us
MyBenchmark.int_testMethod_02_new -42  thrpt   35 300.328 ±  5.644  
ops/us
MyBenchmark.int_testMethod_03_and   0  thrpt   35 301.559 ±  5.453  
ops/us
MyBenchmark.int_testMethod_03_and  42  thrpt   35  299.694 ± 5.000  
ops/us
MyBenchmark.int_testMethod_03_and -42  thrpt   35  301.505 ± 5.664  
ops/us


The benchmark updated in place:
http://cr.openjdk.java.net/~igerasim/8199843/00/MyBenchmark.java

With kind regards
Ivan


Would it be any better for call sites that vary 0 and non-0 argument?

Regards, Peter

On 03/20/2018 09:58 AM, Ivan Gerasimov wrote:

Hello!

The hightestOneBit function doesn't have an intrinsic and is 
currently implemented with a dozen of instructions.
Alternatively, it could be implemented as MIN_VALUE >>> 
numberOfLeadingZeros(i), which works for all integers but zero.
The former function gets intrisified by hotspot, which results in 
+27% of throughput (see the jmh results below).


Would you please help review this simple fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8199843
WEBREV: http://cr.openjdk.java.net/~igerasim/8199843/00/webrev/
Benchmark: 
http://cr.openjdk.java.net/~igerasim/8199843/00/MyBenchmark.java


Benchmark results:

Benchmark(arg)   Mode  Cnt Score Error Units
MyBenchmark.int_testMethod_new   0  thrpt   35 323430664.593 ±  
7492044.171  ops/s
MyBenchmark.int_testMethod_new  42  thrpt   35 298526237.078 ±  
5978291.689  ops/s
MyBenchmark.int_testMethod_new -42  thrpt   35 302903562.073 ±  
7984723.721  ops/s
MyBenchmark.int_testMethod_org   0  thrpt   35 236245042.891 ±  
3635990.596  ops/s
MyBenchmark.int_testMethod_org  42  thrpt   35 237903410.753 ±  
3437684.390  ops/s
MyBenchmark.int_testMethod_org -42  thrpt   35 238472580.618 ±  
2654886.010  ops/s
MyBenchmark.long_testMethod_new  0  thrpt   35 282646114.501 ± 
48028366.305  ops/s
MyBenchmark.long_testMethod_new 42  thrpt   35 282382228.405 ±  
5781529.307  ops/s
MyBenchmark.long_testMethod_new-42  thrpt   35 276724858.286 ±  
6529561.227  ops/s
MyBenchmark.long_testMethod_org  0  thrpt   35 198500211.972 ± 
15096862.367  ops/s
MyBenchmark.long_testMethod_org 42  thrpt   35 215854630.194 ±  
3112930.563  ops/s
MyBenchmark.long_testMethod_org-42  thrpt   35 217992805.521 ±  
2622877.082  ops/s


Thanks in advance!





--
With kind regards,
Ivan Gerasimov

RFR 8199773 (bf) XXXBuffer:compareTo method is not working as expected

[Paul Sandoz]

Hi,

Please review this simple fix for a silly mistake when calculating the result 
of comparing two buffers:

  
http://cr.openjdk.java.net/~psandoz/jdk/JDK-8199773-buffer-compare-value/webrev/
 


Paul.

Re: RFR: 8199862: Examine ProxyBuilder::referencedTypes startup cost

[mandy chung]

On 3/20/18 6:00 AM, Claes Redestad wrote:

Hi,

by desugaring Stream+lambda usage in 
Proxy$ProxyBuilder::referencedTypes - and carefully avoiding cloning 
of parameter and exception type arrays - we can significantly reduce 
the startup overhead of doing annotation processing in a set of 
typical applications.


Webrev: http://cr.openjdk.java.net/~redestad/8199862/open.00/

Bug: https://bugs.openjdk.java.net/browse/JDK-8199862


This looks fine.   Thanks for doing it.   I started looking into the 
ProxyBuilder performance for annotations which creates proxies for one 
single interface but didn't get back to it.


Mandy

Re: RFR: 8199865: Avoid initializing ShortCache in ProxyGenerator

[mandy chung]

Looks fine.  Nice small perf gain.

Mandy

On 3/20/18 7:47 AM, Claes Redestad wrote:

Hi,

by using Map instead of Map 
(preserving shortness of values) we can avoid having ProxyGenerator 
depend on Short$ShortCache, which is a small startup and footprint win 
(since Integer$IntegerCache is 100% likely to have already been 
initialized):


http://cr.openjdk.java.net/~redestad/8199865/open.00/

Thanks!

/Claes

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

[Xuelei Fan]

I have no other comments.

Thanks,
Xuelei

On 3/20/2018 7:50 AM, Adam Petcher wrote:

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

Comments inline below.

In addition, I also changed the name of IntegerModuloP_Base to 
IntegerModuloP, and IntegerModuloP to ImmutableIntegerModuloP.



On 3/11/2018 12:04 PM, Xuelei Fan wrote:

On 2/26/2018 10:39 AM, Adam Petcher wrote:


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.


If it is not widely used by other classes, please have these methods 
in the class where is get called.   The sun.security.util is exported 
to other modules as well, we may not want to add stuff into this 
package unless it is really necessary.


Okay. I put these methods in BigIntegerModuloP and removed the ArrayUtil 
class. This array reverse code will be duplicated where it is used by 
XDH public keys (in the other code review).






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.


I did not get the point about the need to avoid branching.  Can you 
have more details?


The goal is to avoid things like if(secret){...}, in order to prevent 
the secrets from leaking into side channels (mostly timing and cache). 
The way this method is used by XDH, the swap parameter is a single bit 
of the private key. By storing this bit as an integer, and then doing 
the swap using only integer arithmetic, we can avoid branching which may 
leak the bits of the key.






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.



I see your point.  The benefits is obviously.

OK, why you need the immutable version then? Sounds like the mutable 
version interface is sufficient, including performance. If an 
immutable version is really needed, we can have the implementation 
making the decision.  Accordingly, the conditionalSwapWith() can be 
defined as optional method, if it is not required to be implemented in 
immutable implementation.


It's confusing to me that the immutable and mutable and the base 
versions/interfaces mixed together.  It would be nice if we can 
simplify the interface a little bit.


For internal APIs, sometimes we don't want the same quality level as 
public APIs.  I think this set of class will be widely used by new EC 
curves, ChaCha20/Poly1305, or more in the future.  It would be nice if 
we could do it good at the beginning.


The mutable version adds the conditional swap as well as mutable 
versions of many of the basic operations. The XDH implementation uses 
both the mutable and immutable versions. The immutable version allows me 
to simplify the client code because I don't have to worry about whether 
some value has been modified. For example, the XDH code keeps a 
representation of 0, 1, and the constant that defines the curve as 
immutable values.


So I prefer to have both. It complicates this API a bit, but it allows 
simpler and more robust code in the client of this API.







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 

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

[Adam Petcher]

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

Comments inline below.

In addition, I also changed the name of IntegerModuloP_Base to 
IntegerModuloP, and IntegerModuloP to ImmutableIntegerModuloP.



On 3/11/2018 12:04 PM, Xuelei Fan wrote:

On 2/26/2018 10:39 AM, Adam Petcher wrote:


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.


If it is not widely used by other classes, please have these methods 
in the class where is get called.   The sun.security.util is exported 
to other modules as well, we may not want to add stuff into this 
package unless it is really necessary.


Okay. I put these methods in BigIntegerModuloP and removed the ArrayUtil 
class. This array reverse code will be duplicated where it is used by 
XDH public keys (in the other code review).






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.


I did not get the point about the need to avoid branching.  Can you 
have more details?


The goal is to avoid things like if(secret){...}, in order to prevent 
the secrets from leaking into side channels (mostly timing and cache). 
The way this method is used by XDH, the swap parameter is a single bit 
of the private key. By storing this bit as an integer, and then doing 
the swap using only integer arithmetic, we can avoid branching which may 
leak the bits of the key.






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.



I see your point.  The benefits is obviously.

OK, why you need the immutable version then? Sounds like the mutable 
version interface is sufficient, including performance. If an 
immutable version is really needed, we can have the implementation 
making the decision.  Accordingly, the conditionalSwapWith() can be 
defined as optional method, if it is not required to be implemented in 
immutable implementation.


It's confusing to me that the immutable and mutable and the base 
versions/interfaces mixed together.  It would be nice if we can 
simplify the interface a little bit.


For internal APIs, sometimes we don't want the same quality level as 
public APIs.  I think this set of class will be widely used by new EC 
curves, ChaCha20/Poly1305, or more in the future.  It would be nice if 
we could do it good at the beginning.


The mutable version adds the conditional swap as well as mutable 
versions of many of the basic operations. The XDH implementation uses 
both the mutable and immutable versions. The immutable version allows me 
to simplify the client code because I don't have to worry about whether 
some value has been modified. For example, the XDH code keeps a 
representation of 0, 1, and the constant that defines the curve as 
immutable values.


So I prefer to have both. It complicates this API a bit, but it allows 
simpler and more robust code in the client of this API.







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 

RFR: 8199865: Avoid initializing ShortCache in ProxyGenerator

[Claes Redestad]

Hi,

by using Map instead of Map (preserving 
shortness of values) we can avoid having ProxyGenerator depend on 
Short$ShortCache, which is a small startup and footprint win (since 
Integer$IntegerCache is 100% likely to have already been initialized):


http://cr.openjdk.java.net/~redestad/8199865/open.00/

Thanks!

/Claes

Re: Raw String Literal Library Support

[Jim Laskey]

Summary.

A. Line support.

- Supporting a mix of line terminators `\n|\r\n|\r` is already a well 
established pattern in language parsers, in the JDK (ex. see  
java.nio.file.FileChannelLinesSpliterator) and RegEx (ex. see `\R`). The 
performance difference between checking one vs the three is negligible.

- Yes, Stream stream = 
Pattern.compile("\n|\r\n|\r").splitAsStream(string); is very useful 
(Spliterators rule), but is cumbersome in this expected to be common use case. 
Only so-so streamy. :-)

- BufferedRead.lines() vs. String.lines() is a tricky discussion. It comes down 
to whether the new line is a terminator or a separator.  In the i/o case, it 
seems terminator is the right answer. A well formed text file will have a new 
line at the end of every line.  However, I think you’ll find when people work 
with multi-line strings they think of new line as a separator. Hence, the 
common use of split(“\n”) and “”.split(“\n”).length == 1. Indentation, the 
position of closing delimiter and margin trimming makes that last line very 
fluid.

What clinches the deal is that  
string.lines().collect(joining(“\n”)).equals(string). I’ll ensure both versions 
of lines() have the difference well javadocumented.

- The current Spliterator implementation makes 
String.lines().toArray(String[]::new) an order of magnitude faster than 
split(`\n|\r\n|\r`). That’s why I implemented it for margin management. Faster 
still if no collection/array is constructed.

BTW: split(`\R`) is 2x-3x faster than split(`\n|\r\n|\r`). Nice.

B. Additions to basic trim methods.

- Revamped to become strip, stripLeading, stripTrailing using 
Character.isWhiteSpace(codepoint) as the test (optimized using ch == ‘ ' || ch 
== ‘\t’ || Character.isWhiteSpace(ch)).

- No strong feeling about it, but String.trim() could be recommended for 
deprecation.

C. Margin management.

- String.trimMarkers() as a default to String.trimMarkers(“|”, “|”) is 
reasonable.  Will put it in the CSR for broader discussion.

- Re use of patterns. I think the Stream lines() method will make it 
very easy enough to create custom trim margin lambdas.

D. Escape management.

- Good

Cheers,

— Jim




> On Mar 13, 2018, at 10:47 AM, Jim Laskey  wrote:
> 
> With the announcement of JEP 326 Raw String Literals, we would like to open 
> up a discussion with regards to RSL library support. Below are several 
> implemented String methods that are believed to be appropriate. Please 
> comment on those mentioned below including recommending alternate names or 
> signatures. Additional methods can be considered if warranted, but as always, 
> the bar for inclusion in String is high.
> 
> You should keep a couple things in mind when reviewing these methods.
> 
> Methods should be applicable to all strings, not just Raw String Literals.
> 
> The number of additional methods should be minimized, not adding every 
> possible method.
> 
> Don't put any emphasis on performance. That is a separate discussion.
> 
> Cheers,
> 
> -- Jim
> 
> A. Line support.
> 
> public Stream lines()
> Returns a stream of substrings extracted from this string partitioned by line 
> terminators. Internally, the stream is implemented using a Spliteratorthat 
> extracts one line at a time. The line terminators recognized are \n, \r\n and 
> \r. This method provides versatility for the developer working with 
> multi-line strings.
> Example:
> 
>String string = "abc\ndef\nghi";
>Stream stream = string.lines();
>List list = stream.collect(Collectors.toList());
> 
> Result:
> 
> [abc, def, ghi]
> 
> 
> Example:
> 
>String string = "abc\ndef\nghi";
>String[] array = string.lines().toArray(String[]::new);
> 
> Result:
> 
> [Ljava.lang.String;@33e5ccce // [abc, def, ghi]
> 
> 
> Example:
> 
>String string = "abc\ndef\r\nghi\rjkl";
>String platformString =
>string.lines().collect(joining(System.lineSeparator()));
> 
> Result:
> 
> abc
> def
> ghi
> jkl
> 
> 
> Example:
> 
>String string = " abc  \n   def  \n ghi   ";
>String trimmedString =
> string.lines().map(s -> s.trim()).collect(joining("\n"));
> 
> Result:
> 
> abc
> def
> ghi
> 
> 
> Example:
> 
>String table = `First Name  SurnamePhone
>Al  Albert 555-
>Bob Roberts555-
>Cal Calvin 555-
>   `;
> 
>// Extract headers
>String firstLine = table.lines().findFirst​().orElse("");
>List headings = List.of(firstLine.trim().split(`\s{2,}`));
> 
>// Build stream of maps
>Stream> stream =
>table.lines().skip(1)
> .map(line -> line.trim())
> .filter(line -> !line.isEmpty())
>

Re: RFR 8199843 : Optimize Integer/Long.highestOneBit()

[Peter Levart]

Hi Ivan,

What about branch-less variant?

public static int highestOneBit(int i) {
    return i & (MIN_VALUE >>> numberOfLeadingZeros(i));
}

Would it be any better for call sites that vary 0 and non-0 argument?

Regards, Peter

On 03/20/2018 09:58 AM, Ivan Gerasimov wrote:

Hello!

The hightestOneBit function doesn't have an intrinsic and is currently 
implemented with a dozen of instructions.
Alternatively, it could be implemented as MIN_VALUE >>> 
numberOfLeadingZeros(i), which works for all integers but zero.
The former function gets intrisified by hotspot, which results in +27% 
of throughput (see the jmh results below).


Would you please help review this simple fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8199843
WEBREV: http://cr.openjdk.java.net/~igerasim/8199843/00/webrev/
Benchmark: 
http://cr.openjdk.java.net/~igerasim/8199843/00/MyBenchmark.java


Benchmark results:

Benchmark    (arg)   Mode  Cnt Score Error  Units
MyBenchmark.int_testMethod_new   0  thrpt   35 323430664.593 ±  
7492044.171  ops/s
MyBenchmark.int_testMethod_new  42  thrpt   35 298526237.078 ±  
5978291.689  ops/s
MyBenchmark.int_testMethod_new -42  thrpt   35 302903562.073 ±  
7984723.721  ops/s
MyBenchmark.int_testMethod_org   0  thrpt   35 236245042.891 ±  
3635990.596  ops/s
MyBenchmark.int_testMethod_org  42  thrpt   35 237903410.753 ±  
3437684.390  ops/s
MyBenchmark.int_testMethod_org -42  thrpt   35 238472580.618 ±  
2654886.010  ops/s
MyBenchmark.long_testMethod_new  0  thrpt   35 282646114.501 ± 
48028366.305  ops/s
MyBenchmark.long_testMethod_new 42  thrpt   35 282382228.405 ±  
5781529.307  ops/s
MyBenchmark.long_testMethod_new    -42  thrpt   35 276724858.286 ±  
6529561.227  ops/s
MyBenchmark.long_testMethod_org  0  thrpt   35 198500211.972 ± 
15096862.367  ops/s
MyBenchmark.long_testMethod_org 42  thrpt   35 215854630.194 ±  
3112930.563  ops/s
MyBenchmark.long_testMethod_org    -42  thrpt   35 217992805.521 ±  
2622877.082  ops/s


Thanks in advance!

RFR: 8199862: Examine ProxyBuilder::referencedTypes startup cost

[Claes Redestad]

Hi,

by desugaring Stream+lambda usage in Proxy$ProxyBuilder::referencedTypes 
- and carefully avoiding cloning of parameter and exception type arrays 
- we can significantly reduce the startup overhead of doing annotation 
processing in a set of typical applications.


Webrev: http://cr.openjdk.java.net/~redestad/8199862/open.00/

Bug: https://bugs.openjdk.java.net/browse/JDK-8199862

Thanks!

/Claes

Re: RFR 8199843 : Optimize Integer/Long.highestOneBit()

[Claes Redestad]

Hi,

On 2018-03-20 09:58, Ivan Gerasimov wrote:

Hello!

The hightestOneBit function doesn't have an intrinsic and is currently 
implemented with a dozen of instructions.
Alternatively, it could be implemented as MIN_VALUE >>> 
numberOfLeadingZeros(i), which works for all integers but zero.
The former function gets intrisified by hotspot, which results in +27% 
of throughput (see the jmh results below).


Would you please help review this simple fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8199843
WEBREV: http://cr.openjdk.java.net/~igerasim/8199843/00/webrev/


nice optimization!

Benchmark: 
http://cr.openjdk.java.net/~igerasim/8199843/00/MyBenchmark.java


Benchmark results:

Benchmark    (arg)   Mode  Cnt Score Error  Units
MyBenchmark.int_testMethod_new   0  thrpt   35 323430664.593 ±  
7492044.171  ops/s
MyBenchmark.int_testMethod_new  42  thrpt   35 298526237.078 ±  
5978291.689  ops/s
MyBenchmark.int_testMethod_new -42  thrpt   35 302903562.073 ±  
7984723.721  ops/s
MyBenchmark.int_testMethod_org   0  thrpt   35 236245042.891 ±  
3635990.596  ops/s
MyBenchmark.int_testMethod_org  42  thrpt   35 237903410.753 ±  
3437684.390  ops/s
MyBenchmark.int_testMethod_org -42  thrpt   35 238472580.618 ±  
2654886.010  ops/s
MyBenchmark.long_testMethod_new  0  thrpt   35 282646114.501 ± 
48028366.305  ops/s
MyBenchmark.long_testMethod_new 42  thrpt   35 282382228.405 ±  
5781529.307  ops/s
MyBenchmark.long_testMethod_new    -42  thrpt   35 276724858.286 ±  
6529561.227  ops/s
MyBenchmark.long_testMethod_org  0  thrpt   35 198500211.972 ± 
15096862.367  ops/s
MyBenchmark.long_testMethod_org 42  thrpt   35 215854630.194 ±  
3112930.563  ops/s
MyBenchmark.long_testMethod_org    -42  thrpt   35 217992805.521 ±  
2622877.082  ops/s


To nitpick a bit:

Please run with some appropriate time unit, e.g., "-tu us" to make 
results more human readable.

And where are the baseline results? :-)

It'd also be nice to verify we don't regress too much in case there's no 
intrinsic, i.e., test with the

intrinsic disabled.

Thanks!

/Claes

RFR 8199843 : Optimize Integer/Long.highestOneBit()

[Ivan Gerasimov]

Hello!

The hightestOneBit function doesn't have an intrinsic and is currently 
implemented with a dozen of instructions.
Alternatively, it could be implemented as MIN_VALUE >>> 
numberOfLeadingZeros(i), which works for all integers but zero.
The former function gets intrisified by hotspot, which results in +27% 
of throughput (see the jmh results below).


Would you please help review this simple fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8199843
WEBREV: http://cr.openjdk.java.net/~igerasim/8199843/00/webrev/
Benchmark: http://cr.openjdk.java.net/~igerasim/8199843/00/MyBenchmark.java

Benchmark results:

Benchmark(arg)   Mode  Cnt Score  Error  
Units
MyBenchmark.int_testMethod_new   0  thrpt   35 323430664.593 ±  
7492044.171  ops/s
MyBenchmark.int_testMethod_new  42  thrpt   35 298526237.078 ±  
5978291.689  ops/s
MyBenchmark.int_testMethod_new -42  thrpt   35 302903562.073 ±  
7984723.721  ops/s
MyBenchmark.int_testMethod_org   0  thrpt   35 236245042.891 ±  
3635990.596  ops/s
MyBenchmark.int_testMethod_org  42  thrpt   35 237903410.753 ±  
3437684.390  ops/s
MyBenchmark.int_testMethod_org -42  thrpt   35 238472580.618 ±  
2654886.010  ops/s
MyBenchmark.long_testMethod_new  0  thrpt   35 282646114.501 ± 
48028366.305  ops/s
MyBenchmark.long_testMethod_new 42  thrpt   35 282382228.405 ±  
5781529.307  ops/s
MyBenchmark.long_testMethod_new-42  thrpt   35 276724858.286 ±  
6529561.227  ops/s
MyBenchmark.long_testMethod_org  0  thrpt   35 198500211.972 ± 
15096862.367  ops/s
MyBenchmark.long_testMethod_org 42  thrpt   35 215854630.194 ±  
3112930.563  ops/s
MyBenchmark.long_testMethod_org-42  thrpt   35 217992805.521 ±  
2622877.082  ops/s


Thanks in advance!

--
With kind regards,
Ivan Gerasimov