Use Double.doubleToRawLongBits() to compare a double with -0.0

[simonis]

In PowNode::pow() there are optimizations for specific input values which map pow() to the cheaper sqrt() function. But for some double inputs like -0.0 this mapping is not valid and has to be excluded.

However, we can not simply compare a double value d with -0.0 because the JLS (and the IEEE 754 standard) require that positive and negative zeros are considered equal and therefor 0.0 == -0.0.

In order to really check for a negative zero we have to use Double.doubleToRawLongBits().

[iamstolis]

I am not sure that this PR fixes anything. Yes, the check a == -0.0 is satisfied for both 0 and -0 but it is not important which branch is used for 0 as both possible branches provide the correct result for 0.

[simonis]

I am not sure that this PR fixes anything. Yes, the check a == -0.0 is satisfied for both 0 and -0 but it is not important which branch is used for 0 as both possible branches provide the correct result for 0.

For Java the result of using Math.pow() or Math.sqrt() for negative zero is not the same because:

• Math.sqrt(-0.0) == -0.0 (see Math.pow() API spec: "If the argument is positive zero or negative zero, then the result is the same as the argument."
• Math.pow(-0.0, 0.5) == 0.0 (see [Math.pow() API spec](): "If the first argument is negative zero and the second argument is greater than zero but not a finite odd integer, or .. then the result is positive zero.")

From what I found, the ECMA-Script Specification mandates the same behavior for JavaScript's Math.pow() like the one implemented in Java's Math.pow() so you can't substitute it for -0.0. I think that is why we have this special handling in the code in the first place (it just has to be done correctly :).

Without this fix, GraalJS will not optimize Math.pow(base, exponent) for exponent from {0.5, 1,5, 2.5} if it ever sees base == 0.0 || base == -0.0. But it actually could, as long as base != -0.0. So this is a performance rather than a correctness fix.

[iamstolis]

... so you can't substitute it for -0.0 ...

I didn't write that -0 can go into both branches. My comment was about (positive) 0, which is the only value of a that your PR affects.

Without this fix, GraalJS will not optimize Math.pow(base, exponent) for exponent from {0.5, 1,5, 2.5} if it ever sees base == 0.0 || base == -0.0.

That's not true. graal-js will continue to invoke Math.sqrt-based versions for positive values.

So this is a performance rather than a correctness fix.

The only performance improvement that I see is that Math.pow(0, 0.5/1.5/2.5) will go into the faster branch but this feels like a negligible difference for any realistic workload.

Don't get me wrong, I appreciate your interest in graal-js and your effort to improve it. I am just trying to assess realistically the impact of the PR. I agree that whoever wrote (a < 0.0 || a == -0.0) condition probably overlooked that it covers 0 as well. It would be simpler to use a <= 0 otherwise.

[simonis]

OK, I admit that I'm still far from being proficient in Truffle/GraalJS. I'm currently trying to better understand how specializations and substitutions work.I wondered why the following trivial test (attached below) is two times slower if it includes zero as argument for the JS call and found the reason is the incorrect comparison with -0.0.

I think the intention of the original code was to only exclude calls with a base value of -0.0 from being optimized and my patch fixes exactly taht case. I don't pretend that this fix will save the world or will have any significant overall performance impact. But it fixes code which is hard to understand, was obviously not correctly understood by the initial author and is not doing what the initial author intended.

We can of course also fix it by replacing (a < 0.0 || a == -0.0) with (a <= 0) and thus sacrificing the optimizations for normal (i.e. postive) 0.0 values. But why should we do that if the fix I proposed is just as simple while still optimizing the positive zero case?

I don't understand why we have to discuss such a trivial and evident code cleanup to death? Is there any compelling reason for not making this code more readable and performant at the same time without any (at least from my understanding) drawback?

import org.graalvm.polyglot.Context;
import org.graalvm.polyglot.Engine;
import org.graalvm.polyglot.PolyglotException;
import org.graalvm.polyglot.Source;
import org.graalvm.polyglot.Value;

public class GraalJSPowTest {

  public static void main(String[] args) throws Exception {

    Engine engine = Engine.newBuilder("js")
      // Needed for -Dpolyglot.engine.TraceCompilation
      .allowExperimentalOptions(true)
      .build();

    try (Context context = Context.newBuilder()
         .engine(engine)
         .build()) {

      String js = """
                  function test_pow(x) {
                    // `Math.pow(x, 2.5)` will be optimized to `x * x * Math.sqrt(x)`
                    // in com.oracle.truffle.js.builtins.math.PowNode::pow()
                    return Math.pow(x, 2.5);
                  }
                  function main(arg) {
                    let res = 0;
                    for (let i = 0; i < 100000; i++) {
                      res += test_pow(arg);
                    }
                    return res;
                  }
                  """;
      Source source = Source.newBuilder("js", js, "test.js").build();

      context.eval(source);
      Value jsBindings = context.getBindings("js");
      Value main = jsBindings.getMember("main");
      assert main.canExecute();
      int iterations = Integer.getInteger("iterations", 1);
      int base = Integer.getInteger("base", 0);
      for (int j = 0; j < iterations; j++) {
        main.execute((j % 5) + base); // See https://github.com/oracle/graaljs/pull/868
      }
    }
  }
}

Running this code with -Dbase=0 is two times slower than running it with -Dbase=1. In general, Math.pow(0.0, 2.5) is ~four times slower without this patch because 0.0 is treated the same as -0.0 and not optimized.

[iamstolis]

I don't understand why we have to discuss such a trivial and evident code cleanup to death?

That's a pretty strong statement after just 2 comments from the reviewer. Anyway, if you are still alive reading this comment then I have good news for you. It seems that we are on the same page that this PR:

• has no impact on correctness
• has some performance impact on workloads that invoke Math.pow(0, 0.5/1.5/2.5) a lot
• is a code cleanup of a suspicious comparison

Regarding the 2nd point, it is unclear how often such workloads occur in practice, but the fact that you run in this situation shows that they may not be as rare as I would expect otherwise. Moreover, the 3rd point is itself good enough to accept your contribution.

Could you, please, replace Double.doubleToRawLongBits(a) == Double.doubleToRawLongBits(-0.0) by JSRuntime.isNegativeZero(a) and squash the commits? I will merge the PR then. Thank you in advance.

[simonis]

@iamstolis, thanks for your support (and sorry if my previous comment was too harsh).

I've updated and squashed the commits as requested.