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

Closed
agnivade opened this issue Jun 26, 2019 · 2 comments
Closed

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

agnivade opened this issue Jun 26, 2019 · 2 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@agnivade
Copy link
Contributor

{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 Performance NeedsFix The path to resolution is known, but the work has not been done. arch-wasm WebAssembly issues labels Jun 26, 2019
@agnivade agnivade added this to the Unplanned milestone Jun 26, 2019
@agnivade agnivade self-assigned this Jun 26, 2019
@agnivade
Copy link
Contributor Author

@agnivade
Copy link
Contributor Author

But I guess this should be an improvement.

Well, guesses are not good. Turns out converting to a BigInt is far more costlier than 2 memory accesses.

https://jsperf.com/dataviewbigint/1

view.setUint32(0, 2948279085, true); 
view.setUint32(4, Math.floor(2948279085 / 4294967296), true);

266,362,343±3.85%fastest

view.setBigUint64(0, BigInt(2948279085), true); 

20,187,453±1.30%92% slower

Closing this.

@golang golang locked and limited conversation to collaborators May 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

2 participants