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

[Anthony Vanelverdinghe]

My bad. I was looking at the patch at [1], and didn’t consider that the browser rendering likely doesn’t match the actual file content. Sorry for the noise. And good to hear the review process is underway.

Kind regards,
Anthony

[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059783.html

________________________________
From: Raffaello Giulietti <raffaello.giulietti at gmail.com>
Sent: Monday, June 17, 2019 4:56:22 PM
To: Anthony Vanelverdinghe; core-libs-dev
Subject: Re: [PATCH] 4511638: Double.toString(double) sometimes produces incorrect results


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><mailto:core-libs-dev-bounces at openjdk.java.net> on behalf of Raffaello Giulietti <raffaello.giulietti at gmail.com><mailto: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