Skip to content
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

Math.sqrt: add a test with exact input-output expectations #4188

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

michaelficarra
Copy link
Member

@michaelficarra michaelficarra commented Aug 6, 2024

Tests for this ecma262 PR: tc39/ecma262#3345

We could additionally test chosen interesting/"edge-case" inputs, but I wouldn't know how to discover those. @anba @ptomato suggested that you may have some recommendations for these or for inputs that might have different results using known sqrt approximation strategies.

Input-output pairs were generated by the following script:

let taInt32 = new Uint32Array(2);
let taFloat64 = new Float64Array(taInt32.buffer);
function intsToFloat(low, high) {
    taInt32[0] = low;
    taInt32[1] = high;
    return taFloat64[0];
}
function floatToInts(f) {
  taFloat64[0] = f;
  return [taInt32[0], taInt32[1]];
}

const DOUBLE_BIAS = 1024;

let results = new Map;
for (let tests = 0; tests < 1000; ++tests) {
  let [low, high] = crypto.getRandomValues(new Uint32Array(2));
  high &= 0b0_00000000000_11111111111111111111;
  high |= (((Math.random() < 0.5 ? -1 : 1) * tests + DOUBLE_BIAS) << 20);
  let candidate = intsToFloat(low, high);
  results.set(candidate, Math.sqrt(candidate));
}

console.log(JSON.stringify([...results.entries()]));

@michaelficarra michaelficarra requested a review from a team as a code owner August 6, 2024 00:47
@michaelficarra
Copy link
Member Author

I'm not sure why esmeta is getting a stack overflow.

@ioannad
Copy link
Contributor

ioannad commented Aug 8, 2024

I'm not sure why esmeta is getting a stack overflow.

@michaelficarra FWIW I just tried running the test in my local clone of esmeta pointing to the same HEAD as in the github checks here and the test passed. I restarted the test to see if it fails again.

@michaelficarra
Copy link
Member Author

@ioannad That seems to have worked. It no longer gets a stack overflow, but it does fail. It opaquely fails, so I'm not sure which assertion caused the failure. Also, I'm pretty sure the test should pass. I ran the test manually in V8 and it passed there.

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tentative approval pending the merge of the spec PR and some understanding of what's going on with esmeta

@michaelficarra
Copy link
Member Author

@Ms2ger Spec PR is merged. esmeta is getting a stack overflow again after rebase. 🤷‍♂️

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 20, 2024

@doehyunbaek @Zaid-maker either of you able to shed some light on esmeta's stack overflow? Otherwise I'll probably merge this anyway in the next few days

@jhnaldo
Copy link
Contributor

jhnaldo commented Aug 20, 2024

Hi all, I'm the maintainer of ESMeta. It happens because of the limited stack size of the released executable (bin/esmeta). So, I added the option -Xss4m for released executables in es-meta/esmeta#254. I'm sorry about this issue. I believe the latest version of ESMeta v0.4.2 will fix this problem. If it does not work, please let me know.

@jhnaldo
Copy link
Contributor

jhnaldo commented Aug 20, 2024

In addition, we checked that the fixed version passed the test as follows:

$ esmeta test262-test -test262-test:log results.js
========================================
 extract phase
----------------------------------------
========================================
 compile phase
----------------------------------------
========================================
 build-cfg phase
----------------------------------------
========================================
 test262-test phase
----------------------------------------
[Interpreter] Logging into /Users/naldo/project/esmeta/logs/test262/log ...
[Interpreter] Logging finished
PASS

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 20, 2024

Thanks for the swift response! I've opened #4202 to update our CI, and then we can go ahead here when that is landed.

@michaelficarra
Copy link
Member Author

@Ms2ger esmeta test passes now. Should be good to go.

@ptomato ptomato merged commit 22967c5 into main Aug 20, 2024
8 checks passed
@ptomato ptomato deleted the sqrt branch August 20, 2024 17:29
@anba
Copy link
Contributor

anba commented Aug 21, 2024

@anba @ptomato suggested that you may have some recommendations for these or for inputs that might have different results using known sqrt approximation strategies.

I've checked Bugzilla for sqrt precision bugs, but I didn't find any. Probably because the implementation uses std:sqrt, which is known to be correct. Comparing the inputs against naive implementations (*) shows some mismatches, so I guess the randomly generated inputs are good enough.

(*) For example a naive implementation of Goldschmidt's algorithm in JS shows ~380 mismatches, with a maximum error of 2 ULP.

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.

6 participants