browsermt / bergamot-translator

Cross platform C++ library focusing on optimized machine translation on the consumer-grade device.
http://browser.mt
Mozilla Public License 2.0
330 stars 37 forks source link

Rework WASM compilation options #399

Closed jelmervdl closed 2 years ago

jelmervdl commented 2 years ago

Necessary to work with newer versions of emscripten that are more picky about which option goes to the compiler, and which to the linker. Also took the opportunity to remove the need for the patching of the bergamot-translation-worker.js file, this can now easily be done through supported apis. Furthermore, I tried to downsize the generated javascript and wasm code a bit.

Initial estimates show that bergamot-translator compiled with emscripten 3.0.0 runs at about 3x the speed of 2.0.9 (when using embedded intgemm). Speed-up when using mozIntGemm is less dramatic, I expect:

Benchmark 1: node ./test-nodejs.js ../build-wasm/
  Time (mean ± σ):      9.355 s ±  0.222 s    [User: 13.142 s, System: 0.368 s]
  Range (min … max):    9.119 s …  9.882 s    10 runs

Benchmark 2: node ./test-nodejs.js ../build-wasm-github/
  Time (mean ± σ):     27.380 s ±  1.004 s    [User: 30.565 s, System: 0.431 s]
  Range (min … max):   26.419 s … 29.500 s    10 runs

Summary
  'node ./test-nodejs.js ../build-wasm/' ran
    2.93 ± 0.13 times faster than 'node ./test-nodejs.js ../build-wasm-github/'

Filesizes are also a bit smaller:

.rw-r--r--   83Ki jelmer staff  5 Apr 12:27   -I build-wasm/bergamot-translator-worker.js
.rwxr-xr-x  5.8Mi jelmer staff  5 Apr 12:27   -I build-wasm/bergamot-translator-worker.wasm

.rw-r--r--@ 161Ki jelmer staff 31 Mar 11:21   -I build-wasm-github/bergamot-translator-worker.js
.rw-r--r--@ 6.6Mi jelmer staff 31 Mar 11:21   -I build-wasm-github/bergamot-translator-worker.wasm
abhi-agg commented 2 years ago

Initial estimates show that bergamot-translator compiled with emscripten 3.0.0 runs at about 3x the speed of 2.0.9 (when using embedded intgemm)

@jelmervdl Wow. This sounds awesome. Thanks for the initiative. I hope we get the same performance in browser environment as well. Irrespective of that, we should upgrade the toolchain.

Although the PR is in draft state, I am leaving few comments. I hope that is fine with you 👍🏾

jelmervdl commented 2 years ago

I only later noticed that marian-dev also has a wasm example itself. I think I removed too much from its CMakeLists file to still work, so I'll bring back some of that as well.

abhi-agg commented 2 years ago

I only later noticed that marian-dev also has a wasm example itself. I think I removed too much from its CMakeLists file to still work, so I'll bring back some of that as well.

I was about to write the same 👍🏾 If you want, I can also submit a PR to have similar changes in marian-dev if that is helpful.

jerinphilip commented 2 years ago

I only later noticed that marian-dev also has a wasm example itself. I think I removed too much from its CMakeLists file to still work, so I'll bring back some of that as well.

I propose going forward abandoning of WASM marian-decoder to keep things simple. The original attempt to port marian to WASM for inference left us with marian-decoder in WASM (https://github.com/browsermt/marian-dev/pull/6). Bergamot's WASM bindings/port have filled in the role.

jelmervdl commented 2 years ago

I propose going forward abandoning of WASM marian-decoder to keep things simple. The original attempt to port marian to WASM for inference left us with marian-decoder in WASM (https://github.com/browsermt/marian-dev/pull/6). Bergamot's WASM bindings/port have filled in the role.

I'm a fan. Simplifies things a lot.

I was about to write the same 👍🏾 If you want, I can also submit a PR to have similar changes in marian-dev if that is helpful.

If we're abandoning the wasm interface in marian-dev (the one that's not actively being used or tested right now…) then just a minimal change like this one should be sufficient. Maybe another one that removes the 'wasm' directory from marian-dev entirely.

abhi-agg commented 2 years ago

If we're abandoning the wasm interface in marian-dev (the one that's not actively being used or tested right now…) then just a minimal change like this one should be sufficient. Maybe another one that removes the 'wasm' directory from marian-dev entirely.

Although maintaining marian-decode looks to be a minimal change. Very similar to here. Separating compile and link flags, which has to happen anyway. However, I am fine with throwing that away. So, a PR in marian-dev with the corresponding submodule update here should be sufficient. I haven't looked into the exact changes in marian but I can go through them right away 👍🏾

jelmervdl commented 2 years ago

@abhi-agg do you have a specific timeline for this issue? I.e. asap for code freeze or something? I was working on something else today, but can switch if necessary.

abhi-agg commented 2 years ago

@abhi-agg do you have a specific timeline for this issue? I.e. asap for code freeze or something? I was working on something else today, but can switch if necessary.

Yep. We want to freeze by eod today. So upgrading emscripten version if the topmost priority for me. If you don't have bandwidth today then I am more than happy to take it up.

jelmervdl commented 2 years ago

@abhi-agg Feel free to go ahead and take over this pull request. I feel that's probably the faster way of getting this done today.

abhi-agg commented 2 years ago

remove the need for the patching of the bergamot-translation-worker.js file, this can now easily be done through supported apis

Just a heads up. I will not be including this change. I also don't know right now how to have the wasm binary itself initialise the function pointers to the fallback implementations, and then have the ability to override them during initialisation.

jelmervdl commented 2 years ago

Replaced by #414