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

Add support for BigInts and BigInt-based TypedArrays #184

Merged
merged 35 commits into from
Apr 22, 2022
Merged

Add support for BigInts and BigInt-based TypedArrays #184

merged 35 commits into from
Apr 22, 2022

Conversation

j-f1
Copy link
Member

@j-f1 j-f1 commented Apr 9, 2022

Fixes #56. Depends on #183

In the JavaScriptKit package, this PR:

  • adds a JSBigInt class that makes it easy to convert between BigInt and Int64/UInt64 values.
    • the Wasm-JS API assumes all i64 values are signed, so I have to hack around that behavior for UInt64 values
    • It would be nice to support standard operations (+-*/ etc) on JSBigInt instances; however, we currently don’t have a way to invoke arbitrary operators in JS from Swift, and there are no function-based equivalents of the operators that we could access instead.
  • Automatically casts BigInt values to the requested integer type when using .fromJSValue() (this may cause a crash if the BigInt is outside of the bounds of the requested type, I think)
  • Removes support for converting Int64 and UInt64 to JSValue

In addition, a new package (tentatively named JavaScriptKit_I64) includes:

  • Support for JSTypedArray<Int64> and JSTypedArray<UInt64>
  • Support for converting Int64 and UInt64 to a BigInt JSValue
    • they can still be constructed from a regular number; should I add a way to convert them to a Double value?
  • Support for constructing any integer type from a BigInt JSValue
  • Support for getting the int64Value and uInt64Value of a JSBigInt
    • …should this API be replaced with Int64(bigInt)/UInt64(bigInt)?
  • Support for constructing a JSBigInt from an Int64 or UInt64

also, it:

  • changes the integer/float conformances to ConstructibleFromJSValue to reduce boilerplate code (using a Swift 5.5 feature)
  • fixes JSSymbol’s JSValue conversion APIs (oops!)
  • changes the type for the internal setTimeout function to be a Double (since at least Node.js doesn’t support BigInt timeouts)
  • removes JSValueKind.invalid since it is unused
  • Uses an assertNever helper function in TypeScript to ensure switches are exhaustive

@github-actions
Copy link

github-actions bot commented Apr 9, 2022

Time Change: +279.25ms (3%)

Total Time: 8,247.5ms

Test name Duration Change
Object heap/Increment and decrement RC 2,714.75ms +226.25ms (8%) 🔍
ℹ️ View Unchanged
Test name Duration Change
Serialization/Write JavaScript number directly 170.25ms +0.75ms
Serialization/Write JavaScript string directly 175.5ms +2.25ms (1%)
Serialization/Swift Int to JavaScript 2,541ms +19.5ms (0%)
Serialization/Swift String to JavaScript 2,646ms +30.5ms (1%)

performance-action

@j-f1 j-f1 marked this pull request as ready for review April 9, 2022 16:57
@j-f1 j-f1 requested a review from a team April 9, 2022 16:57
Base automatically changed from jed/re-add-symbol to main April 10, 2022 04:24
@MaxDesiatov
Copy link
Contributor

This needs conflicts to be resolved after JSSymbol merge. That would reduce the diff as well, making it easier to review. Thanks!

@kateinoigakukun
Copy link
Member

kateinoigakukun commented Apr 10, 2022

Can we have a separate module for BigInt support instead of having a new feature flag JAVASCRIPTKIT_WITHOUT_BIGINTS?
I think it would be more straightforward than using ifdefs. Unlike JAVASCRIPTKIT_WITHOUT_WEAKREFS, BigInt is relatively independent of the whole runtime mechanism.

BTW symbol support should be also enabled by an explicit option? We need to define the minimum supported ECMAScript edition, and guard for new features introduced after the edition.

IntegrationTests/lib.js Outdated Show resolved Hide resolved
Sources/JavaScriptKit/ConvertibleToJSValue.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit/ConvertibleToJSValue.swift Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Apr 16, 2022

Time Change: -39ms (0%)

Total Time: 19,157ms

Test name Duration Change
Serialization/Write JavaScript number directly 454ms +36ms (7%) 🔍
Serialization/Write JavaScript string directly 471ms -28ms (5%)
View Unchanged
Test name Duration Change
Serialization/Swift Int to JavaScript 6,142ms +26ms (0%)
Serialization/Swift String to JavaScript 6,437ms +144ms (2%)
Object heap/Increment and decrement RC 5,652ms -216ms (3%)

@j-f1 j-f1 requested a review from a team April 16, 2022 13:55
@j-f1
Copy link
Member Author

j-f1 commented Apr 16, 2022

I would especially appreciate feedback on the new package name (JavaScriptKit_I64)

Sources/JavaScriptKit/XcodeSupport.swift Outdated Show resolved Hide resolved
Runtime/src/types.ts Outdated Show resolved Hide resolved
Sources/JavaScriptKit/JSValue.swift Outdated Show resolved Hide resolved
Runtime/src/types.ts Outdated Show resolved Hide resolved
@kateinoigakukun
Copy link
Member

Module name candidates:

  • JavaScriptKitBigIntIntegrationSupport
  • JavaScriptBigIntIntegrationSupport
  • JavaScriptKitBigIntSupport
  • JavaScriptBigIntSupport

Note: The proposal name was JS-BigInt-integration

@MaxDesiatov
Copy link
Contributor

How's about JavaScriptKitI64Support?

@MaxDesiatov
Copy link
Contributor

Ah, sorry, just saw your link to the proposal. 😅 Then I personally vote for JavaScriptBigIntSupport, this seems the least verbose, but still obvious enough for me.

@j-f1
Copy link
Member Author

j-f1 commented Apr 16, 2022

For JavaScriptBigIntSupport, my concern is that people won’t think BigInts are supported at all by vanilla JSKit, but maybe that isn’t such a big deal and we should just be clear in the README in which cases you need it.

@MaxDesiatov
Copy link
Contributor

Is this ready for review by any chance?

@j-f1
Copy link
Member Author

j-f1 commented Apr 19, 2022

Yep!

@j-f1 j-f1 requested a review from a team April 19, 2022 12:00
@kateinoigakukun
Copy link
Member

I prefer JavaScriptBigIntSupport with README note. Other things are LGTM

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits. Great work! 👏

Package.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit/FundamentalObjects/JSBigInt.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit/FundamentalObjects/JSObject.swift Outdated Show resolved Hide resolved
Sources/JavaScriptKit_I64/XcodeSupport.swift Outdated Show resolved Hide resolved
j-f1 and others added 3 commits April 22, 2022 09:50
@j-f1 j-f1 merged commit 34bf9e1 into main Apr 22, 2022
@j-f1 j-f1 deleted the jed/bigint branch April 22, 2022 14:59
@MaxDesiatov
Copy link
Contributor

I wonder if we have to disable Safari WasmTransformer pass when JavaScriptBigIntSupport target is linked? I'm currently seeing WebAssembly.Module doesn't validate: control flow returns with unexpected type. I32 is not a I64, in function at index 89699 error when playing with swiftwasm/WebAPIKit#18

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented May 3, 2022

I can also confirm that the issue is not reproducible with 95d0c4c, which is prior to the merge commit for this PR.

@j-f1
Copy link
Member Author

j-f1 commented May 3, 2022

That sounds good! Would we need to do anything on the JS side to correctly handle the previous APIs that now return BigInts?

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented May 3, 2022

I haven't verified yet if disabling the pass fixes the issue with the latest JSKit main, this is only my current theory that could explain the breakage.

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.

BigInt support
3 participants