[Webkit-unassigned] [Bug 129486] Implement Math.hypot

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=129486





--- Comment #25 from Tibor Mészáros <tmeszaros.u-szeged at partner.samsung.com>  2014-03-17 09:13:06 PST ---
(In reply to comment #21)
> (From update of attachment 225783 [details])
> 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.

- The declaration has been moved to down.

> 
> > 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.

- removed

> 
> > 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.

- removed

> 
> > 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.

- removed

> 
> > 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.

- Yes, we need this case, because if isn't there, the result will be NaN, instead of Infinity, if there were a NaN before.

> 
> > 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.

- removed

> 
> > 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.

- changed to this 

> 
> > 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.

- arg changed to 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.

- The second loop is working with Vector for now.

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

- Added the exception handling after the toNumber call.

> 
> 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.

- Ok, I will test them.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.