2037094 - Use fdlibm:pow instead of std:pow for math operations

Status: NEW

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tom Ritter [:tjr]]

Telemetry analysis showed some divergence in math operations that, upon analysis, were tracked down to a usage of std:pow that we had missed when we cut virtually everything over to fdlibm. Cutting this final one over the fdlibm will unify the divergence in some users - specifically ARM Android users on API <= 28 because these users are using an older or different libc that doesn't match most other things in the world. So while the change will affect everyone, only those users are expected to have a different value at the margins.

Comment 1

[Tom Ritter [:tjr]]

I know from microbenchmarks that this is slower, but I don't think it's going to move the needle much in a real benchmark, going to run and measure.

https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=42754

Comment 2

[Tom Ritter [:tjr]]

Attachment: WIP: Bug 2037094 - Use fdlibm for Math.pow to eliminate Android API <=28 ARM divergence

Math.pow currently calls std::pow, which routes through the platform libm.
On Android API <= 28 ARM devices with unpatched OEM bionic, std::pow is
FreeBSD's e_pow.c which returns 1.9275814160560204e-50 for
Math.pow(Math.PI, -100), versus fdlibm's 1.9275814160560206e-50 (a 1-ULP
difference).

Replace the std::pow call with fdlibm_pow in both js::powi (integer-
exponent fallback) and js::ecmaPow (generic exponent path). All
surrounding fast paths (small integer powers, square root, special-case
infinities/NaN/zero) are unchanged and remain bit-deterministic.

Includes a jit-test pinning the new behavior on so that any future libm
divergence regresses visibly.

Comment 3

[André Bargull [:anba]]

pow was intentionally not moved over to fdlibm because of benchmark regressions, see bug 933257.

I wonder if we should try out Core-Math, maybe its performance for pow is better than fdlibm?

Comment 4

[Nicolas B. Pierron [:nbp]]

Arai, I know what you did last decade!
I guess this one might be for you.

Comment 5

[Tom Ritter [:tjr]]

Thanks for that point, I knew sunspider would be relevant, but I've added kraken and will see how things look.

Early results are showing the expected math-partial-sums regression (2.56 ms -> 3.17 ms on Windows, 4.37 ms -> 5.76 ms on Linux).

I am loathe to add more complicated routing into our math code - I had actually hoped we could rip out a lot of the complexity, but I could put this behavior behind the existing fdlibm pref, so it wouldn't apply universally but only when fingerprinting protection is applied or limit it to affected Android clients.

Awaiting results form Kraken...

Comment 6

[André Bargull [:anba]]

(In reply to Tom Ritter [:tjr] from comment #5)

[...] limit it to affected Android clients.

Windows also has a less precise std::pow implementation: https://searchfox.org/firefox-main/rev/6087999d87e808ec39f260293afd6cc27b6c5944/js/src/tests/non262/Math/pow-approx.js#339. (At least that was the case four years ago.)

Comment 7

[Tooru Fujisawa [:arai]]

If the performance regression is within some acceptable range, then I think it's okay to replace the implementation unconditionally.
There are some previous score in bug 933257 comment #137 etc, also with micro benchmark.

Then, If it's unconditionally changed, please remove the testing function that exposes fdlibm pow, and also rewrite its consumers.

https://searchfox.org/firefox-main/rev/6087999d87e808ec39f260293afd6cc27b6c5944/js/src/builtin/TestingFunctions.cpp#11560-11566

static const JSFunctionSpecWithHelp FdLibMTestingFunctions[] = {
    JS_FN_HELP("pow", FdLibM_Pow, 2, 0,
    "pow(x, y)",
    "  Return x ** y."),

    JS_FS_HELP_END
};

https://searchfox.org/firefox-main/search?q=fdlibm.pow&path=&case=false&regexp=false

Comment 8

[Tom Ritter [:tjr]]

Windows also has a less precise std::pow implementation: https://searchfox.org/firefox-main/rev/6087999d87e808ec39f260293afd6cc27b6c5944/js/src/tests/non262/Math/pow-approx.js#339. (At least that was the case four years ago.)

For whatever reason, Windows is not showing up as a distinct population in the telemetry submissions.

I'm upset to report that kraken seems to yield quite painful regressions: imaging-darkroom is >50% across the board. As when we switched to fdlibm for other functions, it's got strange behaviors though - on mac, kraken's audio-dft and audio-fft get better by 20+%, but then imaging-gaussian-blur worse by 15%.

I'm going to scope the patch to Android only.