Code Review Request, 8152237 Support BigInteger.TWO
Hi, Please review the update for the supporting of BigInteger.TWO: http://cr.openjdk.java.net/~xuelei/8152237/webrev/ BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code. Thanks, Xuelei
Only 3 files touched. Are you going to make the s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another bug fix? Thanks Max
On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
Hi,
Please review the update for the supporting of BigInteger.TWO:
http://cr.openjdk.java.net/~xuelei/8152237/webrev/
BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code.
Thanks, Xuelei
On 3/23/2016 12:10 PM, Wang Weijun wrote:
Only 3 files touched. Are you going to make the s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another bug fix?
There are also uses in security components. I will make the update in another bug. Thanks, Xuelei
Thanks Max
On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
Hi,
Please review the update for the supporting of BigInteger.TWO:
http://cr.openjdk.java.net/~xuelei/8152237/webrev/
BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code.
Thanks, Xuelei
On Mar 23, 2016, at 12:48 PM, Xuelei Fan <Xuelei.Fan@Oracle.COM> wrote:
On 3/23/2016 12:10 PM, Wang Weijun wrote:
Only 3 files touched. Are you going to make the s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another bug fix?
There are also uses in security components. I will make the update in another bug.
I see. But why is ObjectIdentifier.java included in this fix? In you only keep BigInteger and BigDecimal, then I have no other comment. Thanks Max
Thanks, Xuelei
Thanks Max
On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
Hi,
Please review the update for the supporting of BigInteger.TWO:
http://cr.openjdk.java.net/~xuelei/8152237/webrev/
BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code.
Thanks, Xuelei
On 3/23/2016 3:34 PM, Wang Weijun wrote:
On Mar 23, 2016, at 12:48 PM, Xuelei Fan <Xuelei.Fan@Oracle.COM> wrote:
On 3/23/2016 12:10 PM, Wang Weijun wrote:
Only 3 files touched. Are you going to make the s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another bug fix?
There are also uses in security components. I will make the update in another bug.
I see. But why is ObjectIdentifier.java included in this fix?
It happens that the other bug touch those files, but ObjectIdentifier.java is not related to that bug. Does it make sense? Thanks, Xuelei
In you only keep BigInteger and BigDecimal, then I have no other comment.
Thanks Max
Thanks, Xuelei
Thanks Max
On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
Hi,
Please review the update for the supporting of BigInteger.TWO:
http://cr.openjdk.java.net/~xuelei/8152237/webrev/
BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code.
Thanks, Xuelei
Then why not fix the 2 bugs in a single changeset? --Max
在 2016年3月23日，17:06，Xuelei Fan <xuelei.fan@oracle.com> 写道：
On 3/23/2016 3:34 PM, Wang Weijun wrote:
On Mar 23, 2016, at 12:48 PM, Xuelei Fan <Xuelei.Fan@Oracle.COM> wrote:
On 3/23/2016 12:10 PM, Wang Weijun wrote:
Only 3 files touched. Are you going to make the s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another bug fix? There are also uses in security components. I will make the update in another bug.
I see. But why is ObjectIdentifier.java included in this fix? It happens that the other bug touch those files, but ObjectIdentifier.java is not related to that bug.
Does it make sense?
Thanks, Xuelei
In you only keep BigInteger and BigDecimal, then I have no other comment.
Thanks Max
Thanks, Xuelei
Thanks Max
On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
Hi,
Please review the update for the supporting of BigInteger.TWO:
http://cr.openjdk.java.net/~xuelei/8152237/webrev/
BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code.
Thanks, Xuelei
On 3/23/2016 5:44 PM, Wang Weijun wrote:
Then why not fix the 2 bugs in a single changeset?
Both need spec update approval. As they are completely different spec update, better to update in 2 enhancements. As you have concerns here, I removed ObjectIdentifier.java from this update. See the new webrev: http://cr.openjdk.java.net/~xuelei/8152237/webrev.01/ Xuelei
--Max
在 2016年3月23日，17:06，Xuelei Fan <xuelei.fan@oracle.com> 写道：
On 3/23/2016 3:34 PM, Wang Weijun wrote:
On Mar 23, 2016, at 12:48 PM, Xuelei Fan <Xuelei.Fan@Oracle.COM> wrote:
On 3/23/2016 12:10 PM, Wang Weijun wrote:
Only 3 files touched. Are you going to make the s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another bug fix? There are also uses in security components. I will make the update in another bug.
I see. But why is ObjectIdentifier.java included in this fix? It happens that the other bug touch those files, but ObjectIdentifier.java is not related to that bug.
Does it make sense?
Thanks, Xuelei
In you only keep BigInteger and BigDecimal, then I have no other comment.
Thanks Max
Thanks, Xuelei
Thanks Max
On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
Hi,
Please review the update for the supporting of BigInteger.TWO:
http://cr.openjdk.java.net/~xuelei/8152237/webrev/
BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code.
Thanks, Xuelei
Sorry for beating the dead horse of ObjectIdentifier.java change, but I’d suggest that if that code is later revisited, it be changed to "first.compareTo(BigInteger.TWO) > 0" instead of “… == 1”. Comparing the return value of compareTo to zero (instead of relying on specific set of return values) is the “suggested idiom for performing these comparisons" as per BigInteger JavaDoc[1] and consistent with the contract of Comparable.compareTo (even though same BigInteger JavaDoc also explicitly specifies that the return values in this particular case are indeed -1, 0, and 1). Attila. [1] http://docs.oracle.com/javase/8/docs/api/java/math/BigInteger.html#compareTo... <http://docs.oracle.com/javase/8/docs/api/java/math/BigInteger.html#compareTo-java.math.BigInteger->
On 23 Mar 2016, at 12:23, Xuelei Fan <xuelei.fan@oracle.com> wrote:
On 3/23/2016 5:44 PM, Wang Weijun wrote:
Then why not fix the 2 bugs in a single changeset?
Both need spec update approval. As they are completely different spec update, better to update in 2 enhancements.
As you have concerns here, I removed ObjectIdentifier.java from this update. See the new webrev:
http://cr.openjdk.java.net/~xuelei/8152237/webrev.01/
Xuelei
--Max
在 2016年3月23日，17:06，Xuelei Fan <xuelei.fan@oracle.com> 写道：
On 3/23/2016 3:34 PM, Wang Weijun wrote:
On Mar 23, 2016, at 12:48 PM, Xuelei Fan <Xuelei.Fan@Oracle.COM> wrote:
On 3/23/2016 12:10 PM, Wang Weijun wrote:
Only 3 files touched. Are you going to make the s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another bug fix? There are also uses in security components. I will make the update in another bug.
I see. But why is ObjectIdentifier.java included in this fix? It happens that the other bug touch those files, but ObjectIdentifier.java is not related to that bug.
Does it make sense?
Thanks, Xuelei
In you only keep BigInteger and BigDecimal, then I have no other comment.
Thanks Max
Thanks, Xuelei
Thanks Max
> On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei.fan@oracle.com> wrote: > > Hi, > > Please review the update for the supporting of BigInteger.TWO: > > http://cr.openjdk.java.net/~xuelei/8152237/webrev/ > > BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code. > > Thanks, > Xuelei
Hi Attila, Good catch about the comparing. I updated the code in my local workspace, and I would ask for code review in another enhancement update soon. Thanks, Xuelei On 3/23/2016 9:15 PM, Attila Szegedi wrote:
Sorry for beating the dead horse of ObjectIdentifier.java change, but I’d suggest that if that code is later revisited, it be changed to "first.compareTo(BigInteger.TWO) > 0" instead of “… == 1”.
Comparing the return value of compareTo to zero (instead of relying on specific set of return values) is the “suggested idiom for performing these comparisons" as per BigInteger JavaDoc[1] and consistent with the contract of Comparable.compareTo (even though same BigInteger JavaDoc also explicitly specifies that the return values in this particular case are indeed -1, 0, and 1).
Attila.
[1] http://docs.oracle.com/javase/8/docs/api/java/math/BigInteger.html#compareTo...
On 23 Mar 2016, at 12:23, Xuelei Fan <xuelei.fan@oracle.com <mailto:xuelei.fan@oracle.com>> wrote:
On 3/23/2016 5:44 PM, Wang Weijun wrote:
Then why not fix the 2 bugs in a single changeset?
Both need spec update approval. As they are completely different spec update, better to update in 2 enhancements.
As you have concerns here, I removed ObjectIdentifier.java from this update. See the new webrev:
http://cr.openjdk.java.net/~xuelei/8152237/webrev.01/
Xuelei
--Max
在 2016年3月23日，17:06，Xuelei Fan <xuelei.fan@oracle.com <mailto:xuelei.fan@oracle.com>> 写道：
On 3/23/2016 3:34 PM, Wang Weijun wrote:
On Mar 23, 2016, at 12:48 PM, Xuelei Fan <Xuelei.Fan@Oracle.COM <mailto:Xuelei.Fan@oracle.com>> wrote:
On 3/23/2016 12:10 PM, Wang Weijun wrote: > Only 3 files touched. Are you going to make the > s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files > with another bug fix? There are also uses in security components. I will make the update in another bug.
I see. But why is ObjectIdentifier.java included in this fix? It happens that the other bug touch those files, but ObjectIdentifier.java is not related to that bug.
Does it make sense?
Thanks, Xuelei
In you only keep BigInteger and BigDecimal, then I have no other comment.
Thanks Max
Thanks, Xuelei
> Thanks > Max > >> On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei.fan@oracle.com >> <mailto:xuelei.fan@oracle.com>> wrote: >> >> Hi, >> >> Please review the update for the supporting of BigInteger.TWO: >> >> http://cr.openjdk.java.net/~xuelei/8152237/webrev/ >> >> BigInteger.valueOf(2) is a common BigInteger value used in >> binary and cryptography operation calculation. The >> BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) >> is used instead in applications and JDK components. The export >> of static BigInteger.TWO can improve performance and simplify >> existing code. >> >> Thanks, >> Xuelei
On Mar 23, 2016, at 7:23 PM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
On 3/23/2016 5:44 PM, Wang Weijun wrote:
Then why not fix the 2 bugs in a single changeset?
Both need spec update approval. As they are completely different spec update, better to update in 2 enhancements.
As you have concerns here, I removed ObjectIdentifier.java from this update. See the new webrev:
Looks good to me. --Max
Xuelei
Thanks! Xuelei On 3/23/2016 9:44 PM, Wang Weijun wrote:
On Mar 23, 2016, at 7:23 PM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
On 3/23/2016 5:44 PM, Wang Weijun wrote:
Then why not fix the 2 bugs in a single changeset?
Both need spec update approval. As they are completely different spec update, better to update in 2 enhancements.
As you have concerns here, I removed ObjectIdentifier.java from this update. See the new webrev:
Looks good to me.
--Max
Xuelei
Hi, How much is the performance improvement? I see that BigInteger.valueOf (n <= 16) have predefined constants and returns an existing instance. Adding TWO is ok. Roger On 3/23/2016 9:44 AM, Wang Weijun wrote:
On Mar 23, 2016, at 7:23 PM, Xuelei Fan <xuelei.fan@oracle.com> wrote:
On 3/23/2016 5:44 PM, Wang Weijun wrote:
Then why not fix the 2 bugs in a single changeset?
Both need spec update approval. As they are completely different spec update, better to update in 2 enhancements.
As you have concerns here, I removed ObjectIdentifier.java from this update. See the new webrev:
http://cr.openjdk.java.net/~xuelei/8152237/webrev.01/ Looks good to me.
--Max
Xuelei
Hi Xuelei, On 03/23/2016 04:26 AM, Xuelei Fan wrote:
Hi,
Please review the update for the supporting of BigInteger.TWO:
http://cr.openjdk.java.net/~xuelei/8152237/webrev/
BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code.
Thanks, Xuelei
I think (haven't tried, just speculate) you could achieve the same performance by: - adding final qualifier to static BigInteger.[posConst|negConst] fields - annotating those fields with @jdk.internal.vm.annotation.Stable annotation This way BigInteger.valueOf(-MAX_CONSTANT <= i <= MAX_CONSTANT) when called with a constant argument should fold into a constant when compiled by JIT. The same optimization could be performed for valueOf methods of java.lang.Byte, Character, Short, Integer & Long. @Stable annotation was a package-private annotation in java.lang.invoke, reserved for method handles infrastructure, but has since been made public and moved to a concealed package of java.base. There is already a precedent for its use outside in java.lang.invoke: in java.lang.String. For example: static final String s = "....."; s.charAt(0); // is folded into a constant by JIT Regards, Peter
Ok, here's a test... with just the following change: diff -r 9ea9fb3c0c88 src/java.base/share/classes/java/math/BigInteger.java --- a/src/java.base/share/classes/java/math/BigInteger.java Wed Mar 23 18:24:35 2016 +0100 +++ b/src/java.base/share/classes/java/math/BigInteger.java Wed Mar 23 19:55:01 2016 +0100 @@ -41,6 +41,7 @@ import jdk.internal.math.DoubleConsts; import jdk.internal.math.FloatConsts; import jdk.internal.HotSpotIntrinsicCandidate; +import jdk.internal.vm.annotation.Stable; /** * Immutable arbitrary-precision integers. All operations behave as if @@ -1213,8 +1214,10 @@ * Initialize static constant array when class is loaded. */ private static final int MAX_CONSTANT = 16; - private static BigInteger posConst[] = new BigInteger[MAX_CONSTANT+1]; - private static BigInteger negConst[] = new BigInteger[MAX_CONSTANT+1]; + @Stable + private static final BigInteger posConst[] = new BigInteger[MAX_CONSTANT+1]; + @Stable + private static final BigInteger negConst[] = new BigInteger[MAX_CONSTANT+1]; /** * The cache of powers of each radix. This allows us to not have to The results of simple benchmark: /* Original: Benchmark Mode Cnt Score Error Units BigIntegerBench.ONE avgt 10 2.396 ± 0.232 ns/op BigIntegerBench.valueOf_1 avgt 10 2.846 ± 0.233 ns/op BigIntegerBench.valueOf_2 avgt 10 2.808 ± 0.054 ns/op Patched: Benchmark Mode Cnt Score Error Units BigIntegerBench.ONE avgt 10 2.381 ± 0.126 ns/op BigIntegerBench.valueOf_1 avgt 10 2.347 ± 0.089 ns/op BigIntegerBench.valueOf_2 avgt 10 2.323 ± 0.022 ns/op */ package jdk.test; import org.openjdk.jmh.annotations.*; import java.math.BigInteger; import java.util.concurrent.TimeUnit; @BenchmarkMode(Mode.AverageTime) @Fork(1) @Warmup(iterations = 5) @Measurement(iterations = 10) @OutputTimeUnit(TimeUnit.NANOSECONDS) public class BigIntegerBench { @Benchmark public BigInteger ONE() { return BigInteger.ONE; } @Benchmark public BigInteger valueOf_1() { return BigInteger.valueOf(1); } @Benchmark public BigInteger valueOf_2() { return BigInteger.valueOf(2); } } So, no need to change the API and all uses of valueOf(-MAX_CONSTANT <= i <= MAX_CONSTANT) for constant 'i' will be faster a bit. Regards, Peter On 03/23/2016 07:01 PM, Peter Levart wrote:
Hi Xuelei,
On 03/23/2016 04:26 AM, Xuelei Fan wrote:
Hi,
Please review the update for the supporting of BigInteger.TWO:
http://cr.openjdk.java.net/~xuelei/8152237/webrev/
BigInteger.valueOf(2) is a common BigInteger value used in binary and cryptography operation calculation. The BigInteger.TWO is not exported, and hence BigInteger.valueOf(2) is used instead in applications and JDK components. The export of static BigInteger.TWO can improve performance and simplify existing code.
Thanks, Xuelei
I think (haven't tried, just speculate) you could achieve the same performance by:
- adding final qualifier to static BigInteger.[posConst|negConst] fields - annotating those fields with @jdk.internal.vm.annotation.Stable annotation
This way BigInteger.valueOf(-MAX_CONSTANT <= i <= MAX_CONSTANT) when called with a constant argument should fold into a constant when compiled by JIT.
The same optimization could be performed for valueOf methods of java.lang.Byte, Character, Short, Integer & Long.
@Stable annotation was a package-private annotation in java.lang.invoke, reserved for method handles infrastructure, but has since been made public and moved to a concealed package of java.base. There is already a precedent for its use outside in java.lang.invoke: in java.lang.String. For example:
static final String s = ".....";
s.charAt(0); // is folded into a constant by JIT
Regards, Peter
participants (5)
-
Attila Szegedi
-
Peter Levart
-
Roger Riggs
-
Wang Weijun
-
Xuelei Fan