Add support for BigInts and BigInt-based TypedArrays by j-f1 · Pull Request #184 · swiftwasm/JavaScriptKit

j-f1 · 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 · 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}

MaxDesiatov · Apr 10, 2022

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

kateinoigakukun · 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.

github-actions · 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 · Apr 16, 2022

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

kateinoigakukun · Apr 16, 2022

Module name candidates:

JavaScriptKitBigIntIntegrationSupport
JavaScriptBigIntIntegrationSupport
JavaScriptKitBigIntSupport
JavaScriptBigIntSupport

Note: The proposal name was JS-BigInt-integration

MaxDesiatov · Apr 16, 2022

How's about JavaScriptKitI64Support?

MaxDesiatov · Apr 16, 2022

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 · 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 · Apr 19, 2022

Is this ready for review by any chance?

j-f1 · Apr 19, 2022

Yep!

kateinoigakukun · Apr 19, 2022

I prefer JavaScriptBigIntSupport with README note. Other things are LGTM

MaxDesiatov

LGTM, just a few nits. Great work! 👏

Co-Authored-By: Max Desiatov <max@desiatov.com>

MaxDesiatov · May 3, 2022

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 · 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 · 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 · 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.

j-f1 marked this pull request as ready for review April 9, 2022 16:57

j-f1 requested a review from a team April 9, 2022 16:57

j-f1 force-pushed the jed/bigint branch from 484c784 to 4b6ff62 Compare April 9, 2022 17:36

Base automatically changed from jed/re-add-symbol to main April 10, 2022 04:24

j-f1 added 17 commits April 10, 2022 08:08

Initial BigInt support

fa93b91

Switch to const enums

3ca1539

Add JSBigInt/clamped method, fix overload

bf3bac3

Convert BigInts into Swift ints automatically

8c02c79

Add support for JAVASCRIPTKIT_WITHOUT_BIGINTS

814774b

Allow Int64/UInt64-based arrays

0bcb359

Don’t use BigInt literal for backwards compat

d1543fc

bump lib version

f8838eb

long long?

26b51c7

document JAVASCRIPTKIT_WITHOUT_BIGINTS flag

4dd4aae

check compatibility workflow builds all variants

cd3c076

Increase error stack trace limit

76303b9

Use correct type for setTimeout in JavaScriptEventLoop

c356aba

Add symbol support to runtime’s JSValue.decode

6f150ad

remove JavaScriptValueKindInvalid since neither side produces it

6257d35

use assertNever to ensure runtime switch statements are kept up to date

203844f

BigInt tests

9d066f9

j-f1 force-pushed the jed/bigint branch from 4b6ff62 to 9d066f9 Compare April 10, 2022 12:08

j-f1 added 4 commits April 10, 2022 08:11

Revert changes to README

1fdcd26

consistent whitespace (by semantic grouping) in index.ts

14541b8

Rename: type→kind

dcd38c7

drop implicit return

63f33d0

kateinoigakukun reviewed Apr 10, 2022

View reviewed changes

Comment thread IntegrationTests/lib.js Outdated

Comment thread Sources/JavaScriptKit/ConvertibleToJSValue.swift Outdated

Comment thread Sources/JavaScriptKit/ConvertibleToJSValue.swift Outdated

j-f1 added 3 commits April 16, 2022 09:00

SPI for JSObject_id

c54bd10

Move i64 stuff to a separate module

7af5917

fix Signed/UnsignedInteger ConstructibleFromJSValue

b7a6a7d

fix tests?

b83611f

j-f1 requested a review from a team April 16, 2022 13:55

kateinoigakukun reviewed Apr 16, 2022

View reviewed changes

Comment thread Sources/JavaScriptKit/XcodeSupport.swift Outdated

Comment thread Runtime/src/types.ts Outdated

Comment thread Sources/JavaScriptKit/JSValue.swift Outdated

Comment thread Runtime/src/types.ts Outdated

j-f1 added 2 commits April 16, 2022 18:52

Address review comments

4ca7705

Simplify JSValue: CustomStringConvertible conformance

35dd9f7

j-f1 requested a review from a team April 19, 2022 12:00

MaxDesiatov approved these changes Apr 19, 2022

View reviewed changes

Comment thread Package.swift Outdated

Comment thread Sources/JavaScriptKit/FundamentalObjects/JSBigInt.swift Outdated

Comment thread Sources/JavaScriptKit/FundamentalObjects/JSObject.swift Outdated

Comment thread Sources/JavaScriptKit_I64/XcodeSupport.swift Outdated

j-f1 and others added 3 commits April 22, 2022 09:50

rename to JavaScriptBigIntSupport

0ec8fc3

Formatting tweak

7960198

Co-Authored-By: Max Desiatov <max@desiatov.com>

fix typo

5cff142

Co-Authored-By: Max Desiatov <max@desiatov.com>

j-f1 requested a review from kateinoigakukun April 22, 2022 14:10

kateinoigakukun approved these changes Apr 22, 2022

View reviewed changes

j-f1 merged commit 34bf9e1 into main Apr 22, 2022

j-f1 deleted the jed/bigint branch April 22, 2022 14:59

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

j-f1 commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov commented Apr 10, 2022

Uh oh!

kateinoigakukun commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-f1 commented Apr 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kateinoigakukun commented Apr 16, 2022

Uh oh!

MaxDesiatov commented Apr 16, 2022

Uh oh!

MaxDesiatov commented Apr 16, 2022

Uh oh!

j-f1 commented Apr 16, 2022

Uh oh!

MaxDesiatov commented Apr 19, 2022

Uh oh!

j-f1 commented Apr 19, 2022

Uh oh!

kateinoigakukun commented Apr 19, 2022

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaxDesiatov commented May 3, 2022

Uh oh!

MaxDesiatov commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-f1 commented May 3, 2022

Uh oh!

MaxDesiatov commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

j-f1 commented Apr 9, 2022 •

edited

Loading

github-actions Bot commented Apr 9, 2022 •

edited

Loading

kateinoigakukun commented Apr 10, 2022 •

edited

Loading

github-actions Bot commented Apr 16, 2022 •

edited

Loading

MaxDesiatov commented May 3, 2022 •

edited

Loading

MaxDesiatov commented May 3, 2022 •

edited

Loading