Derby
  1. Derby
  2. DERBY-2447

ejbql and floattypes in org.apache.derbyTesting.functionTests.tests.lang.LangScripts intermittently fails with 'expected:<[0.0 ] > but was:<[-0.0] '

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.5.2.0, 10.6.1.0
    • Component/s: SQL
    • Labels:
      None
    • Environment:
    • Bug behavior facts:
      Regression Test Failure

      Description

      Seen intermittently on Linux since 2007-02-27.
      Seen on Solaris 2007-03-12.

      <signature>
      ejbql(org.apache.derbyTesting.functionTests.tests.lang.LangScripts)junit.framework.ComparisonFailure: Output at line 454 expected:<[0.0 ] > but was:<[-0.0] >
      at org.apache.derbyTesting.functionTests.util.CanonTestCase.compareCanon(CanonTestCase.java:100)
      at org.apache.derbyTesting.functionTests.util.ScriptTestCase.runTest(ScriptTestCase.java:124)
      at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:80)
      at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
      at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
      at junit.extensions.TestSetup.run(TestSetup.java:25)
      </signature>

      <signature>
      floattypes(org.apache.derbyTesting.functionTests.tests.lang.LangScripts)junit.framework.ComparisonFailure: Output at line 1823 expected:<[0.0 ] > but was:<[-0.0] >
      at org.apache.derbyTesting.functionTests.util.CanonTestCase.compareCanon(CanonTestCase.java:100)
      at org.apache.derbyTesting.functionTests.util.ScriptTestCase.runTest(ScriptTestCase.java:124)
      at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:80)
      at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
      at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
      at junit.extensions.TestSetup.run(TestSetup.java:25)
      </signature>

      1. d2447.diff
        6 kB
        Knut Anders Hatlen
      2. negative-zero.diff
        2 kB
        Knut Anders Hatlen
      3. Normalize.java
        0.5 kB
        Knut Anders Hatlen

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Bryan. I committed the patch to trunk with revision 772449. I also merged it to 10.5 and committed revision 772452, so that we'll get less noise in the release testing when we create a 10.5 maintenance release.

        Show
        Knut Anders Hatlen added a comment - Thanks, Bryan. I committed the patch to trunk with revision 772449. I also merged it to 10.5 and committed revision 772452, so that we'll get less noise in the release testing when we create a 10.5 maintenance release.
        Hide
        Bryan Pendleton added a comment -

        Thanks for adding the test, Knut. It looks good to me. I can't think of anything else needed here. +1.

        Show
        Bryan Pendleton added a comment - Thanks for adding the test, Knut. It looks good to me. I can't think of anything else needed here. +1.
        Hide
        Knut Anders Hatlen added a comment -

        Here's a new patch which includes test cases that expose the bug. Three methods are affected, but only two of them are tested. The untested method is only called from CastNode when a constant double value is cast to a float, in which case the constant double has already been normalized so a negative zero will never be seen there. All three methods were updated for consistency.

        I haven't run the full regression test suite yet.

        Show
        Knut Anders Hatlen added a comment - Here's a new patch which includes test cases that expose the bug. Three methods are affected, but only two of them are tested. The untested method is only called from CastNode when a constant double value is cast to a float, in which case the constant double has already been normalized so a negative zero will never be seen there. All three methods were updated for consistency. I haven't run the full regression test suite yet.
        Hide
        Knut Anders Hatlen added a comment -

        It looks like a fix for this has been committed in the OpenJDK repository now: http://hg.openjdk.java.net/jdk7/hotspot-comp/hotspot/rev/36ee9b69616e

        Show
        Knut Anders Hatlen added a comment - It looks like a fix for this has been committed in the OpenJDK repository now: http://hg.openjdk.java.net/jdk7/hotspot-comp/hotspot/rev/36ee9b69616e
        Hide
        Bryan Pendleton added a comment -

        I think the test would be useful. Even though it's not our bug, it's behavior that we depend on, so having a test which verifies the expected behavior seems helpful.

        Thanks for taking care of this in a thorough fashion. Much appreciated!

        Also, I see that the JVM bug report is now visible and has been marked "accepted" by Sun.

        Show
        Bryan Pendleton added a comment - I think the test would be useful. Even though it's not our bug, it's behavior that we depend on, so having a test which verifies the expected behavior seems helpful. Thanks for taking care of this in a thorough fashion. Much appreciated! Also, I see that the JVM bug report is now visible and has been marked "accepted" by Sun.
        Hide
        Knut Anders Hatlen added a comment -

        Good point about adding a comment about the JVM bug, Bryan. And the more I think about it, the more I think your suggestion about just changing (v == 0.0f) to (v == -0.0f) is better (simpler and just as clear) than the extra helper method. So I'll post a new patch.

        As to testing, it should be easy enough to write a test that fails reliably on a JVM with this bug. For instance, we could prepare the statement "VALUES -CAST(? AS REAL)", set the parameter to 0 and execute it 10000 times. After about 5000-6000 executions it should stop returning 0.0 and start returning -0.0. I can add a test case doing that in the next patch (unless someone thinks it's a bad idea to add a regression test for something that's arguably a JVM bug).

        Changing issue type from bug to improvement, since this is not a Derby bug and we just add a workaround that happens to make the intention of the code clearer.

        Show
        Knut Anders Hatlen added a comment - Good point about adding a comment about the JVM bug, Bryan. And the more I think about it, the more I think your suggestion about just changing (v == 0.0f) to (v == -0.0f) is better (simpler and just as clear) than the extra helper method. So I'll post a new patch. As to testing, it should be easy enough to write a test that fails reliably on a JVM with this bug. For instance, we could prepare the statement "VALUES -CAST(? AS REAL)", set the parameter to 0 and execute it 10000 times. After about 5000-6000 executions it should stop returning 0.0 and start returning -0.0. I can add a test case doing that in the next patch (unless someone thinks it's a bad idea to add a regression test for something that's arguably a JVM bug). Changing issue type from bug to improvement, since this is not a Derby bug and we just add a workaround that happens to make the intention of the code clearer.
        Hide
        Bryan Pendleton added a comment -

        The fix seems fine to me. Given that we believe that the current code is actually
        fine, and that this change is actually working around a bug in the JVM, it seems
        like it would be nice to have a comment to that effect.

        Do we feel that the existing regression test suite covers this area adequately?

        Show
        Bryan Pendleton added a comment - The fix seems fine to me. Given that we believe that the current code is actually fine, and that this change is actually working around a bug in the JVM, it seems like it would be nice to have a comment to that effect. Do we feel that the existing regression test suite covers this area adequately?
        Hide
        Knut Anders Hatlen added a comment -

        I've logged a bug against Sun's JVM: http://bugs.sun.com/view_bug.do?bug_id=6833879
        (The bug report is not visible yet.)

        Show
        Knut Anders Hatlen added a comment - I've logged a bug against Sun's JVM: http://bugs.sun.com/view_bug.do?bug_id=6833879 (The bug report is not visible yet.)
        Hide
        Knut Anders Hatlen added a comment -

        Derbyall and suites.All ran cleanly. The patch is ready for review.

        Show
        Knut Anders Hatlen added a comment - Derbyall and suites.All ran cleanly. The patch is ready for review.
        Hide
        Knut Anders Hatlen added a comment -

        Here's a patch using the helper method approach. No tests have been run yet.

        Show
        Knut Anders Hatlen added a comment - Here's a patch using the helper method approach. No tests have been run yet.
        Hide
        Knut Anders Hatlen added a comment -

        Another alternative is to write a helper method which inspects the integer representation of the floating point value

        private static boolean isNegativeZero(double d)

        { // If the most significant bit (sign bit) is 1 and the rest are 0, // we have a negative zero. return Double.doubleToRawLongBits(d) == 0x8000000000000000L; }

        and

        if (isNegativeZero(v))

        { v = 0.0f; }

        This has the additional advantage of only matching values that actually are negative zero.

        Both approaches (comparing to -0.0f and using the helper method) appear to make the test program print 0.0 consistently.

        I don't find anything in http://java.sun.com/docs/books/jls/third_edition/html/typesValues.html#4.2.3 indicating that there is something wrong with the current implementation of normalizeREAL() and normalizeDOUBLE(), so I'll file a bug against the JVM, at least to get a verification of whether or not it is a bug.

        JVM bug or not, both of the suggested workarounds will make the code clearer, so I guess changing the Derby code wouldn't make any harm.

        Show
        Knut Anders Hatlen added a comment - Another alternative is to write a helper method which inspects the integer representation of the floating point value private static boolean isNegativeZero(double d) { // If the most significant bit (sign bit) is 1 and the rest are 0, // we have a negative zero. return Double.doubleToRawLongBits(d) == 0x8000000000000000L; } and if (isNegativeZero(v)) { v = 0.0f; } This has the additional advantage of only matching values that actually are negative zero. Both approaches (comparing to -0.0f and using the helper method) appear to make the test program print 0.0 consistently. I don't find anything in http://java.sun.com/docs/books/jls/third_edition/html/typesValues.html#4.2.3 indicating that there is something wrong with the current implementation of normalizeREAL() and normalizeDOUBLE(), so I'll file a bug against the JVM, at least to get a verification of whether or not it is a bug. JVM bug or not, both of the suggested workarounds will make the code clearer, so I guess changing the Derby code wouldn't make any harm.
        Hide
        Bryan Pendleton added a comment -

        What about writing that line as:

        if (v == -0.0f) v = 0.0f;

        If that works, it might also have the desirable effect of making the intent of the code clearer.

        Show
        Bryan Pendleton added a comment - What about writing that line as: if (v == -0.0f) v = 0.0f; If that works, it might also have the desirable effect of making the intent of the code clearer.
        Hide
        Knut Anders Hatlen added a comment -

        Yes, that's the correct way to run it.

        The first line of output is not significant, it's just printed so that the run-time compiler is not allowed to optimize away the loop.

        The second line prints either 0.0 or -0.0. I think the correct result is 0.0. However, when I run with the server VM I see -0.0 most of the time, but not always. I have only seen 0.0 printed with the client VM, probably because the client VM is not that eager to generate native code.

        You could try "java -server Normalize" to force the use of the server VM.

        Show
        Knut Anders Hatlen added a comment - Yes, that's the correct way to run it. The first line of output is not significant, it's just printed so that the run-time compiler is not allowed to optimize away the loop. The second line prints either 0.0 or -0.0. I think the correct result is 0.0. However, when I run with the server VM I see -0.0 most of the time, but not always. I have only seen 0.0 printed with the client VM, probably because the client VM is not that eager to generate native code. You could try "java -server Normalize" to force the use of the server VM.
        Hide
        Bryan Pendleton added a comment -

        Hi Knut, this is an interesting observation, and an interesting little snippet of code.
        What might I see when I run it? Am I doing it right:

        -bash-2.05b$ javac Normalize.java
        -bash-2.05b$ java -cp . Normalize
        sum: 0.0
        normalizeREAL(-0.0f): 0.0

        I got these results with both of the following:

        java version "1.5.0_05"
        Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_05-b05)
        Java HotSpot(TM) Client VM (build 1.5.0_05-b05, mixed mode, sharing)

        java version "1.4.2_13"
        Java(TM) 2 Runtime Environment, Standard Edition (build 1.4.2_13-b06)
        Java HotSpot(TM) Client VM (build 1.4.2_13-b06, mixed mode)

        Show
        Bryan Pendleton added a comment - Hi Knut, this is an interesting observation, and an interesting little snippet of code. What might I see when I run it? Am I doing it right: -bash-2.05b$ javac Normalize.java -bash-2.05b$ java -cp . Normalize sum: 0.0 normalizeREAL(-0.0f): 0.0 I got these results with both of the following: java version "1.5.0_05" Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_05-b05) Java HotSpot(TM) Client VM (build 1.5.0_05-b05, mixed mode, sharing) java version "1.4.2_13" Java(TM) 2 Runtime Environment, Standard Edition (build 1.4.2_13-b06) Java HotSpot(TM) Client VM (build 1.4.2_13-b06, mixed mode)
        Hide
        Knut Anders Hatlen added a comment -

        I think this instability is caused by the JVM. At least if I run the attached Java class, which imitates the normalizeREAL() method in NumberDataType, the result of normalizeREAL(-0.0f) varies.

        I've tested it on OpenSolaris with the following JVM:

        java version "1.6.0_13"
        Java(TM) SE Runtime Environment (build 1.6.0_13-b03)
        Java HotSpot(TM) Server VM (build 11.3-b02, mixed mode)

        It looks like this line in normalizeREAL() is optimized away when the run-time compiler translates the byte code to native code (haven't actually verified that, though):

        if (v == 0.0f) v = 0.0f;

        This line is the one that's removing the sign for -0.0f, but I'll need to check if the language specification actually guarantees that it should work.

        Show
        Knut Anders Hatlen added a comment - I think this instability is caused by the JVM. At least if I run the attached Java class, which imitates the normalizeREAL() method in NumberDataType, the result of normalizeREAL(-0.0f) varies. I've tested it on OpenSolaris with the following JVM: java version "1.6.0_13" Java(TM) SE Runtime Environment (build 1.6.0_13-b03) Java HotSpot(TM) Server VM (build 11.3-b02, mixed mode) It looks like this line in normalizeREAL() is optimized away when the run-time compiler translates the byte code to native code (haven't actually verified that, though): if (v == 0.0f) v = 0.0f; This line is the one that's removing the sign for -0.0f, but I'll need to check if the language specification actually guarantees that it should work.
        Hide
        Kathey Marsden added a comment -

        Unchecking Regession checkbox as this is not a confirmed product regression

        Show
        Kathey Marsden added a comment - Unchecking Regession checkbox as this is not a confirmed product regression

          People

          • Assignee:
            Knut Anders Hatlen
            Reporter:
            Ole Solberg
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development