Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4511638: Double.toString(double) sometimes produces incorrect results #3402

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

@rgiulietti
Copy link
Contributor

@rgiulietti rgiulietti commented Apr 8, 2021

Hello,

here's a PR for a patch submitted on March 2020 1 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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issue

  • JDK-4511638: Double.toString(double) sometimes produces incorrect results

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3402/head:pull/3402
$ git checkout pull/3402

Update a local copy of the PR:
$ git checkout pull/3402
$ git pull https://git.openjdk.java.net/jdk pull/3402/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3402

View PR using the GUI difftool:
$ git pr show -t 3402

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3402.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 8, 2021

👋 Welcome back rgiulietti! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 8, 2021

Forgot to add that other changes in the code are the use of switch expressions and the use of instanceof patterns.

@openjdk openjdk bot added the rfr label Apr 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 8, 2021

@rgiulietti The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs label Apr 8, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 8, 2021

Webrevs

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 9, 2021

Hello,

here's some background information for those that didn't follow the mailing list for the last couple of years.

Some enjoyable properties of the novel algorithm:

  • No intermediate objects are instantiated.
  • Loop-free core algorithm.
  • Only int and long arithmetic.
  • No divisions at all.
  • 17.7x speedup (jmh) 1.
  • Randomized, yet reproducible deep diving tests (jtreg).
  • Clear, unambiguous spec.
  • All floats have been tested to fully meet the spec.
  • Fully documented in 2 and/or in comments.

See 3 for some (invented) Q&A. The last Q&A deals with your investment in time for an informed review.

Greetings
Raffaello

@bplb
Copy link
Member

@bplb bplb commented Apr 9, 2021

@rgiulietti Please issue the /csr/ command here [1]. Speaking of which, does the CSR [2] need to be updated?

[1] https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/csr
[2] https://bugs.openjdk.java.net/browse/JDK-8202555

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 9, 2021

/csr needed

@openjdk openjdk bot added the csr label Apr 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 9, 2021

@rgiulietti this pull request will not be integrated until the CSR request JDK-8202555 for issue JDK-4511638 has been approved.

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 9, 2021

@bplb No, the CSR [1] (https://bugs.openjdk.java.net/browse/JDK-8202555) needs no updates.

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 9, 2021

The langtools/tier1 test tools/javac/sym/ElementStructureTest.java fails on all 4 platforms supported by the automatic GH actions.
Does anybody have a clue? Is this something I should be worried about?

Thanks

@bplb
Copy link
Member

@bplb bplb commented Apr 9, 2021

@bplb No, the CSR [1] (https://bugs.openjdk.java.net/browse/JDK-8202555) needs no updates.

OK, good. I wonder whether it should be moved back to Draft until someone else other than me has reviewed it?

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Apr 15, 2021

Regarding the ElementStructureTest, it prints the API elements (including compile-time constants) computes hash for the printed API and compares it with an expected hash. doubles and floats are printed using String.valueOf, and it apparently changed for Float.MIN_NORMAL from 1.17549435E-38 to 1.1754944E-38 (I assume that is intentional). So regarding ElementStructureTest.java we can just update it. How about this?

diff --git a/test/langtools/tools/javac/sym/ElementStructureTest.java b/test/langtools/tools/javac/sym/ElementStructureTest.java
index 29776ce28c2..d15f2447749 100644
--- a/test/langtools/tools/javac/sym/ElementStructureTest.java
+++ b/test/langtools/tools/javac/sym/ElementStructureTest.java
@@ -121,29 +121,22 @@ import toolbox.ToolBox;
  */
 public class ElementStructureTest {
 
-    static final byte[] hash6 = new byte[] {
-        (byte) 0x99, (byte) 0x34, (byte) 0x82, (byte) 0xCF,
-        (byte) 0xE0, (byte) 0x53, (byte) 0xF3, (byte) 0x13,
-        (byte) 0x4E, (byte) 0xCF, (byte) 0x49, (byte) 0x32,
-        (byte) 0xB7, (byte) 0x52, (byte) 0x0F, (byte) 0x68
-    };
     static final byte[] hash7 = new byte[] {
-        (byte) 0x3C, (byte) 0x03, (byte) 0xEA, (byte) 0x4A,
-        (byte) 0x62, (byte) 0xD2, (byte) 0x18, (byte) 0xE5,
-        (byte) 0xA5, (byte) 0xC2, (byte) 0xB7, (byte) 0x85,
-        (byte) 0x90, (byte) 0xFA, (byte) 0x98, (byte) 0xCD
+        (byte) 0xA7, (byte) 0x3B, (byte) 0x91, (byte) 0xF6,
+        (byte) 0xEF, (byte) 0x99, (byte) 0x07, (byte) 0xF2,
+        (byte) 0x79, (byte) 0xAB, (byte) 0x19, (byte) 0xF4,
+        (byte) 0x59, (byte) 0x44, (byte) 0xF7, (byte) 0x65
     };
     static final byte[] hash8 = new byte[] {
-        (byte) 0x24, (byte) 0x38, (byte) 0x52, (byte) 0x1C,
-        (byte) 0x5E, (byte) 0x83, (byte) 0x82, (byte) 0xE6,
-        (byte) 0x41, (byte) 0xC2, (byte) 0xDD, (byte) 0x2A,
-        (byte) 0xFD, (byte) 0xFF, (byte) 0x5E, (byte) 0x2F
+        (byte) 0xF3, (byte) 0x93, (byte) 0xCA, (byte) 0x53,
+        (byte) 0xFD, (byte) 0xA3, (byte) 0x5D, (byte) 0x57,
+        (byte) 0xD2, (byte) 0xED, (byte) 0x39, (byte) 0xC5,
+        (byte) 0x56, (byte) 0x62, (byte) 0xE0, (byte) 0x1F
     };
 
     final static Map<String, byte[]> version2Hash = new HashMap<>();
 
     static {
-        version2Hash.put("6", hash6);
         version2Hash.put("7", hash7);
         version2Hash.put("8", hash8);
     }
@@ -484,7 +477,7 @@ public class ElementStructureTest {
                 return null;
             try {
                 analyzeElement(e);
-                out.write(String.valueOf(e.getConstantValue()));
+                writeConstant(e.getConstantValue());
                 out.write("\n");
             } catch (IOException ex) {
                 ex.printStackTrace();
@@ -514,6 +507,16 @@ public class ElementStructureTest {
             throw new IllegalStateException("Should not get here.");
         }
 
+        private void writeConstant(Object value) throws IOException {
+            if (value instanceof Double) {
+                out.write(Long.toString(Double.doubleToRawLongBits((Double) value)));
+            } else if (value instanceof Float) {
+                out.write(Integer.toString(Float.floatToRawIntBits((Float) value)));
+            } else {
+                out.write(String.valueOf(value));
+            }
+        }
+
     }
 
     final class TestFileManager implements JavaFileManager {
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 15, 2021

Mailing list message from Joe Darcy on core-libs-dev:

Hi Jan,

I recommend using {Double, Float}.toHexString to get a straightforward
textual form of the floating-point values. The hex string is isomorphic
to the big-level value, but is (more) human readable as a numerical
quantity.

-Joe

On 4/15/2021 10:26 AM, Jan Lahoda wrote:

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 16, 2021

Hi Jan,

I had to change a string in test
test/jdk/java/lang/String/concat/ImplicitStringConcatBoundaries.java
because it failed with the current string but passes with the new one. Indeed, the new implementation of Float.toString(float) produces the new string, which, like the current one, is correct in the sense that, upon reading, it recovers Float.MIN_NORMAL.

However, I didn't change the definition of MIN_NORMAL in java.lang.Float because there it is already expressed in hex notation.

As suggested before and by Joe, using the hex representation instead of the decimal would be more robust because the conversions from/to hex are almost trivial, hence much less subject to slight errors. So, rather than printing the raw bits as you suggest, you could use the hex string rendering instead.

Thanks
Raffaello

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Apr 16, 2021

Sure, here you are:

diff --git a/test/langtools/tools/javac/sym/ElementStructureTest.java b/test/langtools/tools/javac/sym/ElementStructureTest.java
index 29776ce28c2..428ba03361f 100644
--- a/test/langtools/tools/javac/sym/ElementStructureTest.java
+++ b/test/langtools/tools/javac/sym/ElementStructureTest.java
@@ -121,29 +121,22 @@ import toolbox.ToolBox;
  */
 public class ElementStructureTest {
 
-    static final byte[] hash6 = new byte[] {
-        (byte) 0x99, (byte) 0x34, (byte) 0x82, (byte) 0xCF,
-        (byte) 0xE0, (byte) 0x53, (byte) 0xF3, (byte) 0x13,
-        (byte) 0x4E, (byte) 0xCF, (byte) 0x49, (byte) 0x32,
-        (byte) 0xB7, (byte) 0x52, (byte) 0x0F, (byte) 0x68
-    };
     static final byte[] hash7 = new byte[] {
-        (byte) 0x3C, (byte) 0x03, (byte) 0xEA, (byte) 0x4A,
-        (byte) 0x62, (byte) 0xD2, (byte) 0x18, (byte) 0xE5,
-        (byte) 0xA5, (byte) 0xC2, (byte) 0xB7, (byte) 0x85,
-        (byte) 0x90, (byte) 0xFA, (byte) 0x98, (byte) 0xCD
+        (byte) 0x65, (byte) 0x41, (byte) 0xC8, (byte) 0x17,
+        (byte) 0xF0, (byte) 0xB1, (byte) 0x62, (byte) 0x9A,
+        (byte) 0xD8, (byte) 0x19, (byte) 0xBA, (byte) 0xB0,
+        (byte) 0xC1, (byte) 0x70, (byte) 0x5E, (byte) 0x3E
     };
     static final byte[] hash8 = new byte[] {
-        (byte) 0x24, (byte) 0x38, (byte) 0x52, (byte) 0x1C,
-        (byte) 0x5E, (byte) 0x83, (byte) 0x82, (byte) 0xE6,
-        (byte) 0x41, (byte) 0xC2, (byte) 0xDD, (byte) 0x2A,
-        (byte) 0xFD, (byte) 0xFF, (byte) 0x5E, (byte) 0x2F
+        (byte) 0x83, (byte) 0x62, (byte) 0x2F, (byte) 0x1C,
+        (byte) 0x95, (byte) 0x6D, (byte) 0x31, (byte) 0x18,
+        (byte) 0xF5, (byte) 0xB2, (byte) 0x8C, (byte) 0x39,
+        (byte) 0x81, (byte) 0x2E, (byte) 0x2C, (byte) 0x34
     };
 
     final static Map<String, byte[]> version2Hash = new HashMap<>();
 
     static {
-        version2Hash.put("6", hash6);
         version2Hash.put("7", hash7);
         version2Hash.put("8", hash8);
     }
@@ -484,7 +477,7 @@ public class ElementStructureTest {
                 return null;
             try {
                 analyzeElement(e);
-                out.write(String.valueOf(e.getConstantValue()));
+                writeConstant(e.getConstantValue());
                 out.write("\n");
             } catch (IOException ex) {
                 ex.printStackTrace();
@@ -514,6 +507,16 @@ public class ElementStructureTest {
             throw new IllegalStateException("Should not get here.");
         }
 
+        private void writeConstant(Object value) throws IOException {
+            if (value instanceof Double) {
+                out.write(Double.toHexString((Double) value));
+            } else if (value instanceof Float) {
+                out.write(Float.toHexString((Float) value));
+            } else {
+                out.write(String.valueOf(value));
+            }
+        }
+
     }
 
     final class TestFileManager implements JavaFileManager {
@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 16, 2021

Fine, thanks!

Will your change be integrated soon on master?
What am I supposed to do then, rebasing my branch with the updated master?

(BTW, you could make use of instanceof pattern matching if you don't need backward compat ;-) )

@lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Apr 16, 2021

@ rgiulietti, I thought you'd simply add the test change to your patch. But I can start a PR for it, if you prefer.

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 16, 2021

@lahodaj Oh, my understanding was that yours is a permanent change worth of integrating anyway. But yes, I can add your change to my changeset.

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Apr 16, 2021

Modified
test/langtools/tools/javac/sym/ElementStructureTest.java
as suggested by @lahodaj

static final int Q_MAX = (1 << (W - 1)) - P;

/* 10^(E_MIN - 1) <= MIN_VALUE < 10^E_MIN */
static final int E_MIN = -323;

This comment has been minimized.

@turbanoff

turbanoff Apr 18, 2021
Contributor

It seems that E_MIN/E_MAX/K_MIN/K_MAX are not used in production code.
I think it worth to move them to tests.

* -d.ddddddddddddddddE-eee H + 7 characters
* where there are H digits d
*/
public final int MAX_CHARS = H + 7;

This comment has been minimized.

@turbanoff

turbanoff Apr 18, 2021
Contributor

Can it be made static ?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 19, 2021

Mailing list message from Raffaello Giulietti on core-libs-dev:

Hi Andrey,

thanks for your careful reading.

I'll keep a note and collect yours with changes coming from other
reviewers before committing a larger batch of small changes. I would
like to avoid wasting a lot of energy right now just to rebuild
everything and to run the automatic tests on GitHub :-)

Greetings
Raffaello

On 2021-04-18 23:19, Andrey Turbanov wrote:

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 19, 2021

Mailing list message from Raffaello Giulietti on core-libs-dev:

Hi Andrey,

thanks for your careful reading.

I'll keep a note and collect yours with changes coming from other
reviewers before committing a larger batch of small changes. I would
like to avoid wasting a lot of energy right now just to rebuild
everything and to run the automatic tests on GitHub :-)

Greetings
Raffaello

On 2021-04-18 23:19, Andrey Turbanov wrote:

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 17, 2021

@rgiulietti This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented May 25, 2021

Hi,

a reminder to keep PR [1] alive and to invite interested reviewers to
comment it.

The corresponding CSR is in [2]

Greetings
Raffaello


[1] #3402
[2] https://bugs.openjdk.java.net/browse/JDK-8202555

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 22, 2021

@rgiulietti This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@rgiulietti
Copy link
Contributor Author

@rgiulietti rgiulietti commented Jul 16, 2021

This is just to keep [1] alive. No news.

[1] #3402

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants