|
[
Permalink
| « Hide
]
Kathey Marsden added a comment - 11/Sep/07 03:30 PM
Unchecking Regession checkbox as this is not a confirmed product regression
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. 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) 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. 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. 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. Here's a patch using the helper method approach. No tests have been run yet.
Derbyall and suites.All ran cleanly. The patch is ready for review.
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.) 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? 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. 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. 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
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. Thanks for adding the test, Knut. It looks good to me. I can't think of anything else needed here. +1.
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.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Resolved
Minor
ejbql and floattypes in org.apache.derbyTesting.functionTests.tests.lang.LangScripts intermittently fails with 'expected:<[0.0 ] > but was:<[-0.0] '
