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

Adding support to compile to WebAssembly #2

Closed
wants to merge 3 commits into from

Conversation

shahrodkh
Copy link

I have made additions to your Makefile to compile to a .wasm file instead of .asm
Some other minor changes had to be made so that it will compile.

@fta2012
Copy link
Owner

fta2012 commented Mar 26, 2017

Cool I didn't know web assembly was ready!

Some comments:

  • It's my fault that the make file isn't organized better but there should multiple sets of flags and build targets. They are not named but the existing grouping of flags are roughly mandatory, release, and debug (where the debug flags are currently commented out and aren't tested/used).
    • I am mentioning this because I recall docs saying that the flags you enabled (ASSERTIONS, ALLOW_MEMORY_GROWTH) will prevent certain optimizations so probably shouldn't be in the release build.
    • Likewise the -s WASM flag should probably be a target specific variable for a new wasm build target . Then you can have both the wasm and regular build and choose which to use depending on browser support
  • What is the sed replacement for? I think there are callbacks like Module.preInit/preRun/postRun that you can hook into more cleanly if that's what you're going for. If that fails you can also add EM_ASM to the int main() in bindings to make it call your callback. But testing it out myself it seems like postRun will work.
  • I don't like adding a completely new demo since you can't compare it with the non-wasm build. Also a quicker way to do a static webserver is sudo python -m SimpleHTTPServer 80 instead of adding a boilerplate express server.

I am glad you got the wasm working but overall I don't really like this change. If you can simplify it to just adding a new build target and adding wasm feature detection for the existing demo, then that would be okay with me!

I made some quick changes in f63c56c to reflect some of the issues I mentioned above. If you're still interested in doing so you can rebase and try adding the wasm target again.

Thanks!

return val(typed_memory_view(numElement, pointer->data.f64));
// case CCV_64S:
// return val(typed_memory_view(numElement, pointer->data.i64));
// case CCV_64F:
Copy link
Owner

@fta2012 fta2012 Mar 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this case also needs to be commented out. There are no Uint64Array in js but there are Float64Array.

@fta2012
Copy link
Owner

fta2012 commented Mar 26, 2017

Btw I changed it so that it will look for a global CCV variable to call CCVLib with (checkout index.html). I haven't tried this but you should be able to add your wasmBinary there too before loading the script:

// load your wasm
...
CCV = {
  wasmBinary: buffer,
  postRun: [ whatever ]
};
...
// load ccv.js 

@edge0701 edge0701 mentioned this pull request Apr 20, 2017
@shahrodkh
Copy link
Author

hey got caught up elsewhere -- looks like you made the changes though? hopefully I'll get a chance to check it out this weekend 👍

@shahrodkh shahrodkh closed this Jul 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants