Skip to content

Add BigInt64Array shim for Safari 14 #17103

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

Merged
merged 50 commits into from
Jun 27, 2022

Conversation

hoodmane
Copy link
Collaborator

@hoodmane hoodmane commented May 30, 2022

As discussed in #16693, -s WASM_BIGINT requires both BigInt (present since Safari 14.0 in 90.91% of users' browsers) and BigInt64Array (present since Safari 15.0 in 87.67% of users' browsers). We want to target Safari 14.0 as our minimal supported version, so we need this shim to work.

I didn't shim all of the TypedArray APIs, just the ones that I think are most important. It would be pretty easy to shim the rest of them if you want.

@hoodmane hoodmane force-pushed the bigint64array-safari14-shim branch from c59631b to 9e78b25 Compare May 30, 2022 01:41
@hoodmane hoodmane mentioned this pull request May 30, 2022
1 task
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Is this fast enough to be useful?

We usually put polyfills under src/polyfill/, see examples there. It should also be gated behind a check to avoid the constant code size overhead. Perhaps it could check MIN_SAFARI_VERSION perhaps.

@hoodmane
Copy link
Collaborator Author

Is this fast enough to be useful?

I dunno, probably? Do you have a specific benchmark in mind? The main goal is to be able to use -sWASM_BIGINT so we can avoid patches like #16693, but without dropping support for Safari 14 entirely. It's okay if some code paths are a bit slow for Safari 14 I just don't want it to be raising a ReferenceError and failing on startup.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

This looks good to me as it stands.

We have not used globalThis in any of our other polyfills that I know of... but if it works in this case I guess that is fine.

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jun 1, 2022

Oh I can just use var because it doesn't respect scopes:

function f(){
  if(true) {
    var x = 2;
  }
  console.log(x); // 2
}
console.log(x);  // ReferenceError

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jun 1, 2022

This raises:

parseTools.js preprocessor error in preamble.js:289: "#if !POLYFILL"!
Internal compiler error in src/compiler.js!
Please create a bug report at https://github.com/emscripten-core/emscripten/issues/
with a log of the build and the input files used to run. Exception message: "ReferenceError: POLYFILL is not defined" | ReferenceError: POLYFILL is not defined

@hoodmane
Copy link
Collaborator Author

hoodmane commented Jun 1, 2022

Oh that may only be a problem in the backport.

@hoodmane
Copy link
Collaborator Author

Okay I added a fairly thorough test suite and caught a few bugs. There are still a few cases where the real thing throws an error but my shim just does something, but I don't think it matters.

@hoodmane
Copy link
Collaborator Author

Can this be merged?

@hoodmane
Copy link
Collaborator Author

Bump

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

For the other polyfils we have tended to test them in place ? By running emscripten -sXXX_VERSION=YY to enable them. But this direct testing you have implemented seems reasonable too.

@hoodmane
Copy link
Collaborator Author

Bump

@kripken
Copy link
Member

kripken commented Jun 24, 2022

@sbc100 , any remaining comments from you? I think everything was resolved but didn't want to merge before asking.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 27, 2022

lgtm

@kripken kripken merged commit 7aa4f64 into emscripten-core:main Jun 27, 2022
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