Skip to content

fix: prevent powi opt in js engine only when exponent <= -2 #2920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HerrCai0907
Copy link
Member

@HerrCai0907 HerrCai0907 commented May 15, 2025

When exponent > 0, powi opt will not cause loss of precision at current nodejs version

@HerrCai0907 HerrCai0907 force-pushed the fix/node-version-bug branch 3 times, most recently from b404098 to 4d9b05b Compare May 15, 2025 08:04
@HerrCai0907 HerrCai0907 force-pushed the fix/node-version-bug branch from 4d9b05b to a022591 Compare May 15, 2025 08:05
@HerrCai0907 HerrCai0907 changed the title fix: avoid powi opt in js engine only when exponent <= 2 fix: prevent powi opt in js engine only when exponent <= 2 May 15, 2025
@HerrCai0907 HerrCai0907 changed the title fix: prevent powi opt in js engine only when exponent <= 2 fix: prevent powi opt in js engine only when exponent <= -2 May 15, 2025
@MaxGraey
Copy link
Member

Hmm, but how about of rest browsers? AS may run in different browsers. Besides, there is no guarantee that even in the next versions of node this speculation will not appear again

@CountBleck
Copy link
Member

Is the current behavior causing any problems?

@MaxGraey
Copy link
Member

MaxGraey commented May 15, 2025

But it still can be slightly optimized I gues to:

if (Math.trunc(y) == y && Math.abs(y) > 2 && Math.abs(y) < 100) {
   ...
}
return Math.pow(x, y);

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented May 16, 2025

Is the current behavior causing any problems?

In latest version, it will get this result

x = 2
y = 2
Math.pow(x, y - 0.5) * Math.pow(x, 0.5)  // 4.000000000000001

It the reason why #2917 CI failed

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented May 16, 2025

Hmm, but how about of rest browsers? AS may run in different browsers. Besides, there is no guarantee that even in the next versions of node this speculation will not appear again

It is meaningless actually, change Math.pow(x,y) to Math.pow(x, y + 0.5) / Math.pow(x, 0.5) also introduce some calculation errors because of loss of precision during multiple calculation.

IMO, It is impossible to keep determinism between different js engine except introducing high precision pow lib.
WDYT? should we introduce this kind of deps? @MaxGraey @CountBleck

@CountBleck
Copy link
Member

CountBleck commented May 16, 2025

If there's still a (potential) need for this function, is there any other working alternative? Throwing an idea out: Math.exp(y * Math.log(x)), but I don't know if that's any more precise or useful than what we have now... (edit: y is an integer, but not necessarily x, so precision could be lost for sure)

@HerrCai0907
Copy link
Member Author

according to https://tc39.es/ecma262/#sec-numeric-types-number-exponentiate. Math.pow only required to be precision as much as possible. To be honest, I think it is hard to make everything same except not using IEEE754 floating number (software implement).
Modifying this function is just like debug for current test cases which does not make any sense because the precision loss can be happened everywhere, associative law, arithmetically equivalent operations, .... everything will introduce new precision loss.
For me, I think we should remove the whole function totally and use Math.pow directly and don't consider any corner cases like (10 ** -5). Or introducing a library like decimal.js to do high precision floating calculation.

@CountBleck
Copy link
Member

I'm also tempted to say "throw this function out", because I can also see this imprecise behavior with integer exponents on Python and Java (tested via jshell). We should therefore try to be bug-for-bug compatible with all these tools, which includes V8 and such. I should probably test if this imprecision is also present in C...

@MaxGraey
Copy link
Member

The best way I see, and the way I originally thought, is to compile the pow from std to external wasm npm module and include it as dep for the js version of the compiler (since the wasm version will use the necessary implementation anyway).

@CountBleck
Copy link
Member

What I was thinking: shouldn't we fix our Math.pow implementation to be compatible with everyone else's?

Even C exhibits this behavior:

#include <math.h>
#include <stdio.h>

int main(int argc, char* argv[]) {
    printf("pi: %e\n", M_PI);
    double regular = pow(M_PI, 210);
    double accurate = pow(M_PI, 209.5) * pow(M_PI, 0.5);
    printf("regular: %.18e, accurate: %.18e, equal: %d\n", regular, accurate, regular == accurate);
    return 0;
}

I get this output:

~ $ ./a.out
pi: 3.141593e+00
regular: 2.520422023118315886e+104, accurate: 2.520422023118315568e+104, equal: 0

And testing these values in Node.js too:

> Math.PI ** 210 === 2.520422023118315886e+104
true
> Math.PI ** 210 === 2.520422023118315568e+104
false

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented May 19, 2025

The best way I see, and the way I originally thought, is to compile the pow from std to external wasm npm module and include it as dep for the js version of the compiler (since the wasm version will use the necessary implementation anyway).

what do you think about to introduce 3rd lib to ensure mathematical correctness? IMO it can ensure compliance with ECMA specifications to the greatest extent possible.

According to spec:

An implementation-approximated facility is one that defers its definition to an external source while recommending an ideal behaviour. While conforming implementations are free to choose any behaviour within the constraints put forth by this specification, they are encouraged to strive to approximate the ideal. Some mathematical operations, such as Math.exp, are implementation-approximated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants