[PATCH] 4511638: Double.toString(double) sometimes produces incorrect results

[Raffaello Giulietti]

Hi Anthony,

On 16/06/2019 19.17, Anthony Vanelverdinghe wrote:
>
> Hi Raffaello
>
> While I don't have feedback on the actual math, here's a few suggestions:
>
> - there's some use of non-ASCII characters in the patch. I don't think 
> this is common in the JDK's Java sources, so you might want to replace 
> them with their Unicode escapes. The characters are: ≤ (\u2264), ∞ 
> (\u221e), × (\u00d7), ≥ (\u2265), … (\u2026), ≠ (\u2260), ⌊(\u230a), 
> ⌋(\u230b), · (\u00b7), β (\u03b2)
>
I'm not sure what you mean here: there are indeed usages of HTML 
entities like ∞ that appear as math symbols in the rendered 
Javadoc but the sources are 100% US-ASCII in my IDE and should be so 
even in the patch.

If not, I'd be interested in the "coordinates" of the characters outside 
of the US-ASCII set.


> - there are 2 invocations of a deprecated String constructor for 
> performance reasons. I don't know how big the performance difference 
> is, but I would suggest replacing these invocations with `new 
> String(bytes, StandardCharsets.US_ASCII)` instead, and filing a bug 
> for the performance difference with the deprecated constructor
>
The perf diff is measurable. The chosen variant is the fastest that I 
could come up with. As long as the deprecated constructor does not 
become "forRemoval = true", if at all, I wouldn't worry. Even then, 
there's plenty of time to switch.

Until then I can try to understand why the used constructor is faster 
and perhaps file a bug as you suggest.


> - there are 2 occurrences of a typo "left-to-tight"
>
Yep, thanks for your eagle's eyes!


> Other than that, I can only say this is an impressive piece of work, 
> so I hope some official Reviewers will help you add your contribution 
> to the JDK.
>
The review process has indeed begun some days ago. However, since 
there's is quite some maths to digest to understand the code, a full 
review could take 1 to 2 days of concentrated reading: not the average 
review, I guess.

Plus, there's a change in the specification, which must undergo a 
further review.


Thanks for your time!

Greetings
Raffaello



> Kind regards
>
> Anthony
>
> ------------------------------------------------------------------------
> *From:* core-libs-dev <core-libs-dev-bounces at openjdk.java.net> on 
> behalf of Raffaello Giulietti <raffaello.giulietti at gmail.com>
> *Sent:* Monday, May 6, 2019 2:08:48 PM
> *To:* core-libs-dev
> *Subject:* [PATCH] 4511638: Double.toString(double) sometimes produces 
> incorrect results
> Hi,
>
> no new code this time, only a warm invitation to review the rather
> substantial patch submitted on 2019-04-18 [1] and the CSR [2].
>
> I spent an insane amount of (unpaid) free time offering my contribution
> to solve a bug known since about 15 years. Thus, I think it is
> understandable that I'd like to see my efforts come to a successful
> conclusion, ideally for OpenJDK 13.
>
>
> Greetings
> Raffaello
>
>
> P.S.
> Some enjoyable properties of the novel algorithm:
> * No objects are instantiated, except, of course, the resulting String.
> * Loop-free core algorithm.
> * Only int and long arithmetic.
> * No divisions at all.
> * 16x speedup (jmh).
> * Randomized, yet reproducible deep diving tests (jtreg).
> * Clear, unambiguous spec.
> * All floats have been tested to fully meet the spec.
> * Fully documented in [3] or in comments.
>
> ----
>
> [1]
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059783.html
> [2] https://bugs.openjdk.java.net/browse/JDK-8202555
> [3] https://drive.google.com/open?id=1KLtG_LaIbK9ETXI290zqCxvBW94dj058