Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
Hi, the aim of this PR was firstly to have a correct implementation of the spec. Speed improvement was a secondary goal, which was intentionally pursued with conventional Java arithmetic and just a few calls to simple methods in the standard library. Whether the Vector API (which is still in incubation phase) might help in this specific case remains to be explored. As a first impression, there seem to be no good spots to parallelize using SIMD instructions. But engineers with more SIMD experience might find creative ways to make good use of them. HTH Raffaello On 2022-06-07 18:35, LifeIsStrange wrote: On Mon, 9 May 2022 12:50:31 GMT, Raffaello Giulietti wrote: Marked as reviewed by limck...@github.com (no known OpenJDK username). @limck599 While we at OpenJDK appreciate constructive reviews from GitHub users not registered in the [census](https://openjdk.java.net/census), only officially nominated reviewers have the authority to approve this PR. @rgiulietti friendly ping, noob question: Do you believe some parts of the algorithm could exploit explicit SIMD instructions (via the high level [Vector API](https://openjdk.java.net/jeps/417)) for even better performance (such as https://github.com/wrandelshofer/FastDoubleParser ) ? - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Mon, 9 May 2022 12:50:31 GMT, Raffaello Giulietti wrote: >> Marked as reviewed by limck...@github.com (no known OpenJDK username). > > @limck599 > While we at OpenJDK appreciate constructive reviews from GitHub users not > registered in the [census](https://openjdk.java.net/census), only officially > nominated reviewers have the authority to approve this PR. @rgiulietti friendly ping, noob question: Do you believe some parts of the algorithm could exploit explicit SIMD instructions (via the high level [Vector API](https://openjdk.java.net/jeps/417)) for even better performance (such as https://github.com/wrandelshofer/FastDoubleParser ) ? - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Tue, 10 May 2022 03:22:01 GMT, Joe Darcy wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 4511638: Double.toString(double) sometimes produces incorrect results > > src/java.base/share/classes/java/lang/Double.java line 33: > >> 31: import java.util.Optional; >> 32: >> 33: import jdk.internal.math.FloatingDecimal; > > Presumably the FloatingDecimal import here and in Float can be removed. `FloatingDecimal` is used in the `parse*(String)` methods - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Tue, 10 May 2022 15:50:26 GMT, Raffaello Giulietti wrote: > `[Float|Double]ToDecimal` do not have access to `AbstractStringBuilder`, so > have to fail over `Appendable`, which can throw `IOException` in `append(*)` > methods. > > I have to find another way if this wrapping to make the compiler happy is > unacceptable. Ah, I haven't used it myself recently enough to recall the details, but I believe the shared secrets mechanism is intended to allow such cross-package access. If the current code is kept and if the exception can actually be thrown, explicitly throwing an assertion error is preferable to "assert false". - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Tue, 10 May 2022 03:21:21 GMT, Joe Darcy wrote:
>> Raffaello Giulietti has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 4511638: Double.toString(double) sometimes produces incorrect results
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 882:
>
>> 880: try {
>> 881: FloatToDecimal.appendTo(f, this);
>> 882: } catch (IOException ignored) {
>
> What is the motivation for wrapping with IOException?
`[Float|Double]ToDecimal` do not have access to `AbstractStringBuilder`, so
have to fail over `Appendable`, which can throw `IOException` in `append(*)`
methods.
I have to find another way if this wrapping to make the compiler happy is
unacceptable.
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Mon, 9 May 2022 12:38:48 GMT, Raffaello Giulietti
wrote:
>> Hello,
>>
>> here's a PR for a patch submitted on March 2020
>> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was
>> a thing.
>>
>> The patch has been edited to adhere to OpenJDK code conventions about
>> multi-line (block) comments. Nothing in the code proper has changed, except
>> for the addition of redundant but clarifying parentheses in some expressions.
>>
>>
>> Greetings
>> Raffaello
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 4511638: Double.toString(double) sometimes produces incorrect results
Generally looks solid!
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 882:
> 880: try {
> 881: FloatToDecimal.appendTo(f, this);
> 882: } catch (IOException ignored) {
What is the motivation for wrapping with IOException?
src/java.base/share/classes/java/lang/Double.java line 33:
> 31: import java.util.Optional;
> 32:
> 33: import jdk.internal.math.FloatingDecimal;
Presumably the FloatingDecimal import here and in Float can be removed.
src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java line 38:
> 36: * This class exposes a method to render a {@code double} as a string.
> 37: *
> 38: * @author Raffaello Giulietti
New code shouldn't use author tags.
-
PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Mon, 9 May 2022 12:34:20 GMT, limck599 wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 4511638: Double.toString(double) sometimes produces incorrect results > > Marked as reviewed by limck...@github.com (no known OpenJDK username). @limck599 While we at OpenJDK appreciate constructive reviews from GitHub users not registered in the [census](https://openjdk.java.net/census), only officially nominated reviewers have the authority to approve this PR. - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
On Mon, 9 May 2022 12:34:20 GMT, Raffaello Giulietti wrote: >> Hello, >> >> here's a PR for a patch submitted on March 2020 >> [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was >> a thing. >> >> The patch has been edited to adhere to OpenJDK code conventions about >> multi-line (block) comments. Nothing in the code proper has changed, except >> for the addition of redundant but clarifying parentheses in some expressions. >> >> >> Greetings >> Raffaello > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 4511638: Double.toString(double) sometimes produces incorrect results Marked as reviewed by limck...@github.com (no known OpenJDK username). - PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v11]
> Hello, > > here's a PR for a patch submitted on March 2020 > [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was a > thing. > > The patch has been edited to adhere to OpenJDK code conventions about > multi-line (block) comments. Nothing in the code proper has changed, except > for the addition of redundant but clarifying parentheses in some expressions. > > > Greetings > Raffaello Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 4511638: Double.toString(double) sometimes produces incorrect results - Changes: - all: https://git.openjdk.java.net/jdk/pull/3402/files - new: https://git.openjdk.java.net/jdk/pull/3402/files/bd323d15..907abfd6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3402&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3402&range=09-10 Stats: 30 lines in 2 files changed: 2 ins; 24 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3402.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3402/head:pull/3402 PR: https://git.openjdk.java.net/jdk/pull/3402
