[webkit-reviews] review granted: [Bug 129486] Implement Math.hypot : [Attachment 225783] Patch v6

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tibor Mészáros
<tmeszaros.u-szeged at partner.samsung.com>'s request for review:
Bug 129486: Implement Math.hypot
https://bugs.webkit.org/show_bug.cgi?id=129486

Attachment 225783: Patch v6
https://bugs.webkit.org/attachment.cgi?id=225783&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225783&action=review


This is OK, but needs some work. I am going to say review+ but I would
appreciate an improved patch that addresses the exception-correctness and
multiple-evaluation issues, and removes some of the unneeded code that is
included here.

> Source/JavaScriptCore/runtime/MathObject.cpp:186
> +    double sum = 0;
> +    double compensation = 0;

These two variables should be declared down below right at the point where we
do the summation algorithm rather than at the top of the function.

> Source/JavaScriptCore/runtime/MathObject.cpp:187
> +    bool nan = false;

No need for a separate nan boolean. The code should just set max to NaN. We
don’t need to optimize NaN cases, just make sure they work correctly.

> Source/JavaScriptCore/runtime/MathObject.cpp:189
> +    if (!argsCount)
> +	   return JSValue::encode(jsNumber(0));

Why this special case? The function already handles zero arguments correctly
without a special case, and I see no need to optimize this for speed. Please
remove this code unless it’s needed to get a correct result.

> Source/JavaScriptCore/runtime/MathObject.cpp:193
> +	   if (argsCount == 1 && (arg == 0.0 || (arg+0.0) == 0.0 ))
> +	       return JSValue::encode(jsNumber(0));

This is not the right way to check for both positive and negative zero. The
IEEE floating point standard mandates that negative and positive zero both
compare as equal so this should work:

    if (argsCount == 1 && !arg)

But also, why do we need this special case at all? I would imagine the
algorithm would yield zero without a special case. If we do need a special
case, I suggest we put it outside the loop, and check max rather than checking
the argument directly.

> Source/JavaScriptCore/runtime/MathObject.cpp:195
> +	   if (std::isinf(arg))
> +	       return
JSValue::encode(jsDoubleNumber(+std::numeric_limits<double>::infinity()));

Do we need this special case? If we remove this, do we still correctly get a
result of positive infinity? If so, please remove this code.

> Source/JavaScriptCore/runtime/MathObject.cpp:197
> +	   if (std::isnan(arg))
> +	       nan = true;

Do we need this special case? If we remove this, do we still correctly get a
result of NaN? If so, please remove this code.

> Source/JavaScriptCore/runtime/MathObject.cpp:199
> +	   else if (fabs(arg)>max)
> +	       max = fabs(arg);

It would be better to write this using std::max:

    max = std::max(fabs(arg), max);

I believe that this will yield NaN if arg is NaN, although of course we should
test to be sure that works correctly. If we were keeping this code we’d need to
fix the formatting. WebKit coding style puts spaces around the ">" operator
that are missing here. I also am not sure the compiler will optimize the two
calls to fabs into only a single call, so we could consider using a local
variable.

> Source/JavaScriptCore/runtime/MathObject.cpp:205
> +    // Kahan summation algorithm significantly reduces the numerical error
in the total obtained.

Do we have tests that demonstrate the reduced numerical error, which would
detect the poorer results we would see if we did simpler summation?

> Source/JavaScriptCore/runtime/MathObject.cpp:207
> +	   double arg = exec->argument(i).toNumber(exec) / max;

I’m not sure that "arg" is quite the right name here, since this is "arg /
max". I might call this scaledArgument.

It’s not correct for this function to evaluate all the arguments twice.
Programs can observe the side effect of an argument being evaluated, and can
detect if it’s done twice. The algorithm must do it only once. So we need to
store the argument values somewhere so we don’t have to compute them again. An
easy way to do that is to use a Vector<double, 32>, which should be relatively
high performance.

This function needs to exit early if we get an exception when calling toNumber.


We should create tests that cover both these issues: That each valueOf function
is called only once, not twice, and that no additional functions are called
after a prior function throws an exception. We should test other math functions
too; I’m pretty sure some of them are handling these issues incorrectly.