1511958 - Implement i64<>JavaScript’s BigInt conversions proposal (basic functionality + most stubs)

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Sven Sauleau]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) QtWebEngine/5.11.1 Chrome/65.0.3325.230 Safari/537.36

Steps to reproduce:

Take the following Wasm module: `(func (export "f")) (result i64) (i64.const 66))`.



Actual results:

Calling `f` is not possible in JavaScript because the signature contains an i64 type and there no way to represent it properly on the host currently.


Expected results:

JavaScript recently introduced the BigInt object, allowing the representation of arbitrary length integers.

The proposal is specifying the conversion between these two types.

Comment 1

[Sven Sauleau]

Note that I can work on this, I'm currently implementing it in V8 as well.

Comment 2

[Jan de Mooij [:jandem]]

Probably good to coordinate with the Igalia folks working on BigInt.

Comment 3

[Sven Sauleau]

Yes, I'm working for Igalia.

I forgot to mention that the proposal (https://github.com/WebAssembly/JS-BigInt-integration) is stage 2 in WebAssembly's GC process. I don't think it should block the JavaScript BigInt implementation since it should be considered as an experimental WebAssembly feature.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Sven Sauleau from comment #3)
> Yes, I'm working for Igalia.

Oh, awesome \o/

Comment 5

[Sven Sauleau]

Attachment: Implement i64<>JavaScript’s BigInt conversions proposal (JS runtime)

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:sven, could you have a look please?

Comment 7

[Asumu Takikawa]

Attachment: Bug 1511958 - Implement i64<>JavaScript’s BigInt conversions proposal (JS runtime & Ion)

Continues rev D22990 to implement the I64 to BigInt conversions proposal (https://github.com/WebAssembly/JS-BigInt-integration), original rev by Sven Sauleau

Comment 8

[Asumu Takikawa]

Attachment: Bug 1511958 - Implement i64<>JavaScript’s BigInt conversions proposal (part 1, build system changes)

This is part 1 of a series of revs that split up D41710 into smaller revs. This rev just adds a compile-time flag for wasm BigInt to I64 conversion but does not use it anywhere, as its uses are for toggling features in subsequent revs.

Comment 9

[Asumu Takikawa]

Attachment: Bug 1511958 - Implement i64<>JavaScript’s BigInt conversions proposal (part 2, runtime flag and testing function)

This is part 2 of a series of revs that split up D41710 (for Wasm I64 to BigInt conversion) into smaller revs. This rev depends on the compile-time flag added in D43177 and adds a runtime flag to JSContext options that will toggle whether I64 to BigInt conversion is used. The flag will get used mostly in WasmInstance.cpp, but it also needs to be used to toggle I64 error checks in both Ion inlining code and in Wasm stub generation code. To pass that information along, the flag is also put in CompileArgs for WasmCompile and then copied to wasm module metadata (so that it can be read from lazy stub generation code).

Comment 10

[Andy Wingo [:wingo]]

Setting checkin-needed for https://phabricator.services.mozilla.com/D43177. @asumu I think it can make sense to open new bugs blocking this one when you have additional patches beyond the second one in the series that's already up.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2072e34a3b1
Implement i64<>JavaScript’s BigInt conversions proposal (part 1, build system changes) r=wingo,bbouvier

Comment 12

[Raul Gurzau (:RaulG)]

https://phabricator.services.mozilla.com/D43179 needs review from bbouvier to continue. Thank you.

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/d2072e34a3b1

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d2072e34a3b1

Comment 15

[Asumu Takikawa]

Attachment: Bug 1511958 - Implement i64<>JavaScript’s BigInt conversions proposal (part 3, JIT and runtime changes)

This is part 3 of a series of revs that split up D41710 (for Wasm I64 to BigInt conversion) into smaller revs. This rev depends on the compile-time flag added in D43177 and the run-time flag in D43179. Sorry for the delay in uploading this, there were a number of bugs I had to fix.

The rev adds support for BigInt<->I64 conversion in WasmInstance and to the Wasm stubs for the JIT, and in the inlining code in MCallOptimize. It addresses some of the main concerns that wingo brought up in comments on the original rev: the BigInt to I64 code path in the stubs is now written in assembly (in MacroAssembly.cpp) to avoid some hacky code. Most of the changes are also guarded by an ifdef for ENABLE_WASM_BIGINT now.

One comment that I haven't addressed yet but I'll add is BigInt fast paths for the JIT entry stub for simple cases.

Comment 16

[Asumu Takikawa]

Attachment: Bug 1511958 - Implement i64<>JavaScript's BigInt conversions proposal (part 4, jit tests)

This is part 4 of a series of revs that split up D41710 (for Wasm I64 to BigInt conversion) into smaller revs. This rev depends on the compile-time flag added in D43177 and the run-time flag in D43179 and the main implementation in D46205.

This rev adds tests in the JIT test suite that exercises the BigInt conversion code, and also modifies existing tests to work with the --wasm-big-int flag either enabled or disabled (with different expected results).

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Is the spec rendered somewhere, please? In particular, I wanted to learn if/why it was fine to wrap an incoming BigInt into an i64 (since a big integer may be arbitrarily large, unless its name stands for a lie).

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

bbouvier: it is now: https://webassembly.github.io/JS-BigInt-integration/js-api/index.html

BigInt values wrap around modulo 2^64, just like Numbers do for i32.

Comment 19

[Lars T Hansen [:lth]]

Code freeze is now lifted, we can land scary things again.

Asumu, what is your estimate for when you think you might land this? I have a big patch I think will conflict with your work quite a lot and I'm happy to wait until you've landed and take the burden of rebasing, but only if you think you're going to land by Dec 13, say -- I'd like to get things landed before the holidays.

Comment 20

[Asumu Takikawa]

Hi Lars, I expect to address all the remaining comments on the part 3 and 4 revs for this today, so parts 2 to 4 should be ready very soon. So I am on board with your timeline, and thanks for taking on the rebasing in that case.

I do have two patches remaining (parts 5 and 6) that I will put on phabricator that I think are less involved and adds the changes to wasm globals needed for the proposal plus tests. These changes are confined to WasmJS.cpp/h and test files. I can put these up today too.

Comment 21

[Asumu Takikawa]

Attachment: Bug 1511958 - Implement i64<>JavaScript's BigInt conversions proposal (part 5, wasm global changes)

This is part 5 of a series of revs that split up D41710 (to implement the Wasm I64 to BigInt conversion proposal) into smaller revs.

The rev adds support for handling globals with I64 types in the Wasm to JS interface.

Comment 22

[Asumu Takikawa]

Attachment: Bug 1511958 - Implement i64<>JavaScript's BigInt conversions proposal (part 6, wasm global tests)

This is part 6 of a series of revs that split up D41710 (to implement the Wasm I64 to BigInt conversion proposal) into smaller revs.

The rev adds tests for wasm global handling with the Wasm to JS interface.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f33a1ee1f74a
Implement i64<>JavaScript’s BigInt conversions proposal (part 2, runtime flag and testing function)

Comment 24

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f33a1ee1f74a

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/491b05117886
Implement i64<>JavaScript’s BigInt conversions proposal (part 3, JIT and runtime changes) r=wingo,lth

Comment 26

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/491b05117886

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4c1422991dc
Implement i64<>JavaScript's BigInt conversions proposal (part 4, jit tests) r=lth

Comment 28

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c4c1422991dc

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09dc201fb083
Implement i64<>JavaScript's BigInt conversions proposal (part 5, wasm global changes) r=lth

Comment 30

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/09dc201fb083

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/352639357a90
Implement i64<>JavaScript's BigInt conversions proposal (part 6, wasm global tests) r=lth

Comment 32

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/352639357a90

Comment 33

[Lars T Hansen [:lth]]

Fabulous work!

Are we still missing code for the extra-special Ion->Wasm fast call path? Or maybe I should ask, are there plans to do something about that, or should we close this bug now and do that "later"?

Comment 34

[Lars T Hansen [:lth]]

We should also discuss whether it would not be appropriate to enable this feature by default in the shell and in Nightly.

Comment 35

[Asumu Takikawa]

Hi Lars, thanks! (sorry for the delayed response, I'm only reading e-mail intermittently during these last weeks of the year) I have a work-in-progress patch that would enable the Ion->Wasm fast path, which has been made easier by some of the recent BigInt-related macroassembler changes. I am hoping to finish that in early January or so when I'm back from the holidays, so I could file that patch under this bug then (or under a new one too if that's preferable).

Also, sounds good on the discussion, what are the typical criteria for enabling a feature in the shell/nightly?

Comment 37

[Lars T Hansen [:lth]]

(In reply to Asumu Takikawa from comment #35)

Hi Lars, thanks! (sorry for the delayed response, I'm only reading e-mail intermittently during these last weeks of the year) I have a work-in-progress patch that would enable the Ion->Wasm fast path, which has been made easier by some of the recent BigInt-related macroassembler changes. I am hoping to finish that in early January or so when I'm back from the holidays, so I could file that patch under this bug then (or under a new one too if that's preferable).

Let's do a new bug.

I'm also going to remove the [meta] tag from this bug since metabugs should carry no patches but this one has many :-) I will make a new metabug and create new bugs for the ion->wasm fast path and for the enable-on-nightly, and I'll assign them both to you.

After that I think we can close this one?

Also, sounds good on the discussion, what are the typical criteria for enabling a feature in the shell/nightly?

Basically, we need to believe that the feature design is "stable enough" and that the code is pretty solid and can benefit from the extra testing. The spec seems to be stage 3, which is fine on the first count, and so far all our testing and reviews have lead to code that's pretty decent, I would say, so I'm not worried about the second count either.

Comment 38

[Lars T Hansen [:lth]]

Metabug for remaining work is bug 1608770.