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

misc/wasm: use {get/set}BigUint64 functions when BigInt hits stage4 #32785

Open
agnivade opened this issue Jun 26, 2019 · 1 comment

Comments

@agnivade
Copy link
Member

commented Jun 26, 2019

{get/set}BigUint64 does not have full support yet

This is a reminder issue to change the code to the following once BigInt reaches stage4.

diff --git a/misc/wasm/wasm_exec.js b/misc/wasm/wasm_exec.js
index 7341e755e7..5d38fac5fd 100644
--- a/misc/wasm/wasm_exec.js
+++ b/misc/wasm/wasm_exec.js
@@ -114,14 +114,16 @@
                        this._nextCallbackTimeoutID = 1;
 
                        const setInt64 = (addr, v) => {
+                               if (Number.isInteger(v)) {
+                                       this.mem.setBigUint64(addr, BigInt(v), true);
+                                       return;
+                               }
                                this.mem.setUint32(addr + 0, v, true);
                                this.mem.setUint32(addr + 4, Math.floor(v / 4294967296), true);
                        }
 
                        const getInt64 = (addr) => {
-                               const low = this.mem.getUint32(addr + 0, true);
-                               const high = this.mem.getInt32(addr + 4, true);
-                               return low + high * 4294967296;
+                               return Number(this.mem.getBigUint64(addr, true));
                        }
 
                        const loadValue = (addr) => {

Coercion to and from BigInt and Number should be okay since we are anyways dealing with 2^53 bit integers.

Benchmarks do not show any noticeable difference mainly because the variance is so large:

name old time/op new time/op delta
DOM 101µs ±32% 98µs ±37% ~ (p=0.853 n=10+10)

But I guess this should be an improvement.

Related #32591

@agnivade agnivade added this to the Unplanned milestone Jun 26, 2019

@agnivade agnivade self-assigned this Jun 26, 2019

@agnivade

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.