Implementation of the Math.log10 with a test for every possible value.

Comment on attachment 587799 [details] [diff] [review] Test and implementation Clearing review flag. I chatted with Marco about this patch on IRC. More tests are coming.

Added some other tests.

Corrected the case of 0 arguments (the function was returning false).

Updated to the latest changes. I'm now using the math cache also for the special cases. I've added another test that checks the log10 of 7. About JSBool and jsdouble, should I use these or the standard bool and double? This information could be useful also for the next things I'll work on.

Comment on attachment 587881 [details] [diff] [review] Patch v4 >+ else if (JSDOUBLE_IS_NaN(d) || JSDOUBLE_IS_NEG(d)) Instead of 'JSDOUBLE_IS_NEG(d)', write 'd < 0'. >+ else if (d == 1) >+ return +0.0; Try deleting these two lines. I'm pretty sure log10(1) is going to be 0.0 (and not negative zero) on all platforms, and anyway we have a test. >+assertEq(Math.log10(7), 0.8450980400142568); Hmm. That is the closest floating-point value to the exact answer, as far as I can tell. I wonder though if log10() always returns the closest value on all the platforms we care about. In any case the spec only requires an approximation, so let's not assert an exact match. Instead write a little function assertNear() and use that, here and throughout the test. r=me with the final set of changes. Thanks for the work! Generally you can use double and bool instead of jsdouble and JSBool. But note that bool is not the same type as JSBool, which is an integer type; that means that in a few places, where a function's signature has to be an exact match, you must use JSBool.

Sorry for the delay, I was studying for an exam :) Here's the patch with the requested changes to jsmath.cpp and with the function assertNear (is the gap 0.005 good?).

Comment on attachment 589706 [details] [diff] [review] Patch v5 0.005 is a little sloppier than I want to allow; for this function I think the answer should be good to within 1.0e-13 or even better. I'll change it for check-in. r=me. Are you interested in working any more on Math functions? If so, we could keep going in this bug... I'd be willing to write all the tests, if you're interested... or, you can just move on to something more interesting and someone else can do the remaining new Math functions.

(In reply to Jason Orendorff [:jorendorff] from comment #7) > Are you interested in working any more on Math functions? If so, we could > keep going in this bug... I'd be willing to write all the tests, if you're > interested... or, you can just move on to something more interesting and > someone else can do the remaining new Math functions. Yes, I'm still interested. I'll finish with this and then move to something else ;)

I've implemented log2, log1p, sign and relative tests. I think I'll finish the other functions by tomorrow.

This is mostly complete. The problem is that some functions (for example hypot) could be absent in msvc or other compilers (I've tried only with GCC so far).

Don't worry about Windows. I'll push the patch to the Try Server, whenever you're ready, to make sure it builds and the tests pass on all platforms.

I've added the missing functions for MSVC and for Android.

This was the try run (for an older version of the patch): https://tbpl.mozilla.org/?tree=Try&rev=00965186af1b

I'll start looking at this tomorrow.

Comment on attachment 591146 [details] [diff] [review] Complete patch Instead of #if defined(_MSC_VER) and so on, please use #ifdef HAVE_LOG1P and add the necessary functions to the list in configure.in here: >dnl Checks for library functions. >dnl ======================================================== >AC_PROG_GCC_TRADITIONAL >AC_FUNC_MEMCMP >AC_CHECK_FUNCS([fchmod flockfile getc_unlocked _getc_nolock getpagesize \ > lchown localtime_r lstat64 memmove random rint sbrk snprintf \ > stat64 statvfs statvfs64 strerror strtok_r truncate64]) I need to look and see if there are better approximations for log1p and so on. I think the floating-point rounding is pretty bad when you actually write out log(1 + d). For small enough d, you get a result that's exactly 0 when it should actually just be very small. Style nit: no spaces after '(' or before ')', but always spaces around an arithmetic operator. I'll finish looking at this tomorrow.

(In reply to Jason Orendorff [:jorendorff] from comment #15) > I need to look and see if there are better approximations for log1p and so > on. I think the floating-point rounding is pretty bad when you actually > write out log(1 + d). For small enough d, you get a result that's exactly 0 > when it should actually just be very small. Yes, and there is the same problem with other functions (like expm1). If we decide what should be an acceptable approximation, I can add tests to see what happens with the sensible cases and develop more precise functions for the failing ones. > I'll finish looking at this tomorrow. Perfect, I'll resolve the latest issues after the full review ;)

I have to beg your forgiveness. Today got away from me and I can't finish this review right now. But I will finish it on Monday. I found out that on x86, log1p is actually implemented using an FPU instruction (fyl2xp1). That of course is not super helpful; what we want is a fallback. Worst case, Taylor series. I'm asking around trying to find a numerics expert.

Comment on attachment 591146 [details] [diff] [review] Complete patch Review of attachment 591146 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Let's go at least one more iteration, if you can stand it... :) ::: js/src/jsmath.cpp @@ +672,5 @@ > +JSBool > +math_log10(JSContext *cx, uintN argc, Value *vp) > +{ > + double x, z; > + Throughout the C++ code, indent four spaces and put a space between 'if' and '('. The same goes for the JS test code, actually (though many tests flout the coding conventions). @@ +719,5 @@ > + return false; > + z = mathCache->lookup(log2, x); > + vp->setDouble(z); > + return true; > +} Blah. The code looks awfully redundant. Can you eliminate some of that using a C++ template? @@ +982,5 @@ > + vp->setDouble(js_PositiveInfinity); > + else if (x == js_NegativeInfinity) > + vp->setDouble(js_NegativeInfinity); > + else > + vp->setDouble((int)x); This last line is a bug: assertEq(Math.trunc(1e40 + 0.5), 1e40); assertEq(Math.trunc(1e300), 1e300); ::: js/src/tests/ecma_6/Math/expm1.js @@ +1,4 @@ > +assertNear = function(v1, v2) { > + if(Math.abs(v1-v2) > 0.0005) > + throw "Assertion failed: " + v1 + " too different from " + v2; > +} Write it like this: function assertNear(actual, expected) { var err = Math.abs(actual - expected); if (err > 1e-300 && err > actual * 1e-16) throw new Error("Assertion failed: " + actual + " too different from " + expected); } and instead of repeating it in each test, put it in ecma_6/Math/shell.js (that file is automatically loaded before each test in the directory). @@ +14,5 @@ > +for (i=-1; i>-20; i--) > + assertNear(Math.expm1(i), Math.exp(i)-1); > + > +for (i=200; i<500; i++) > + assertNear(Math.expm1(0.05 / i), Math.exp(0.05 / i) - 1); The whole point of Math.expm1 is that these two calculations will give different results as the argument gets closer to 0, due to rounding error! In other words, if this passes, it's either because assertNear() is not strict enough or because the test isn't trying hard enough. Both are bad. We need a test that actually checks Math.exmp1's behavior for values quite close to 0. @@ +27,5 @@ > +assertEq(Math.expm1(undefined), NaN); > +assertEq(Math.expm1(null), 0); > +assertEq(Math.expm1(void 0), NaN); > +assertEq(Math.expm1(false), 0); > +assertEq(Math.expm1({valueOf: function () { return -0; }}), -0); Hmm. Instead of repeating these lines, why not have a test that checks these properties for all the new functions that take a single Number argument: function test(fn) { assertEq(fn(), fn(NaN)); assertEq(fn(undefined), fn(NaN)); assertEq(fn(null), fn(0)); assertEq(fn(false), fn(0)); ... } test(Math.expm1); test(Math.log10); ... ::: js/src/tests/ecma_6/Math/jstests.list @@ +8,5 @@ > +script tanh.js > +script acosh.js > +script asinh.js > +script atanh.js > +script hypot.js A bunch of these test files are missing from the patch! ::: js/src/tests/ecma_6/Math/log1p.js @@ +6,5 @@ > +for (i=1; i<20; i++) > + assertNear(Math.log1p( Math.pow(Math.E, i) - 1 ), i); > + > +for (i=1; i<20; i++) > + assertNear(Math.log1p( Math.pow(Math.E, -i) - 1 ), -i); Here again, we need tests that actually attack log1p in the range where it is useful, i.e. the range where Math.log(1+x) gives imprecise results. For example: log1p(1e-15) 9.999999999999995e-16 log(1 + 1e-15) 1.110223024625156e-15 log1p(1e-52) 1e-52 log(1 + 1e-52) 0.0 The assertNear() definition I gave should be able to detect the difference; that is, if you write assertNear(Math.log1p(1e-15), Math.log(1 + 1e-15)); // incorrect test then I would expect that to fail, because the two values differ. The correct test is: assertNear(Math.log1p(1e-15), 9.999999999999995e-16); and we need to make sure the fallback code is good enough that this passes on all platforms! ::: js/src/tests/ecma_6/Math/trunc.js @@ +1,5 @@ > +for (i=1,f=1.1; i<20; i++,f+=1.0) > + assertEq(Math.trunc(f), i); > + > +for (i=-1,f=-1.1; i>-20; i--,f-=1.0) > + assertEq(Math.trunc(f), i); This also needs to test that trunc is different from round, i.e. that trunc(0.9999) is 0, and trunc(-0.9999) is -0. +1 for recognizing that unlike most of the others, trunc needs to be exact. :) @@ +7,5 @@ > +assertEq(Math.trunc(0), 0); > +assertEq(Math.trunc(-0), -0); > +assertEq(Math.trunc(NaN), NaN); > +assertEq(Math.trunc(Number.POSITIVE_INFINITY), Number.POSITIVE_INFINITY); > +assertEq(Math.trunc(Number.NEGATIVE_INFINITY), Number.NEGATIVE_INFINITY); As I mentioned, this needs to test more cases. 1e40 is one such case; 1e40 + 0.5 is another; you can find some interesting cases around 2^32 and 2^64; really huge and tiny values should be tested too.

For trunc(), take a look at how floor() and ceil() are implemented. It should be possible to compute the correct result without so many conditional jumps.

Erm, tests have always been anything-goes as far as coding style. As long as the style's not actively confusing, I've never said a word about coding style in in any test I've reviewed. (I can only remember that happening once, tho the real count's probably a few more.) And I'd say I tend to be a pretty strong code style stickler... :-)

Sorry if I'm working slowly, but I've been (and am) really busy. I've added new tests for edge cases and created a new assertNear function. The new function uses typed arrays, I know it only works on little-endian CPUs for now.

Looks good to me. I'm going to run this against the Try Server and see if the template trick works in MSVC. (I had some trouble doing something like that once.) Also to see if we need to worry about big-endian platforms.

This breaks the build on Android: js/src/jsmath.cpp: At global scope: js/src/jsmath.cpp:861: error: 'log2' is not a valid template argument for type 'double (*)(double)' because function 'double log2(double)' has not external linkage js/src/jsmath.cpp:861: error: 'log2' is not a valid template argument for type 'double (*)(double)' because function 'double log2(double)' has not external linkage js/src/jsmath.cpp:861: error: no matches converting function 'math_func' to type 'JSBool (*)(struct JSContext*, unsigned int, class jsval*)' js/src/jsmath.cpp:672: error: candidates are: template<double (* f)(double)> JSBool math_func(JSContext*, unsigned int, JS::Value*) https://tbpl.mozilla.org/?tree=Try&rev=5a5ce7307356

Some tests fail on Windows. ecma_6/Math/log1p.js: 0 too different from 5e-103 by 3076498402659817000 ecma_6/Math/expm1.js: 0 too different from 5e-103 by 3076498402659817000 ecma_6/Math/acosh.js: Infinity too different from 691.4686750787737 by 4569574998620487700 ecma_6/Math/asinh.js: -Infinity too different from -20 by 4592545720011063300 ecma_6/Math/atanh.js: 3.3306690738754686e-16 too different from 3.191891195797325e-16 by 281474976710656

Comment on attachment 604640 [details] [diff] [review] Patch Clearing the review flag for now.

The fallback functions for MSVC were only placeholders, this is why the tests failed on that platform. This new iteration of the patch compiles on Android. The tests now pass on Windows. I took inspiration from Boost for the implementation of the functions. I took (a)cosh,(a)sinh and (a)tanh test data from Boost's tests, that used WolframAlpha's data. This is the try run for Mac and Android (I tested the patch locally with Linux and Windows): https://tbpl.mozilla.org/?tree=Try&rev=8b913241566f

Jason, do you have time for a review? I'll unbitrot the patch once it's reviewed.

Reviewing now.

Comment on attachment 613728 [details] [diff] [review] Patch Review of attachment 613728 [details] [diff] [review]: ----------------------------------------------------------------- I need to look a little closer at these hyperbolic function formulas and make sure it's ok to take code from Boost (the license is extremely permissive so I'd be surprised if there were any issues). Otherwise this is starting to look good. Unbitrot this and set r?me, let's get it in. ::: js/src/jsmath.cpp @@ +85,5 @@ > +#ifndef EPS > +#define EPS 2.4651903288156618919116517665087e-32L > +#endif > +#ifndef ROOT_EPS > +#define ROOT_EPS 0.32927225399135962333569506281281311031656150598474e-9L What is ROOT_EPS supposed to be? (Note that it is not the square root of EPS, even approximately.) None of the algorithms are particularly sensitive to these constants, are they? It seems wrong to show 50 digits of precision here, when the C++ compiler is going to immediately throw away all but the first 17 digits. I also think it would be better to drop the L in these constants. As a last note, it seems weird to use scientific notation this way; the usual thing would be 3.2927225399135965e-10 not 0.32927225399135965e-9 @@ +88,5 @@ > +#ifndef ROOT_EPS > +#define ROOT_EPS 0.32927225399135962333569506281281311031656150598474e-9L > +#endif > +#ifndef FORTH_ROOT_EPS > +#define FORTH_ROOT_EPS 0.18145860519450699870567321328132261891067079047605e-4L It's spelled "fourth". @@ +674,5 @@ > vp->setDouble(z); > return JS_TRUE; > } > > +typedef double (*F)(double); Instead of 'F', could you use a more descriptive name? This is only used in two places, so it could just as well be MathFunction or something. @@ +774,5 @@ > + > + double dAbs = fabs(d); > + > + if (dAbs < EPS) > + return d; Style nit: here and throughout, indent 4 spaces. @@ +775,5 @@ > + double dAbs = fabs(d); > + > + if (dAbs < EPS) > + return d; > + else if (dAbs < 0.5) { Style nit: according to prevailing style, if any branch of an if...else if...else... chain uses curly braces, all of them do. @@ +779,5 @@ > + else if (dAbs < 0.5) { > + expm1_series series(d); > + double result = 0; > + for (int i=0; i<50; i++) > + result += series.nextTerm(); Definitely inline this: else if (dAbs < 0.5) { // Maclaurin series: exp(x) - 1 = x + x^2/2! + x^3/3! + ... double term = 1.0; double result = 0.0; for (int i = 1; i < 50; i++) { term = term * d / i; result += term; } return result; } and delete class expm1_series. Same thing in log1p. Either way, these two functions definitely need a one-line comment briefly explaining what series this is. Style nit: use spaces around '=' and '<' in 'for (int i=0; i<50; i++)' @@ +783,5 @@ > + result += series.nextTerm(); > + return result; > + } > + else > + return exp(d) - 1.0; Style nit: curly braces again @@ +801,5 @@ > + } > + else > + return log(d + sqrt(d * d - 1)); > + } > + else if (d > 1) { Style nit: Here and elsewhere, put "else" on the same line as the preceding "}" if any. @@ +844,5 @@ > + if (fabs(d) >= ROOT_EPS) { > + double d3 = d * d * d; > + result -= d3 / 6.; > + } > + Style nit: remove the space characters on this blank line. @@ +939,5 @@ > + vp->setDouble(js_NaN); > + return true; > + } > + > + if (!ToNumber(cx, vp[2], &x) || !ToNumber(cx, vp[3], &y)) Don't access vp[3] if argc is less than 2. It's better to use CallArgs for this, because it has a nice API and because it asserts that you're not trying to read past the end of vp: CallArgs args = CallArgsFromVp(argc, vp); if (!ToNumber(cx, args[0], &x)) return false; if (argc > 1) { if (!ToNumber(cx, args[1], &y)) return false; } else { y = js_NaN; } args.rval().setDouble(hypot(x, y)); return true; ::: js/src/tests/ecma_6/Math/acosh.js @@ +17,5 @@ > + > +for (i=-20; i<1; i++) > + assertEq(Math.acosh(i), NaN); > + > +assertNear(Math.acosh(1e300), 691.46867507877365051481466852676743884840594672299214); More than 17 digits is a waste. js> 691.46867507877365051481466852676743884840594672299214 691.4686750787737 js> 691.46867507877365051481466852676743884840594672299214 === 691.4686750787737 true ::: js/src/tests/ecma_6/Math/cosh_data.js @@ +1,2 @@ > +cosh_data = [ > + [ 1.000001430511474609375, 0.001691455665129294448190238354291760044433 ], Style nit: remove all the trailing whitespace in this file and in all the other similar test files. Reduce to the amount of precision JS can handle (you can do this using a regular expression in JS, if you like): js> var s = ' [ 1.0000019073486328125, 0.001953124689559275021527821917817027620963 ], '; js> s.replace(/[0-9]+(\.[0-9]+)?([Ee][+-][0-9]+)?\b/g, function (a) { return Number(a).toString(); }); " [ 1.0000019073486328, 0.001953124689559275 ], " Since we're using data from Boost tests here, we'll have to include the Boost license somewhere. Don't worry about it; I'll take care of the licensing side of things. ::: js/src/tests/ecma_6/Math/expm1.js @@ +77,5 @@ > + [ 0.65641486644744873046875e0, 0.504655547804425337585747233639878779038e0, 0.927868264760303541518755094801625468889e0 ], > + [ 0.6588299274444580078125e0, 0.5061124908504770714998094003973873746865e0, 0.932529810907327764547265337859703076731e0 ], > + [ 0.673553526401519775390625e0, 0.5149492258613715166147831918156453824087e0, 0.9611941077924732903931571711562687857294e0 ], > + [ 0.69003379344940185546875e0, 0.5247485248589687068613254590642295922543e0, 0.9937829089064982966773652105234409046975e0 ], > + [ 0.69504582881927490234375e0, 0.5277097781183924897416903820415806504912e0, 0.1003800903666412193968978978979482061619e1 ] Only the first and last number in each row is used; can we remove the middle one then? Or merge the logp1 test with this one. Either way, but let's not have totally unused data hanging around like that. @@ +85,5 @@ > + assertNear(Math.expm1(log1p_expm1_data[i][0]), log1p_expm1_data[i][2]); > + > +function expm1_taylor(x) { > + if (Math.abs(x) < 1e-5) > + return x + Math.pow(x,2)/2 + Math.pow(x,3)/6 + Math.pow(x,4)/24 + Math.pow(x,5)/120 + Math.pow(x,6)/720 + Math.pow(x,7)/5040 + Math.pow(x,8)/40320 + Math.pow(x,9)/362880 + Math.pow(x,10)/3628800; If making assertNear() pickier causes this or the tests using powp1_taylor to fail, it could (just conceivably) be because of the order in which these terms are added. (When adding a series of floating-point numbers, you're supposed to start with the smallest (magnitude) values. It minimizes roundoff error when every bit counts. In JS, addition is left-associative and defined to happen left-to-right, so here we're starting by adding together the greatest values. Here I think it probably shouldn't matter because of other details about the particular series.) ::: js/src/tests/ecma_6/Math/hypot.js @@ +15,5 @@ > +for (i=-20; i<20; i++) > + assertEq(Math.hypot(i, Number.POSITIVE_INFINITY), Number.POSITIVE_INFINITY); > + > +for (i=-20; i<20; i++) > + assertEq(Math.hypot(i, Number.NEGATIVE_INFINITY), Number.POSITIVE_INFINITY); Also test the cases where both arguments are infinities. @@ +18,5 @@ > +for (i=-20; i<20; i++) > + assertEq(Math.hypot(i, Number.NEGATIVE_INFINITY), Number.POSITIVE_INFINITY); > + > +for (i=1,j=1; i<2; i+=0.05,j+=0.05) > + assertNear(Math.hypot(i, j), Math.sqrt(i*i + j*j)); Nit: Remove trailing whitespace on this line. @@ +30,5 @@ > +assertEq(Math.hypot(void 0, void 0), NaN); > +assertEq(Math.hypot(false, false), 0); > + > +assertEq(Math.hypot(), NaN); > +assertEq(Math.hypot(1), NaN); Also test Math.hypot(Infinity). (The result should be NaN, not infinity.) Also test that Math.hypot({valueOf: function() {...}}) calls the function and then returns NaN (it can't skip converting the object to a number). ::: js/src/tests/ecma_6/Math/jstests.list @@ +12,5 @@ > +script atanh.js > +script hypot.js > +script trunc.js > +script sign.js > +script trunc.js This file won't be necessary anymore; please remove it. ::: js/src/tests/ecma_6/Math/shell.js @@ +5,5 @@ > + float64View[1] = expected; > + > + var int32View = new Uint32Array(buffer); > + var num1 = (int32View[1] * 0x100000000) + int32View[0]; > + var num2 = (int32View[3] * 0x100000000) + int32View[2]; JavaScript numbers are just 64-bit floats. They can't hold the full range of 64-digit numbers. Therefore these lines computing num1 and num2 discard something like 12 bits of precision. For example, if int32View[1] is 0xf0000000, then num1 is the same whether int32View[0] is 0 or 1000. This causes numbers to seem to be near even when they are not. I think this works as desired: var dist = Math.abs((int32View[3] - int32View[1]) * 0x100000000 + int32View[2] - int32View[0]); if (dist > 1) throw ...; If you factor this function into two functions, near() and assertNear(), then you can add a test to prove that near() works properly: assertEq(near(0x10000000000002, 0x100000000001ff), false); and even: assertEq(near(0x10000000000002, 0x10000000000004), false); @@ +6,5 @@ > + > + var int32View = new Uint32Array(buffer); > + var num1 = (int32View[1] * 0x100000000) + int32View[0]; > + var num2 = (int32View[3] * 0x100000000) + int32View[2]; > + Nit: remove the space characters on this blank line. ::: js/src/tests/ecma_6/Math/tanh.js @@ +1,1 @@ > +load("ecma_6/Math/tanh_data.js"); Now that we're autodetecting tests, I'm not sure you can have separate data files like this anymore. However you ultimately get it to work is fine with me. ::: js/src/tests/ecma_6/jstests.list @@ +1,3 @@ > include Map/jstests.list > include Set/jstests.list > +include Math/jstests.list ecma_6/jstests.list file doesn't exist anymore. This change is no longer needed. (We auto-generate the lists of tests now.)

Hey Marco, have you had time to look at the review comments?

(In reply to :Ms2ger from comment #30) > Hey Marco, have you had time to look at the review comments? I won't be able to work on this bug in the near future, I'm too busy with my University. If someone wants to take this in the meantime, feel free to do so.

I hope somebody wants to finish this.

Let's unassign me. Maybe I'll have time the next month, but I don't want to "occupy" the bug.

Implementation of new functions defined by ES6. This patch used the original patch and tests as a base, but it contains several modifications including some fixes and additions. The test files remain mostly unchanged (with some exception) The approach was trying to take into consideration relevant discussions in this bug report + some questions over IRC.

Some additional comments: - As suggested by the ES6 draft, native functions are given a preference over custom implementations. I took C++98 as a base to draw a line between std functions and extensions provided by the compiler. - As suggested in IRC, I use MathCache only for non-trivial functions (ie. those that require some noticeable computation) - Similarly to the original patch, the inverse of hyperbolic functions are based on the Boost library (which relies on Wolfram Alpha documentation) - Most test files remain with little change but still may need more work. Jason suggested using relative error of 1e-16 but I had to relax it to 1e-12 given that my stdlib implementation of hyperbolic functions would fail the tests otherwise. - All tests have been made on Ubuntu 12.04 with gcc 4.6.3 . Haven't been able to try other environments. Any comments or corrections appreciated.

Comment on attachment 734724 [details] [diff] [review] First iteration of my patch Review of attachment 734724 [details] [diff] [review]: ----------------------------------------------------------------- Thank you again. I am probably not the best person to comment on the specifics of how we should implement the math functions. I would however like to give some feedback on the style. I would agree with your ideas in comment 35. ::: js/src/jsmath.cpp @@ +697,5 @@ > } > > +JSBool > +js::math_log10(JSContext *cx, unsigned argc, Value *vp) > +{ So, I know all other math functions don't do this (yet). However I would like you to use CallArgs. public/CallArgs.h has a good overview of that, but I am still going to give an example. If you don't mind could apply these review comments to all other functions? @@ +698,5 @@ > > +JSBool > +js::math_log10(JSContext *cx, unsigned argc, Value *vp) > +{ > + if (argc == 0) { This would be CallArgs args = CallArgsFromVp(argc, vp); if (args.length() == 0) { args.rval().setNumber(js_NaN); return true; } Also note the usage of true/false instead of JS_TRUE, JS_FALSE. @@ +704,5 @@ > + return JS_TRUE; > + } > + > + double x; > + if (!ToNumber(cx, vp[2], &x)) args[0] @@ +710,5 @@ > + > + if (MOZ_DOUBLE_IS_NaN(x) || x < 0) { > + vp->setDouble(js_NaN); > + } > + else if (x == 0.0) { } else if on one line @@ +715,5 @@ > + vp->setDouble(js_NegativeInfinity); > + } > + else if (x == js_PositiveInfinity) { > + vp->setDouble(js_PositiveInfinity); > + } Does log10 give the wrong output for these values, or why are they treated like that? @@ +721,5 @@ > + MathCache *mathCache = cx->runtime->getMathCache(cx); > + if (!mathCache) > + return JS_FALSE; > + double z = mathCache->lookup(log10, x); > + vp->setDouble(z); args.thisv().setDouble(z) @@ +861,5 @@ > + */ > + double z = x + (x * x) / 2 + (x * x * x) / 6; > + return z; > + } > + else { } else is the same line as well. @@ +1139,5 @@ > +} > + > +double atanh(double x) > +{ > + const double EPSILON = std::numeric_limits<double>::epsilon(); drop the space. @@ +1263,5 @@ > +} > + > +double sign(double x) > +{ > + return x == 0? x : x < 0? -1 : 1; x == 0 ? (note the extra space) ::: js/src/tests/ecma_6/Math/sinh_data.js @@ +1,1 @@ > +sinh_data = [ Is there any need to have that in an extra file? Also note the trailing whitespace.

>args.thisv().setDouble(z) args.rval().setDouble(z) of course! Thank you Ms2ger.

(In reply to Tom Schuster [:evilpie] from comment #36) > Comment on attachment 734724 [details] [diff] [review] > First iteration of my patch > > Review of attachment 734724 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you again. I am probably not the best person to comment on the > specifics of how we should implement the math functions. I would however > like to give some feedback on the style. > > I would agree with your ideas in comment 35. Thanks for the feedback, it's much appreciated. > ::: js/src/jsmath.cpp > @@ +697,5 @@ > > } > > > > +JSBool > > +js::math_log10(JSContext *cx, unsigned argc, Value *vp) > > +{ > > So, I know all other math functions don't do this (yet). However I would > like you to use CallArgs. public/CallArgs.h has a good overview of that, but > I am still going to give an example. > > If you don't mind could apply these review comments to all other functions? Sure, no problem with that. > @@ +715,5 @@ > > + vp->setDouble(js_NegativeInfinity); > > + } > > + else if (x == js_PositiveInfinity) { > > + vp->setDouble(js_PositiveInfinity); > > + } > > Does log10 give the wrong output for these values, or why are they treated > like that? My initial approach was dealing with these cases explicitly as described in the draft, but of course this wouldn't be always needed. Will test these cases. > ::: js/src/tests/ecma_6/Math/sinh_data.js > @@ +1,1 @@ > > +sinh_data = [ > > Is there any need to have that in an extra file? > Also note the trailing whitespace. As I commented before, I didn't deal much with the existing test code, but recall reading somewhere else (some other bug report) about having everything in a single file. So far my workaround was appending "reportCompare(0, 0, 'ok');" but this isn't good, so will merge this in a single file and also prune the extra decimals in these test files. Will work on this during this week, so should get back to you soon.

This iteration contains the changes requested: style (including use of CallArgs), removal of unneeded cases (zero, infinite, etc) and merge of data into a single test file (including removal of unneeded precision in test data)

Comment on attachment 736331 [details] [diff] [review] Second iteration Requesting review on behalf on David, who asked me on IRC on how to proceed.

Hi, I was a computational mathematics student a few years ago and dropped out, to the point: I want to get back into programming and for the first time contribute to a open source project. I had 3 semesters of algorithms and date structures courses (first in Java, then in C), so I can read code and write basic stuff, upon some research and searching I got here and want to get started, what needs to be done and how do I start?

I will review this patch tonight if I get the chance and Monday morning at the latest.

Comment on attachment 736331 [details] [diff] [review] Second iteration Review of attachment 736331 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the very slow review. I promise to get them turned around much faster in this bug from now on. Beautiful patch. There's a lot of work still to do though. Some platforms have at least some of these functions in <math.h>. Let's use them when they're available. You can use the configure stuff from Marco's last patch. ::: js/src/jsmath.cpp @@ +709,5 @@ > + if (!ToNumber(cx, args[0], &x)) > + return false; > + > + if (MOZ_DOUBLE_IS_NaN(x) || x < 0) { > + args.rval().setNumber(js_NaN); These cases should be handled by the platform's log10 function. Please delete the extra `if`! The name of the game here is speed, so we lean on platform libraries, backstopped by our assertions and tests, for correctness. For weird platforms where a particular library function is badly-behaved in some case, we deploy goofy #ifdef workarounds (see math_acos_impl, ecmaAtan2, js_math_ceil_impl, math_exp_impl, and so on). Also aggressively remove special cases in all the other functions, at least when we're calling a library function or using plain old double arithmetic. (An exception is noted below.) Background: <jorendorff> what's the theory behind us not special-casing NaN in math_sin and friends? <luke> it's just an "in practice math functions don't seem to produce non-canonical nans" thing. (i tried adding canonicalization once and it was a big slowdown) <luke> in theory, they shouldn't be injecting non-canonical nans because, by spec, the payload portion is supposed to communicate some meaning <jwalden> weren't we doing a canonicalization in at least some of those math functions, because OS X libc or something? <luke> well, because of an OSX libc, we slightly relaxed our predicate for "what is a double" <luke> to allow the OSX lib to inject non-canonical low-bits nans @@ +719,5 @@ > + args.rval().setNumber(z); > + } > + > + return true; > +} For speed, the math cache should be checked before special cases. (The whole wager with the math cache is that user code will do log10(n) for the same n many times in a row; we first implemented this for some bogus benchmark, but found to our astonishment that it actually helps some real-world code. Branches are slow, so we put the fast path up front, in front of as many branches as possible.) Also, see if you can eliminate some of the code duplication using macros or templates. @@ +783,5 @@ > + > + if (MOZ_DOUBLE_IS_NaN(x) || x < -1.0) { > + args.rval().setNumber(js_NaN); > + } else if (x == 0.0) { > + args.rval().setNumber(x); Definitely remove the special case for NaN; try removing the special case for -0 too (it might work). @@ +826,5 @@ > + > + if (MOZ_DOUBLE_IS_NaN(x)) { > + args.rval().setNumber(js_NaN); > + } else if (x == 0.0) { > + args.rval().setNumber(x); Removing this special case for -0 might not work; at least move it into expm1. @@ +1048,5 @@ > + const double FOURTH_ROOT_EPSILON = sqrt(SQUARE_ROOT_EPSILON); > + > + if (fabs(x) >= FOURTH_ROOT_EPSILON) { > + // http://functions.wolfram.com/ElementaryFunctions/ArcTanh/02/ > + if(fabs(x) < 0.5) Style int: space after `if`. @@ +1093,5 @@ > + return true; > +} > + > +// Euclidean distance > +// http://www.fon.hum.uva.nl/praat/old/src/sources_5200/GSL/gsl_sys__hypot.c Is this better than `hypot(hypot(x, y), z)`? @@ +1098,5 @@ > +double hypot3(double x, double y, double z) > +{ > + double xabs = std::abs(x); > + double yabs = std::abs(y); > + double zabs = std::abs(z); Style micro-nit: Please use mozilla::Abs() instead of std::abs(). This file already contains `using mozilla::Abs;` so you can just write `Abs(x)` and so on. std::abs would be correct in this particular case, but there are enough cases where it *is* wrong that the explanation of why it's correct is unobvious; it depends on which headers we've included. http://whereswalden.com/2013/04/30/introducing-mozillaabs-to-mfbt/ Rather than commenting the unobvious, just use Abs(). @@ +1133,5 @@ > + if (!ToNumber(cx, args[2], &z)) > + return false; > + } else { > + z = 0; > + } Since there's a branch here already, just call the C standard function hypot() when there are two arguments. @@ +1138,5 @@ > + > + if (x == js_PositiveInfinity || x == js_NegativeInfinity || > + y == js_PositiveInfinity || y == js_NegativeInfinity || > + z == js_PositiveInfinity || z == js_NegativeInfinity) { > + args.rval().setDouble(js_PositiveInfinity); It is pretty subtle why this special case is necessary (the division); add a one-line comment? (This special case will move to hypot3.) @@ +1203,5 @@ > + return true; > +} > + > +/* > + * Compute cube root of a positive number. This code is based on Ken Turkowski's example code Interesting. Is this better than: return x > 0.0 ? pow( x, 1.0 / 3.0) : -pow(-x, 1.0 / 3.0); ? @@ +1221,5 @@ > + ex = (ex - shx) / 3; /* exponent of cube root */ > + fr = std::ldexp(fr, shx); > + /* 0.125 <= fr < 1.0 */ > + > + /* Compute seed with a quadratic qpproximation */ "approximation" @@ +1222,5 @@ > + fr = std::ldexp(fr, shx); > + /* 0.125 <= fr < 1.0 */ > + > + /* Compute seed with a quadratic qpproximation */ > + fr = (-0.46946116F * fr + 1.072302F) * fr + 0.3812513F; /* 0.5 <= fr < 1 */ The F suffix in the original code is because it's using float arithmetic, but double arithmetic (in non-vectorized code) is not any slower -- we can probably get more bits just by using more precision here, and reduce the number of iterations -- if all those code is really necessary at all. ::: js/src/jsmath.h @@ -98,5 @@ > namespace js { > > extern JSBool > -math_exp(JSContext *cx, unsigned argc, Value *vp); > - Good catch. (A little embarrassing for us that the extra declaration was there to be caught, though...) @@ +133,5 @@ > extern JSBool > +math_log10(JSContext *cx, unsigned argc, js::Value *vp); > + > +extern JSBool > +math_log2(JSContext *cx, unsigned argc, js::Value *vp); The reason all these are exposed in the header file is that they're used by: ion/AsmJSLink.cpp ion/CodeGenerator.cpp ion/MCallOptimize.cpp Please add boilerplate in those files. If you hate boilerplate, you can eliminate the need for it in all those places by defining: #define FOR_EACH_CACHED_MATH_FUNCTION(macro) \ macro(sin) \ macro(cos) \ macro(tan) \ ... macro(atanh) and, for example, in jsmath.h: #define DECLARE_ENTRY_POINTS(name) \ extern JSBool math_##name(JSContext *cx, unsigned argc, js::Value *vp); \ extern double math_##name##_impl(MathCache *cache, double x); FOR_EACH_CACHED_MATH_FUNCTION(DECLARE_ENTRY_POINTS); #undef DECLARE_ENTRY_POINTS which will let you delete all these extern declarations ... at some cost in greppability, I suppose. The same trick can be used in all the other places. To be clear: this macro stuff is purely optional, but please make sure the new functions work with ion. ::: js/src/tests/ecma_6/Math/acosh.js @@ +21,5 @@ > + [1.9838471412658691, 1.307581416910453], > + [2.1576128005981445, 1.4035188779741334], > + [2.406397819519043, 1.5250070845427517], > + [3.386958122253418, 1.8905372013072799], > + [4.451677322387695, 2.1735673399948254], I think that should be 2.1735673399948268. Just kidding. @@ +262,5 @@ > + [1875817529344, 28.953212876533797] > +] > + > +for (var i = 0; i < cosh_data.length; i++) > + assertNear(Math.acosh(cosh_data[i][0]), cosh_data[i][1]); for (let [x, y] of cosh_data) assertNear(Math.acosh(x), y);

You can copy what's in js/src/tests/js1_8_5/shell.js to ecma_6/shell.js to allow let syntax in this test.

Thanks for your comments, both here and on IRC. Hopefully this iteration will get us closer. > Beautiful patch. There's a lot of work still to do though. > > Some platforms have at least some of these functions in <math.h>. Let's use > them when they're available. You can use the configure stuff from Marco's > last patch. Done. The rationale I used was checking for functions both available since C99 or C++11 (I'm assuming C++98 as a baseline) > ::: js/src/jsmath.cpp > @@ +709,5 @@ > > + if (!ToNumber(cx, args[0], &x)) > > + return false; > > + > > + if (MOZ_DOUBLE_IS_NaN(x) || x < 0) { > > + args.rval().setNumber(js_NaN); > > These cases should be handled by the platform's log10 function. Please > delete the extra `if`! Done, although I'm still checking for < 0. The idea is avoid flooding the math cache with arguments that lead to NaN. I should have put a comment in the code, so I added it now. That's my rationale, but please let me know if this isn't worth it (anyway, removing these extra cases is a trivial task) > The name of the game here is speed, so we lean on platform libraries, > backstopped by our assertions and tests, for correctness. For weird > platforms where a particular library function is badly-behaved in some case, > we deploy goofy #ifdef workarounds (see math_acos_impl, ecmaAtan2, > js_math_ceil_impl, math_exp_impl, and so on). I changed the current implementation in three separate layers. First one is js::math_XXX() which calls js::math_impl_XXX() which uses a cached value or relies on a library (platform or custom implementation when not available). This approach allowed to create a template function and avoid some code duplication. The code of js::math() simply specializes an implementation function. This made it easier to integrate with the ion modules (as requested below) but I don't know if we should aim to a better approach (would inlining such functions make any noticeable difference?) > Also aggressively remove special cases in all the other functions, at least > when we're calling a library function or using plain old double arithmetic. > (An exception is noted below.) [...] > For speed, the math cache should be checked before special cases. > > (The whole wager with the math cache is that user code will do log10(n) for > the same n many times in a row; we first implemented this for some bogus > benchmark, but found to our astonishment that it actually helps some > real-world code. Branches are slow, so we put the fast path up front, in > front of as many branches as possible.) > > Also, see if you can eliminate some of the code duplication using macros or > templates. Done. > @@ +783,5 @@ > > + > > + if (MOZ_DOUBLE_IS_NaN(x) || x < -1.0) { > > + args.rval().setNumber(js_NaN); > > + } else if (x == 0.0) { > > + args.rval().setNumber(x); > > Definitely remove the special case for NaN; try removing the special case > for -0 too (it might work). It certainly did! > @@ +826,5 @@ > > + > > + if (MOZ_DOUBLE_IS_NaN(x)) { > > + args.rval().setNumber(js_NaN); > > + } else if (x == 0.0) { > > + args.rval().setNumber(x); > > Removing this special case for -0 might not work; at least move it into > expm1. That's correct. Done. > @@ +1093,5 @@ > > + return true; > > +} > > + > > +// Euclidean distance > > +// http://www.fon.hum.uva.nl/praat/old/src/sources_5200/GSL/gsl_sys__hypot.c > > Is this better than `hypot(hypot(x, y), z)`? Certainly not, and being hypot() in C++11 it definitely makes more sense to use it as a base. > @@ +1098,5 @@ > > +double hypot3(double x, double y, double z) > > +{ > > + double xabs = std::abs(x); > > + double yabs = std::abs(y); > > + double zabs = std::abs(z); > > Style micro-nit: Please use mozilla::Abs() instead of std::abs(). This file > already contains `using mozilla::Abs;` so you can just write `Abs(x)` and so > on. > > std::abs would be correct in this particular case, but there are enough > cases where it *is* wrong that the explanation of why it's correct is > unobvious; it depends on which headers we've included. > > http://whereswalden.com/2013/04/30/introducing-mozillaabs-to-mfbt/ > > Rather than commenting the unobvious, just use Abs(). Note taken, although hypot3() is now gone. > @@ +1133,5 @@ > > + if (!ToNumber(cx, args[2], &z)) > > + return false; > > + } else { > > + z = 0; > > + } > > Since there's a branch here already, just call the C standard function > hypot() when there are two arguments. Please take a look at the hypot()'s new implementation. Considering that calling it with two arguments is the usual case, I've changed the code accordingly. I'm wondering whether it would make sense to add suport for binary functions in MathCache::lookup() > @@ +1203,5 @@ > > + return true; > > +} > > + > > +/* > > + * Compute cube root of a positive number. This code is based on Ken Turkowski's example code > > Interesting. Is this better than: > > return x > 0.0 > ? pow( x, 1.0 / 3.0) > : -pow(-x, 1.0 / 3.0); > > ? There is some loss of precision for small floats when using the above code. For example, Math.pow(1e-300, 1.0 / 3.0) = 1.0000000000000128e-100, that is approx. > 1e-14 of relative error. On the other hand the above algorithm gives the expected result (1e-100) But considering the current relative error is at 1e-12 then it doesn't make much of a difference. This also pointed out the fact that the test code was using assertEq() instead of assertNear() for such case (corrected now) > @@ +133,5 @@ > > extern JSBool > > +math_log10(JSContext *cx, unsigned argc, js::Value *vp); > > + > > +extern JSBool > > +math_log2(JSContext *cx, unsigned argc, js::Value *vp); > > The reason all these are exposed in the header file is that they're used by: > ion/AsmJSLink.cpp > ion/CodeGenerator.cpp > ion/MCallOptimize.cpp > > Please add boilerplate in those files. I chose this option. Besides this, I also added cases to AsmJS.cpp (although I had problems adding "sign" as this is not a std c++ function) I lack some time to get familiar with that code, so would appreciate some pointer about how to proceed with this and thus avoid the warning. > @@ +262,5 @@ > > + [1875817529344, 28.953212876533797] > > +] > > + > > +for (var i = 0; i < cosh_data.length; i++) > > + assertNear(Math.acosh(cosh_data[i][0]), cosh_data[i][1]); > > for (let [x, y] of cosh_data) > assertNear(Math.acosh(x), y); > You can copy what's in js/src/tests/js1_8_5/shell.js to ecma_6/shell.js to allow > let syntax in this test. Thanks! That made it.

One further comment: the new template function would allow us to use it with some existing unary math functions (tan, sqrt, etc). I presume this could be made as a separate bug id although it could also be changed within this patch. Please let me know the more appropriate approach.

Corrected due to code change in latest branch

psior, thanks for your great work in this bug. We're getting very close. (In reply to David Caabeiro [:psior] from comment #45) > > ::: js/src/jsmath.cpp > > > + if (MOZ_DOUBLE_IS_NaN(x) || x < 0) { > > > + args.rval().setNumber(js_NaN); > > > > These cases should be handled by the platform's log10 function. Please > > delete the extra `if`! > > Done, although I'm still checking for < 0. The idea is avoid flooding the > math cache with arguments that lead to NaN. [...] Not worth it, I think: we don't want to punish code that's using log in its useful domain to benefit code that's passing it negative numbers. The same goes for a few others. (Also, I think this simplifies the code considerably, eliminating one of the layers.) > Please take a look at the hypot()'s new implementation. Considering that > calling it with two arguments is the usual case, I've changed the code > accordingly. Looks good! > I'm wondering whether it would make sense to add suport for binary functions > in MathCache::lookup() Maybe! But maybe not today. > > > + * Compute cube root of a positive number. This code is based on Ken Turkowski's example code > > > > Interesting. Is this better than: > > > > return x > 0.0 > > ? pow( x, 1.0 / 3.0) > > : -pow(-x, 1.0 / 3.0); > > > > ? > > There is some loss of precision for small floats when using the above code. > For example, Math.pow(1e-300, 1.0 / 3.0) = 1.0000000000000128e-100, that is > approx. > 1e-14 of relative error. On the other hand the above algorithm > gives the expected result (1e-100) > > But considering the current relative error is at 1e-12 then it doesn't make > much of a difference. This also pointed out the fact that the test code was > using assertEq() instead of assertNear() for such case (corrected now) Hmmmm. Precision is important, speed is important... At the time I wrote that comment, I thought pow() was probably faster. But on second thought, we should just check. If you have time, please write a quick C program to compute a bunch of cube roots, and see which implementation is faster. If you prefer, we can check in the simpler code for now. I can file a followup bug to investigate further. > I chose this option. Besides this, I also added cases to AsmJS.cpp (although > I had problems adding "sign" as this is not a std c++ function) > > I lack some time to get familiar with that code, so would appreciate some > pointer about how to proceed with this and thus avoid the warning. Great. Please just remove the new mentions of "sign" from the AsmJS code, or comment them out, and file a followup bug to add support for sign() to AsmJS, and Cc: me. If you like, you can put a FIXME comment in some sensible place, citing the followup bug number.

Just for reference: http://www.musicdsp.org/showone.php?id=206 It's hand-rolled SSE assembly and the results are low precision. All I took from this is that at least *someone* thinks this method is faster than pow().

> > Done, although I'm still checking for < 0. The idea is avoid flooding the > > math cache with arguments that lead to NaN. [...] > > Not worth it, I think: we don't want to punish code that's using log in its > useful domain to benefit code that's passing it negative numbers. The same > goes for a few others. Sounds good. Removing the code was trivial and didn't need any special handling (for NaN, etc) either. So in this sense it was totally worth it. > (Also, I think this simplifies the code considerably, eliminating one of the > layers.) It could, but I wonder if this would be the right moment to strip out such code. Has this been tested on other platforms? In some cases I've seen code like this: double js::math_log_impl(MathCache *cache, double x) { #if defined(SOLARIS) && defined(__GNUC__) if (x < 0) return js_NaN; #endif return cache->lookup(log, x); } so perhaps we should test first on these other platforms and make sure the code doesn't need any special handling? Also, this layered approach is already used by some existing code, even in trivial cases. See for instance: double js::math_cos_impl(MathCache *cache, double x) { return cache->lookup(cos, x); } This somehow comes related to my last comment: "One further comment: the new template function would allow us to use it with some existing unary math functions (tan, sqrt, etc). I presume this could be made as a separate bug id although it could also be changed within this patch. Please let me know the more appropriate approach." Would you advise ignoring this for now, perhaps filling a new bug to perform these changes? Or something else? I wouldn't mind taking care of it, as I feel familiar with the module. > > I'm wondering whether it would make sense to add suport for binary functions > > in MathCache::lookup() > > Maybe! But maybe not today. No problem :-) > > > > + * Compute cube root of a positive number. This code is based on Ken Turkowski's example code > > > > > > Interesting. Is this better than: > > > > > > return x > 0.0 > > > ? pow( x, 1.0 / 3.0) > > > : -pow(-x, 1.0 / 3.0); > > > > > > ? > > > > There is some loss of precision for small floats when using the above code. > > For example, Math.pow(1e-300, 1.0 / 3.0) = 1.0000000000000128e-100, that is > > approx. > 1e-14 of relative error. On the other hand the above algorithm > > gives the expected result (1e-100) > > > > But considering the current relative error is at 1e-12 then it doesn't make > > much of a difference. This also pointed out the fact that the test code was > > using assertEq() instead of assertNear() for such case (corrected now) > > Hmmmm. Precision is important, speed is important... > > At the time I wrote that comment, I thought pow() was probably faster. But > on second thought, we should just check. If you have time, please write a > quick C program to compute a bunch of cube roots, and see which > implementation is faster. I did a very simple test, comparing performance among two custom implementations (the one I submitted the first time, and the one based on pow()) and std cbrt(). Both custom implementations had similar performance, but the platform's one almost double it! So it's evident that a better implementation should be possible. I attached the simple test code. One of my outputs looks like this: $ ./pow_test Testing.................... Mean test #0: 0.678 # std cbrt() Mean test #1: 1.237 # cbrt() using pow() Mean test #2: 1.2 # cbrt() using Ken Turkowski's approach > If you prefer, we can check in the simpler code for now. I can file a > followup bug to investigate further. I think that would be a good option, as I'll be somehow busy these next few days. But please don't hesitate in CC'ing me in any related bug report you think I could help, as I'd like to complete the work over the following weeks. > > I chose this option. Besides this, I also added cases to AsmJS.cpp (although > > I had problems adding "sign" as this is not a std c++ function) > > > > I lack some time to get familiar with that code, so would appreciate some > > pointer about how to proceed with this and thus avoid the warning. > > Great. Please just remove the new mentions of "sign" from the AsmJS code, or > comment them out, and file a followup bug to add support for sign() to > AsmJS, and Cc: me. > > If you like, you can put a FIXME comment in some sensible place, citing the > followup bug number. Will do.

Removed unwanted special handling for ranges leading to NaN (made to avoid possible flooding of math cache) and added reference to bug id in AsmJS.cpp

Followup comment: I noticed I'm using args.rval().setNumber() in the template function. Considering we're dealing with doubles, would it make more sense (performance-wise) to just use setDouble()? Is there any unwanted side effect of doing so?

(In reply to David Caabeiro [:psior] from comment #51) > > (Also, I think this simplifies the code considerably, eliminating one of the > > layers.) > > It could, but I wonder if this would be the right moment to strip out such > code. Has this been tested on other platforms? In some cases I've seen code > like this: > > double > js::math_log_impl(MathCache *cache, double x) > { > #if defined(SOLARIS) && defined(__GNUC__) > if (x < 0) > return js_NaN; > #endif > return cache->lookup(log, x); > } You're right, let's leave it. I always forget the JITs want access to the math cache... > Would you advise ignoring this for now, perhaps filling a new bug to perform > these changes? Or something else? I wouldn't mind taking care of it, as I > feel familiar with the module. Since we're so close, let's land what we've got. > I attached the simple test code. One of my outputs looks like this: > > $ ./pow_test > Testing.................... > Mean test #0: 0.678 # std cbrt() > Mean test #1: 1.237 # cbrt() using pow() > Mean test #2: 1.2 # cbrt() using Ken Turkowski's approach OK. So the answer to my question "Is this better than pow(x, 1.0/3.0)?" is, yes! It's more precise and just as fast. So we should use the stdlib where available, and Ken Turkowski's approach elsewhere. As I said before, we can check in the simpler code for now.

David, this patch doesn't build on Windows. There are two problems. 1. The Visual C++ compiler, poor thing, can't cope with hypot for some reason. It says: > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/jsmath.cpp(1055) : > error C2084: function 'double hypot(double,double)' already has a body > > c:\tools\msvs10\vc\include\math.h(161) : see previous definition of 'hypot' > > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/jsmath.cpp(1077) : > error C3861: 'hypot': identifier not found Full log: https://tbpl.mozilla.org/php/getParsedLog.php?id=24441432&tree=Try&full=1#error0 The code in question: https://hg.mozilla.org/try/file/cfe7e5452639/js/src/jsmath.cpp#l1053 I grant that this makes no sense. I think I will fix this using a gruesome hack like #ifdef _WIN32 #define hypot js_hypot #endif or else add _hypot to the names we check for; haven't decided. 2. The other problem is that the code in ion/AsmJS.cpp doesn't work if the functions aren't provided by the system: > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src > /ion/AsmJS.cpp(3684) : error C2065: 'log2' : undeclared identifier > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src > /ion/AsmJS.cpp(3685) : error C2065: 'log1p' : undeclared identifier > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src > /ion/AsmJS.cpp(3686) : error C2065: 'expm1' : undeclared identifier etc. Feel free to fix problem #2, or both, or whatever. If you don't by Monday, then I will take a stab at it.

The Try Server run, fwiw: https://tbpl.mozilla.org/?tree=Try&rev=cfe7e5452639

(In reply to Jason Orendorff [:jorendorff] from comment #56) > David, this patch doesn't build on Windows. There are two problems. > > 1. The Visual C++ compiler, poor thing, can't cope with hypot for some > reason. It says: > > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/jsmath.cpp(1055) : > > error C2084: function 'double hypot(double,double)' already has a body Interesting. From what I read, it looks like hypot() became an inline function in latest VS releases and thus somehow failing the check. Perhaps an option would be adding this to the definition? #ifdef _WIN32 extern "C" __declspec(dllexport) #endif double hypot(double x, double y) { ... } You can find some related discussion here: http://stackoverflow.com/questions/6809275/unresolved-external-symbol-hypot-when-using-static-library > 2. The other problem is that the code in ion/AsmJS.cpp doesn't work if the > functions aren't provided by the system: > > > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src > > /ion/AsmJS.cpp(3684) : error C2065: 'log2' : undeclared identifier > > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src > > /ion/AsmJS.cpp(3685) : error C2065: 'log1p' : undeclared identifier > > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src > > /ion/AsmJS.cpp(3686) : error C2065: 'expm1' : undeclared identifier > > etc. > > Feel free to fix problem #2, or both, or whatever. If you don't by Monday, > then I will take a stab at it. The "problem" - which I'm actually thankful to have ;-) - is that at this moment I don't have any available Windows environment where to test these fixes. Does Mozilla have any of this, or at least some (restricted) access to this test server? Or something else? That would make things easier at least for me. I'm sorry for not being of better help, but please feel free to let me know the best way to move forward.

(In reply to David Caabeiro [:psior] from comment #58) > (In reply to Jason Orendorff [:jorendorff] from comment #56) > > 1. The Visual C++ compiler, poor thing, can't cope with hypot for some > > reason. It says: > > > e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/js/src/jsmath.cpp(1055) : > > > error C2084: function 'double hypot(double,double)' already has a body > > Interesting. From what I read, it looks like hypot() became an inline > function in latest VS releases and thus somehow failing the check. Ah, I see, the check is really depending on the linker to figure out if the function exists or not. Silly autoconf. > The "problem" - which I'm actually thankful to have ;-) Same here! > - is that at this > moment I don't have any available Windows environment where to test these > fixes. Does Mozilla have any of this, or at least some (restricted) access > to this test server? Oh - yes, you *can* get a test server. The first step would be to file a bug like bug 886831 requesting Level 1 commit access. But for the moment, let me try troubleshooting these problems. For problem 1, I made this change: > dnl Check for math functions > dnl ======================================================== >-AC_CHECK_FUNCS(log2 log1p expm1 sqrt1pm1 acosh asinh atanh hypot trunc cbrt) >+AC_CHECK_FUNCS(log2 log1p expm1 sqrt1pm1 acosh asinh atanh trunc cbrt) >+ >+dnl Check specially for hypot, since it is an inline function in some MSVC versions. >+dnl (Plain AC_CHECK_FUNCS checks that we can link against the symbol without having >+dnl #included any relevant header, so it doesn't see inlines.) >+AC_CACHE_CHECK(for hypot, >+ ac_cv_have_hypot, >+ [AC_TRY_LINK([#include <math.h>], >+ [double result = hypot(3.0, 4.0);], >+ ac_cv_have_hypot="yes", >+ ac_cv_have_hypot="no")]) >+if test "$ac_cv_have_hypot" = "yes"; then >+ AC_DEFINE(HAVE_HYPOT) >+fi For problem 2, I'm factoring out the AsmJS changes for a later bug. I think I will also split the non-AsmJS Ion changes into a second patch. On a second reading, I'm pretty sure this is no good: >+ if (native == js::math_hypot) >+ return inlineMathFunction(callInfo, MMathFunction::Hypot); We need something more like inlineMathAtan2() for that case. So I'm removing IonMonkey hypot() support and I'll file a follow-up bug to add it. We'll see what the try server says about all this later today. > Or something else? That would make things easier at > least for me. I'm sorry for not being of better help, but please feel free > to let me know the best way to move forward. You've been a tremendous help. No worries.

(In reply to Jason Orendorff [:jorendorff] from comment #59) > Ah, I see, the check is really depending on the linker to figure out if the > function exists or not. Silly autoconf. Except it turns out none of the tests run on win32 at all, because of SKIP_COMPILER_CHECKS. I don't want to know the underlying reason for that, so I'll just AC_DEFINE(hypot). With that, we have a crash on 64-bit Ubuntu debug, at least... I'll look into it as soon as I get a chance. https://tbpl.mozilla.org/?tree=Try&rev=c095579a62ee

Jason, that crash is due to a failed assertion that for some reason I left in the current patch, and which should have been removed (they were reminiscences of the previous code that made explicit tests for NaN, Infinity, etc) First MOZ_ASSERT is in line 921 (acosh) <-- current crash in Ubuntu debug Second MOZ_ASSERT is in line 1010 (atanh) Removing both lines will pass the tests and should pass the debug build. Sorry about that. Let me know if you prefer me to submit a new corrected patch, or if it's easier for you just to do it yourself. BTW, today I started looking for a better implemention for cbrt. Got some promising initial results but will need to do some more testing. Will let you know as soon as I get something worth sharing.

Thanks, David. I'll delete the assertions and press on. We fail this test: assertNear(Math.cosh(1e5), 1.4033316802130615); but it's the test that's bogus. According to WolframAlpha, the mathematical answer here is about 1.4033316802130615 × 10^43429, so we should (and do) return Infinity.

I think assertNear() is not picky enough. I wrote a version of it that prints a message when the two values being compared are not either exactly equal or nearest neighbors among the representable IEEE 754 floating-point values. Results using MacOSX library functions: acosh.js - 5 imprecise results. Off by: [4, 8, 8, 4, 3460]. 3460 is a lot! But I think it is just a bug in the test: assertNear(Math.acosh(1.0000000001), 0.00001414213620867); our answer: 0.000014142136208675862 cosh.js and sinh.js - A great many slightly imprecise results, error <= 14. Perhaps it is the test data that is imprecise. Results using the fallback functions (that is, HAVE_CBRT and friends all set to 0): atanh.js - 6 slightly imprecise results, error ranging from 2 to 7. acosh.js - 12 imprecise results. Off by: [2, 5, 6, 7, 3, 26, 3, 14, 35, 69, 213, 42, 5, 7, 7, 5, 3460] Again, that 3460 is a bug in the test. asinh.js - 12 imprecise results. Off by: [140, 825, 515, 467, 240, 95, 56, 16, 7, 4, 3, 3] cosh.js and sinh.js - A great many slightly imprecise results, error <= 14. Perhaps it is the test data that is imprecise. cbrt.js - Just 2 imprecise results: Math.cbrt(1e-300) is 1.0000000000000128e-100 There are 100 representable values between this and the correct answer. The other error is just the negative of this. So our precision is pretty good, and allowing that 3460 to slip through is a bug in assertNear().

I was wrong about this being a bug in the test: > assertNear(Math.acosh(1.0000000001), 0.00001414213620867); > our answer: 0.000014142136208675862 correct answer: 0.0000141421356236131 :-\

Sorry, I was wrong about that being wrong. > assertNear(Math.acosh(1.0000000001), 0.00001414213620867); > our answer: 0.000014142136208675862 correct answer: 0.000014142136208675862 My mistake was computing a precise approximation of acosh(1.0000000001) rather than a precise approximation of acosh(the nearest IEEE-754-representable value to 1.0000000001) The latter is the correct answer.

(In reply to Jason Orendorff [:jorendorff] from comment #63) > I think assertNear() is not picky enough. Indeed, current implementation doesn't handle cases such as Infinity correctly. The test "passes" because of "error > Math.abs(actual) * 1e-12" returning false when both error = actual = Infinity. I have a busy day today, but will connect to IRC either tonight or tomorrow so we can discuss the pending issues if you wish.

Filed bug 892671 to improve test precision. Here is my latest Try run. https://tbpl.mozilla.org/?tree=Try&rev=129ee8f37386 In an earlier run, the hypot test failed on Windows because hypot(Infinity, NaN) was returning NaN rather than Infinity. So I changed js::math_hypot to call math_hypot_impl rather than hypot directly, and added an #ifdef to math_hypot_impl. Would love to land this today.

Please find attached this patch that should improve precision for all inverse hyperbolic functions. I also included the latest implementation of cbrt(). As you'll notice, it's been more a matter of defining new ranges for acosh() and including more terms in the log1p() and expm1() series expressions, on which these functions rely on. Unfortunately it seems that some stdlib math functions aren't as precise as we'd like them to be. We'll need to see how these functions behave on other platforms. The new cbrt() is based on Newton's method and in my tests proved to be faster than both pow-based and Turkowski's and providing good precision. For the hyperbolic test units, I've been able to pass them redifining tolerancy to 8, 16 and 32 for atanh(), asinh() and acosh() respectively.

Regarding log1p() and expm1() implementations, [:ehoogeveen] righly suggested using these expressions instead: log1p: double z = x * (1.0 + x * (-1.0 / 2.0 + x * (1.0 / 3.0 + x * (-1.0 / 4.0 + x * (1.0 / 5.0 + x * (-1.0 / 6.0 + x * (1.0 / 7.0))))))); expm1: double z = x * (1.0 + x * (1.0 / 2.0 + x * (1.0 / 6.0 + x * (1.0 / 24.0 + x * (1.0 / 120.0 + x * (1.0 / 720.0 + x * (1.0 / 5040.0))))))); The original implementation was a series expansion of order 3 so this wasn't an issue, but now that we moved to 7th grade it does make a difference. Please feel free to replace the original code with these.

Comment on attachment 764059 [details] [diff] [review] Fourth iteration Review of attachment 764059 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/configure.in @@ +2730,5 @@ > > + > +dnl Check for math functions > +dnl ======================================================== > +AC_CHECK_FUNCS(log2 log1p expm1 sqrt1pm1 acosh asinh atanh hypot trunc cbrt) The list of functions here should be enclosed in []. Also, on my Linux box, this check doesn't actually find any of the functions because the autoconf link test doesn't have -lm. Adding "AC_CHECK_LIB(m, cos)" immediately before this AC_CHECK_FUNCS line fixes it (though I'm not an autoconf expert so I don't know if this is the best solution).

https://hg.mozilla.org/mozilla-central/rev/348b2ba27515 https://hg.mozilla.org/mozilla-central/rev/ffc604b74974

I just noticed that the patch applied to the repository wasn't the latest version. For instance, it doesn't contain the latest implementations of log1p(), expm1() and cbrt(). Could someone advise on how to proceed to fix this?

(In reply to David Caabeiro [:psior] from comment #72) > I just noticed that the patch applied to the repository wasn't the latest > version. For instance, it doesn't contain the latest implementations of > log1p(), expm1() and cbrt(). > > Could someone advise on how to proceed to fix this? Please create a patch that applies on top of what has already landed attach it and ask for review again. I've reopened the bug to indicate that more work needs to be done.

Please find the patch containing the latest (unreviewed) code here.

Comment on attachment 779974 [details] [diff] [review] Update for current patch with latest code (asinh, acosh, cbrt) Review of attachment 779974 [details] [diff] [review]: ----------------------------------------------------------------- I'm so not qualified to review this.

Hello, thanks for good work on this can i ask? with current algorithm for expm1 (!HAVE_EXPM1), expm1 is not monotonic Math.expm1(-1e-2) === -0.009950166250831893 Math.expm1(-0.009999999999999998) === -0.009950166250831945 so Math.expm1(-1e-2) > Math.expm1(-0.009999999999999998) is this ok?

(back from vacation) This bug is done. Next up is bug 892671. We'll open new bugs for further improvements after that, including the ones in David's patch. 4esn0k, you're right, that's bad. Filed bug 897634. I think this means exp() itself is not monotonic on your platform! Is Math.exp(-1e-2) > Math.exp(-0.009999999999999998) true for you? Please comment in the new bug.

Combine these ES6 support into one release note as "New java script features" as a developer note. Check in https://developer.mozilla.org/en-US/docs/Web/JavaScript/ECMAScript_6_support_in_Mozilla for a list of all these.

Comment on attachment 779974 [details] [diff] [review] Update for current patch with latest code (asinh, acosh, cbrt) File a new bug.

Comment on attachment 797379 [details] [diff] [review] math-lib-detect.patch File a new bug.

Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.

Comment on attachment 797379 [details] [diff] [review] math-lib-detect.patch This is now filed under bug 910877.

Hi guys. Does this still need qa manual verification considering the existing automated tests ?

It does not, no.

Thanks Till. qa- based on comment 85.

Added to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25#JavaScript New reference docs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/acosh https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/asinh https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/atanh https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/cbrt https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/cosh https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/expm1 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/hypot https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/log1p https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/log10 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/log2 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/sign https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/sinh https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/tanh https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/trunc As always, feel free to review and/or to edit pages in the MDN wiki.