On Mon, 13 Feb 2023 17:53:46 GMT, Andriy Plokhotnyuk <d...@openjdk.org> wrote:
>> This PR is waiting for a review > > @rgiulietti Thanks for keeping making JDK faster! > > I have a couple of review comments: > > #### 1) For the line > https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3746 > > It seems that removing this check for 22-bit number introduces an extra > rounding error. > > Please consider just increasing the constant up to `1L << 32` and using > `DOUBLE_10_POW` instead of `FLOAT_10_POW`. > > Bellow are REPL expressions that illustrate my thoughts (sorry for using > Scala): > > $ scala > Welcome to Scala 3.2.0 (17.0.5, Java OpenJDK 64-Bit Server VM). > Type in expressions for evaluation. Or try :help. > > > > scala> new java.math.BigDecimal("167772170E-1").floatValue > val res0: Float = 1.6777216E7 > > > > scala> 167772170E-1f > val res1: Float = 1.6777216E7 > > > > scala> 167772170 / 10.0f > val res2: Float = 1.6777218E7 > > scala> (167772170 / 10.0).toFloat > val res3: Float = 1.6777216E7 > > #### 2) For the line: > https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3791 > > It seems that removing this check for 52-bit number introduces an extra > rounding error: > > $ scala > Welcome to Scala 3.2.0 (17.0.6, Java OpenJDK 64-Bit Server VM). > Type in expressions for evaluation. Or try :help. > > scala> new java.math.BigDecimal("90071992547409930E-1").doubleValue > val res0: Double = 9.007199254740992E15 > > > > scala> 90071992547409930E-1 > val res1: Double = 9.007199254740992E15 > > > > scala> 90071992547409930L / 10.0 > val res2: Double = 9.007199254740994E15 > > #### 3) For the line: > https://github.com/openjdk/jdk/pull/9410/files#diff-94d400b99466045dd76001c37eada6b24d086d8d115b49c439752bbceb233772L3800 > Here before falling back to the costly fallback with big number operations we > can [get a number of digits in > intCompact](https://github.com/plokhotnyuk/jsoniter-scala/blob/6f702ce5cae05df91b5aa6e4bd61acdf43bf18f6/jsoniter-scala-core/jvm/src/main/scala/com/github/plokhotnyuk/jsoniter_scala/core/JsonWriter.scala#L1930) > and use [a trick with two > multiplications](https://github.com/plokhotnyuk/jsoniter-scala/blob/6f702ce5cae05df91b5aa6e4bd61acdf43bf18f6/jsoniter-scala-core/jvm/src/main/scala/com/github/plokhotnyuk/jsoniter_scala/core/JsonReader.scala#L1459) @plokhotnyuk The expression `new java.math.BigDecimal("90071992547409930E-1").doubleValue()` would be processed by the proposed code as follows. We have: `intCompact = 90071992547409930L` `scale = 1` L.3838 sets `v == 9.007199254740994E16` The test `(long) v == intCompact` on L. 3848 fails, so the fast path is not executed. The slow path then ensures that the conversion is affected by at most one rounding error. On the other hand, the expression `scala> 90071992547409930L / 10.0` is subject to two rounding errors, but it i executed neither by the current nor by the proposed code. A similar analysis holds for the `float` example. As for the last paragraph, I'll have to have a deeper look. Stay tuned ;-) ------------- PR: https://git.openjdk.org/jdk/pull/9410
