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

Use WebAssembly #50

Merged
merged 12 commits into from Feb 3, 2018

Conversation

Projects
None yet
3 participants
@kripken
Contributor

kripken commented Feb 2, 2018

This uses wasm to optimize the slow i64 multiply and division operations: If wasm is present, it uses that, and if not it falls back to the old code.

This is just a proof of concept to see what you think, it's not polished at all. In particular I'm not sure what's right for the build system, etc. But it's such a tiny wasm module that for now I just pasted the contents of the binary in the source for testing ;)

Doing some simple benchmarks, this is around 2x faster (I'd like to look into that more carefully as it should be even more, but promising already). Tests pass (npm run-script build, npm test).

@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 2, 2018

Owner

Heh, I had a similar idea a while back, see: https://github.com/AssemblyScript/assemblyscript/tree/master/examples/i64-polyfill (wast)

But at this point in time it'll most likely make more sense to do just the expensive ones in WASM, as you propose. Possibly mod/remainder as well.

Btw. including the WASM-blob as you did seems ideal :)

Owner

dcodeIO commented Feb 2, 2018

Heh, I had a similar idea a while back, see: https://github.com/AssemblyScript/assemblyscript/tree/master/examples/i64-polyfill (wast)

But at this point in time it'll most likely make more sense to do just the expensive ones in WASM, as you propose. Possibly mod/remainder as well.

Btw. including the WASM-blob as you did seems ideal :)

@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Feb 2, 2018

Contributor

Yeah, I agree that in general AssemblyScript makes more sense for code that is meant to be reviewed and maintained over time. The case here is probably an exception, where it's easy to just write the tiny necessary wasm by hand...

I added optimization for the remainder operation.

Contributor

kripken commented Feb 2, 2018

Yeah, I agree that in general AssemblyScript makes more sense for code that is meant to be reviewed and maintained over time. The case here is probably an exception, where it's easy to just write the tiny necessary wasm by hand...

I added optimization for the remainder operation.

@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 2, 2018

Owner

Looking good to me. Unless there are any concerns on your side, I'd merge :)

Owner

dcodeIO commented Feb 2, 2018

Looking good to me. Unless there are any concerns on your side, I'd merge :)

@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Feb 2, 2018

Contributor

Maybe I'm paranoid ;) but how thorough are the tests here - are all the closure long tests run? (or is there a way to run them?)

Also maybe worth doing is finding a way to test both wasm and non-wasm - right now it just tests wasm if supported, or non-wasm if not.

Contributor

kripken commented Feb 2, 2018

Maybe I'm paranoid ;) but how thorough are the tests here - are all the closure long tests run? (or is there a way to run them?)

Also maybe worth doing is finding a way to test both wasm and non-wasm - right now it just tests wasm if supported, or non-wasm if not.

@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 3, 2018

Owner

Well, erm (runs around nervously), it appears there are no closure tests being run at all. Iirc, testing merely focuses on the things I changed and their respective edge cases.

Owner

dcodeIO commented Feb 3, 2018

Well, erm (runs around nervously), it appears there are no closure tests being run at all. Iirc, testing merely focuses on the things I changed and their respective edge cases.

@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 3, 2018

Owner

Regarding WebAssembly: CI runs the tests with both node 0.12 (without WebAssembly) and latest LTS (with WebAssembly). That's ok-ish for me, as I don't expect a lot of future changes here anyway.

Owner

dcodeIO commented Feb 3, 2018

Regarding WebAssembly: CI runs the tests with both node 0.12 (without WebAssembly) and latest LTS (with WebAssembly). That's ok-ish for me, as I don't expect a lot of future changes here anyway.

@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Feb 3, 2018

Contributor

Oh cool, I see you added more tests, merged in and now passing. I had to fix up signed division overflow, which in wasm traps but closure treats it as silent failure (returns the numerator ¯\_(ツ)_/¯).

I also added node stable as well for testing, to make sure latest wasm is also checked. Thoughts?

How do I update dist/? npm build doesn't seem to.

Contributor

kripken commented Feb 3, 2018

Oh cool, I see you added more tests, merged in and now passing. I had to fix up signed division overflow, which in wasm traps but closure treats it as silent failure (returns the numerator ¯\_(ツ)_/¯).

I also added node stable as well for testing, to make sure latest wasm is also checked. Thoughts?

How do I update dist/? npm build doesn't seem to.

@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 3, 2018

Owner

I also added node stable as well for testing, to make sure latest wasm is also checked

I believe lts/* is already using node 8, with WASM.

How do I update dist/? npm build doesn't seem to.

It's npm run build (only test works without run)

Owner

dcodeIO commented Feb 3, 2018

I also added node stable as well for testing, to make sure latest wasm is also checked

I believe lts/* is already using node 8, with WASM.

How do I update dist/? npm build doesn't seem to.

It's npm run build (only test works without run)

@kripken

This comment has been minimized.

Show comment
Hide comment
@kripken

kripken Feb 3, 2018

Contributor

Yeah, LTS is 8.*, but the latest stable is 9.*. I think it's worth testing that too since wasm stuff may change in VMs, and it's fast, but up to you.

Thanks, updated with run build.

Contributor

kripken commented Feb 3, 2018

Yeah, LTS is 8.*, but the latest stable is 9.*. I think it's worth testing that too since wasm stuff may change in VMs, and it's fast, but up to you.

Thanks, updated with run build.

@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 3, 2018

Owner

Yeah, LTS is 8., but the latest stable is 9

Ah, sorry, I didn't read carefully enough. Makes sense!

Owner

dcodeIO commented Feb 3, 2018

Yeah, LTS is 8., but the latest stable is 9

Ah, sorry, I didn't read carefully enough. Makes sense!

@dcodeIO dcodeIO merged commit cb2fb1b into dcodeIO:master Feb 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 3, 2018

Owner

Thanks! :)

Owner

dcodeIO commented Feb 3, 2018

Thanks! :)

@ColinEberhardt

This comment has been minimized.

Show comment
Hide comment
@ColinEberhardt

ColinEberhardt Feb 3, 2018

Nice idea - although this is making my head hurt!

We're using Long within the WebAssembly interpreter for i64 support.

So, for browsers that don't support WebAssembly, you could use our interpreter for your WASM code ...

ColinEberhardt commented Feb 3, 2018

Nice idea - although this is making my head hurt!

We're using Long within the WebAssembly interpreter for i64 support.

So, for browsers that don't support WebAssembly, you could use our interpreter for your WASM code ...

@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 3, 2018

Owner

Heh, but wouldn't your interpreter, when using long.js in an environment without WebAssembly-support, use your interpreter in an environment without Webassembly-support, to use long.js?

Owner

dcodeIO commented Feb 3, 2018

Heh, but wouldn't your interpreter, when using long.js in an environment without WebAssembly-support, use your interpreter in an environment without Webassembly-support, to use long.js?

@ColinEberhardt

This comment has been minimized.

Show comment
Hide comment
@ColinEberhardt

ColinEberhardt Feb 3, 2018

yes, it would. I just found it quite ironic that the library we are using to support i64 in environments that don't support WebAssembly is itself now using WebAssembly :-)

Anyhow, thanks for the great work on Long.js, it's made my job a lot easier!

ColinEberhardt commented Feb 3, 2018

yes, it would. I just found it quite ironic that the library we are using to support i64 in environments that don't support WebAssembly is itself now using WebAssembly :-)

Anyhow, thanks for the great work on Long.js, it's made my job a lot easier!

@dcodeIO

This comment has been minimized.

Show comment
Hide comment
@dcodeIO

dcodeIO Feb 3, 2018

Owner

Did you consider that if your interpreter was written in AssemblyScript, you could also compile it to WebAssembly, you know, for the extra performance where WebAssembly is available, but still be able to use your interpreter compiled to JavaScript to interpret your interpreter compiled to WebAssembly to interpret WebAssembly where WebAssembly isn't available?

Owner

dcodeIO commented Feb 3, 2018

Did you consider that if your interpreter was written in AssemblyScript, you could also compile it to WebAssembly, you know, for the extra performance where WebAssembly is available, but still be able to use your interpreter compiled to JavaScript to interpret your interpreter compiled to WebAssembly to interpret WebAssembly where WebAssembly isn't available?

@ColinEberhardt

This comment has been minimized.

Show comment
Hide comment
@ColinEberhardt

ColinEberhardt Feb 3, 2018

😱😱😱😱😱

ColinEberhardt commented Feb 3, 2018

😱😱😱😱😱

@kripken kripken deleted the kripken:wasm branch May 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment