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

feat: add browser support (ECO-967) (ECO-1240) #1

Merged
merged 45 commits into from Jun 16, 2022

Conversation

bryanchriswhite
Copy link

@bryanchriswhite bryanchriswhite commented May 3, 2022

Status

Browser-applicable tests are passing (i.e. unit/bindings/* and unit/lib/index.test.ts). I assume that a web worker based analogue of the worker_thread could be constructed, this is out of scope for my immediate needs.

I also have this module linked locally (yarn link, etc.) in another project which is able to bundle it (once built) like a regular dependency. Once a release is cut which includes the emscripten binding, consumers should only have to yarn add it and ensure their webpack config is compatible. See README -> using emscripten bindings for more.

Missing Members

Some global members are still missing from the emscripten binding (as I haven't figured out how to bind them yet):

  • BLS12_381_G1
  • BLS12_381_NEG_G1
  • BLS12_381_G2
  • BLS12_381_NEG_G2

Changes

  • add pre.js, post.js, and webpack.config.js to eslint ignore patterns
  • add bundle.js, blst.wasm, and blst.wasm.js to gitignore
  • move prebuild/* to prebuild/swig/*
  • add prebuild/emscripten directory
    • JS wrapper code
    • CPP wrapper code
    • output dir for compiled JS and wasm
  • add scripts to package.json:
    • inline:wasm - serializes wasm binary to base64 encoded JS, exported as an arrray buffer
    • test:browser - runs browser-compatible tests
  • add webpack dependencies (incl. necessary polyfill / fallback dependencies):
    • webpack
    • webpack-cli
    • webpack-dev-server
    • ts-loader
    • path-browserify
    • stream-browserify
    • crypto-browserify
    • buffer
    • assert-browserify
  • add WebIDL definition (used to generate initial bindings, not really useful going forward)
  • dynamically require appropriate binding module based on environment detection (thanks @dapplion!)
  • nest browser-applicable tests in run functions to wait for the emscripten runtime to be ready, if present
  • update README to include details on how to build and use the emscripten bindings

Dependencies

@dapplion
Copy link

dapplion commented May 4, 2022

Hey @bryanchriswhite ! Super curious about this PR, are you able to run blst JS bindings in the browser? If so, what's your strategy?

@bryanchriswhite
Copy link
Author

bryanchriswhite commented May 5, 2022

@dapplion the plan is to use webidl in conjunction with emscripten to generate web assembly bindings. At the moment, I seem to have massaged the generated binding code to the point where it's building the WASM module and supporting JS. Now I'm setting up webpack to integrate this with blst-ts in place of the native node module to get to the point where I can get the blst-ts tests passing in the browser. 😄

(The blst submodule draft changes are in fetchai/blst#1)

@dapplion
Copy link

dapplion commented May 5, 2022

That's amazing! It would be down to help you upstream the browser support if you want. supranational has been very receptive in the past for modifications in the source to support bindings. What's the scope of the changes in blst C side?

@bryanchriswhite
Copy link
Author

@dapplion thanks, that's great to hear! 😄

So far the changes in blst are to include the generated WebIDL binding C++ source in the build script along with any emcc-specific arguments like --post-js. I was planning on tidying up after I get things working, at the moment I'm sort-of trampling over things to expedite progress. 😅

I would love to hear your thoughts on how to structure things best so that browser / web assembly support can coexist upstream!

@dapplion
Copy link

dapplion commented May 6, 2022

@bryanchriswhite Once you got something stable and working I would upstream the changes in the blst repo. Keep modifications minimal and add the new bindings into their own folder along with the absolute minimal CI that ensures it works and shows how to integrate downstream. Then we can discuss in that PR with supranational if they prefer different structures.

@bryanchriswhite bryanchriswhite force-pushed the feat/browser-support branch 2 times, most recently from c16f9d8 to 927537c Compare May 9, 2022 12:36
@bryanchriswhite bryanchriswhite force-pushed the feat/browser-support branch 2 times, most recently from fc78248 to a161ecc Compare May 17, 2022 15:46
@bryanchriswhite
Copy link
Author

bryanchriswhite commented Jun 13, 2022

Hey @dapplion! I did my best to setup karma but it looks like the tests aren't run unless you start in one shell and run a few times in another. I haven't used karma in quite a while so it's not immediately obvious to me what I'm doing wrong. I also have mocha running (natively) in the browser which seems to work fine (and presents a friendlier in-browser UI). I added the relevant details to README -> Testing in the browser.

I've added a build script that uses docker, which seemed like the only sane option to me. I'm thinking that it will make sense to include the in-lined wasm binary in the npm release so that consumers don't need to build it.

As you may have noticed, I turned on actions for the fork but have not added anything to the workflow yet. The only changes I've made to the actions were to update paths to things that I moved. The benchmark is failing because of the missing members (see PR description); I removed them from the Blst interface temporarily.

@bryanchriswhite bryanchriswhite marked this pull request as ready for review June 13, 2022 10:59
@dapplion
Copy link

Amazing progress! How big in the target bin size? Inlining and bundling into the release is the right path IMO.

@bryanchriswhite
Copy link
Author

Amazing progress! How big in the target bin size? Inlining and bundling into the release is the right path IMO.

Thanks! 😄

The last build of the in-line JS I did was ~282kB. The original wasm was ~212 kB.

@bryanchriswhite bryanchriswhite changed the title feat: add browser support (ECO-967) feat: add browser support (ECO-967) (ECO-1240) Jun 15, 2022
Copy link
Member

@ejfitzgerald ejfitzgerald left a comment

Choose a reason for hiding this comment

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

Just some suggestions for the C++ and the new stuff

prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
prebuild/emscripten/glue.cpp Outdated Show resolved Hide resolved
Co-authored-by: Ed Fitzgerald <edward.fitzgerald@fetch.ai>
@bryanchriswhite
Copy link
Author

bryanchriswhite commented Jun 15, 2022

Thanks @ejfitzgerald, that eliminates all mallocs! 🙌

@bryanchriswhite
Copy link
Author

bryanchriswhite commented Jun 15, 2022

@dapplion, any idea why the ARM jobs take soooo long, is that normal?

@dapplion
Copy link

dapplion commented Jun 16, 2022

@dapplion, any idea why the ARM jobs take soooo long, is that normal?

Github does not have ARM runners so we run QEMU with docker buildx. Since the ARM architecture is emulated it's extremely slow to run anything. As it has to install a lot of stuff and do an expensive compilation it takes forever.

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.

None yet

3 participants