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.

Sign up for GitHub

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

Jump to bottom

Proposed changes for v4 #4

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Proposed changes for v4 #4

wants to merge 20 commits into from

Conversation

zbjornson
Copy link
Collaborator

Hello and happy new year! Here's the full set of changes I'd like to propose. Everything is documented in the new changelog and commit messages. Faster, smaller, and build is much simpler.

zbjornson and others added 8 commits December 30, 2022 00:34
@zbjornson
fix build
2bc9d8b 
* `EXPORT_ES6` and `ENVIRONMENT=*node*` requires `USE_ES6_IMPORT_META` to be set. This changed in emscripten-core/emscripten#17915.

* `_malloc` and `_free` must be declared as exported functions to avoid removal by the compiler.

* Remove [`WASM_ASYNC_COMPILATION=0`](https://emsettings.surma.technology/#WASM_ASYNC_COMPILATION).

  The current code was broken by emscripten-core/emscripten#12650. Previously, `WASM_ASYNC_COMPILATION=0` would return `Promise<Module>`, but that PR made it return `Module` directly. (Took ages to find the cause of this!)

  Removing the flag to keep it async avoids a breaking change in this library. Node.js v14.8.0 and later support top-level await, so async isn't that ugly. Alternatively, the Node.js version could easily be changed to load synchronously. Node.js has no limit on the size of synchronously loaded WebAssembly modules, unlike Chromium.
@jvail
Fix build with current emscripten
bb3cb6a
@zbjornson
add changelog
bba113c
@zbjornson
don't install process.uncaughtException, unhandledRejection handlers
6657d6c 
Emscripten adds these by default, but they're inappropriate for a library to have as it affects the whole application's runtime behavior.
@zbjornson
remove package-lock.json
17cd4da 
Libraries shouldn't provide lockfiles, and the next commit removes the only runtime dependency.
@zbjornson
improve performance; fix ignored patience parameter
e81311e 
The evaluation of the parameters is now done in one JS function call per iteration, instead of *num_points* calls per iteration. Major (~2x) speedup for large datasets.

There was also a mismatch in the number of arguments in the `crwap` call that resulted in the last paramter (`patience`) being ignored.
@zbjornson
remove node as a dev dependency
e215feb
@zbjornson
add file extensions to import statements
bac7173 
Omitting extensions is nonstandard. This moves toward allow direct loading in Node.js without rollup.
@jvail
Copy link
Owner

jvail commented Jan 5, 2023

Happy new year @zbjornson and thank you, sounds great! Please give me a couple of days to take a look at it.

@jvail
Copy link
Owner

jvail commented Jan 6, 2023 •
edited
Loading

Very good and thank you for all the work you have done!

After updating to the latest emsdk everything was working fine .. well, except the webworker where I had to replace the import with a importScripts('/dist/lmfit.web.js') . Also had a déjà vu because of the error about import.meta not being available in workers. As far as I recall that was one reason for my - probably - overly complicated set up. Did the webworker example work for you?

I took a second look into the worker solution. I seems it is all the same issue with FF not supporting modules in worker right now. As soon as that is fixed in FF the example will also work in FF: https://bugzilla.mozilla.org/show_bug.cgi?id=1247687

Node is working fine (tested from v14 up to v19).

A few tiny suggestions:

The thing with the worker is a bit unfortunate. But I can live with it. It might just not be a good idea to run any wasm inside the main thread unless it is guaranteed that it returns very quickly. But lets see if there is someone out there having an issue with it.

P.S.:
Not sure if you have a use case for lmfit in the browser but function transfer could be done like this https://github.com/jvail/spl.js/blob/main/src/spl-web.ts#L18 (at least soon as dynamic imports are fixed in FF :\ ).

@zbjornson
rework the build/distribution strategy
3382344 
There's no longer any Node.js or browser-specific code!

1. Use emcc's built-in [`--post-js src/lm.js`](https://emscripten.org/docs/tools_reference/emcc.html#emcc-pre-js)
   option instead of rollup. This allows better (whole-program) optimization and
   is simpler. `src/lm.js` now contains a function that receives the `Module`
   instance.

2. Use emcc's built-in [`-s SINGLE_FILE=1`](https://emsettings.surma.technology/#SINGLE_FILE)
   option instead of the custom build scripts. This is simpler and, when the
   HTTP response containing lmfit.js is compressed (as it should be), saves
   ~2900 bytes.

3. **Breaking change:** The Web browser version now runs in the main thread
  instead of a Web Worker. This change was made because it's not possible to
  securely transfer a Function from the main thread to a Worker; i.e., sites
  using a Content Security Policy would have had to enable `unsafe-eval`.

  An example has been added showing how to use this module from a Web Worker
  instead.

4. **Breaking change:** The Node.js version is now an ESModule instead of CJS.
   All supported (non-EOL) versions of Node.js support ESM, so this should not
   be impactful.

5. Split `npm all` into `npm build` (builds) and `npm rebuild` (cleans and
   builds), to be consistent with the script convention of native modules and
   provide faster builds during development.
@zbjornson
shrink the build
37c9f3c 
Numbers are size of dist/lmfit.web.js (which embeds the WASM and support JS).

1. `151,151 B` Initially
2. ` 95,643 B` Removing [`RETAIN_COMPILER_SETTINGS=1`](https://emsettings.surma.technology/#RETAIN_COMPILER_SETTINGS). These were unused.
3. ` 94,898 B` Adding [`TEXTDECODER=2`](https://emsettings.surma.technology/#TEXTDECODER). Strings aren't used in this library anyway.
4. ` 69,132 B` Removing [`ASSERTIONS=1`](https://emsettings.surma.technology/#ASSERTIONS). Was nervous about removing these, but the docs (for optimization levels) encourage it for distribution builds. Also gives a few percent speedup.
5. ` 66,704 B` Removing [`DISABLE_EXCEPTION_CATCHING=0`](https://emsettings.surma.technology/#DISABLE_EXCEPTION_CATCHING). This is a C program, so there are no C++ exceptions to catch.
6. ` 60,164 B` Adding [`MALLOC=emmalloc`](https://emsettings.surma.technology/#MALLOC), which is smaller and fine for this library.

I did not add `--closure 1` (56,755 B) because IMO minification should be up to the consumer. Stepping through a minified library is miserable.
@zbjornson
additional compiler flags
46b2063 
- Support Safari v13.1
- Include "use strict"
- Explicitly specify `--no-entry`
@zbjornson
improve error messages when checking arguments
cf6f6cd
@zbjornson
include fnorm and status string in return value
f75cc40
@zbjornson
add typescript definitions
80d58cf 
Fixes #2
@zbjornson
update lmfit to fce0bc2e
5d2683f 
https://jugit.fz-juelich.de/mlz/lmfit/-/compare/497505787dcc9a2fa8420dd8f3f9b76c2dc8bab7...fce0bc2ebc9ba570046a7ed3fd2618fe7147e04a
@zbjornson
always populate returned params array
97a32e0 
The values can be useful even if the optimization has not converged.
@zbjornson
list known-working version of emsdk in readme
14614bc
@zbjornson
add zbjornson to package.json contributors
a3d0e3a
@zbjornson
Copy link
Collaborator Author

Thanks for the review, good suggestions. Implemented 1, 2, 3 and 5.

On 4: --minify 1 is deprecated, but we're using --minify 0, which is not deprecated. -Oz and -Os appear to produce identical output for this module (all dist/ files have identical hashes). Let me know if you want it switched to -g1 and -Oz anyway. (I also tested -O3. That increased the size from 61,424 bytes to 61,779 bytes (+355) and had no measurable impact on performance.)

I do plan to use this in the browser, but for very small datasets that take 1-2 ms to run. Blocking the main thread for 1-2 ms is preferable to waiting 50 async ms in my case.

I didn't know Firefox doesn't support modules in workers. If someone asks for it, that'd be worth down-compiling to CJS again for.

😵 I didn't realize CSP is such a headache with WebAssembly. Sites have to use script-src: 'wasm-unsafe-eval' to instantiate the module, but that's only supported by very new browsers (Chrome 97, Safari 16, Firefox 102), so most sites will have to use script-src: 'unsafe-eval'. If we have to do that, we might as well use new Function() and serialize the model. (That's a bit tighter than adding scrip-src: blob:, which is required for the technique used in spl.js.) Finally, the single-file bundle approach prints noisy console messages and is slower if script-src: data: isn't present. ... Changes made in light of this:

  • Removed -s SINGLE_FILE=1. This saves 11.2 KB and, as above, is better for CSPs without data:. It makes it a bit harder to use as the .wasm file has to be served, but encourages better security.
  • Set a Content-Security-Policy in the examples, and restructure so unsafe-inline isn't needed. (Trying to demo best security practices.)
  • Changed the Worker example to show both passing a serialized function or writing it into the worker directly.

@zbjornson
don't use a single-file bundle
62742b4
@zbjornson
rework webworker example
648c7fd 
* Better security demo
* Optionally serialize the model function
@jvail
Copy link
Owner

jvail commented Jan 10, 2023 •
edited
Loading

Thanks for the review, good suggestions. Implemented 1, 2, 3 and 5.

On 4: --minify 1 is deprecated, but we're using --minify 0, which is not deprecated. -Oz and -Os appear to produce identical output for this module (all dist/ files have identical hashes). Let me know if you want it switched to -g1 and -Oz anyway. (I also tested -O3. That increased the size from 61,424 bytes to 61,779 bytes (+355) and had no measurable impact on performance.)

Ok. The confusion on my side was that I removed --minify 0 and changed optimization to -Oz. Then at least (some) white space was removed from the dist file. I thought the effect was due to -Oz but it is --minify 0 that needs to be removed to get a minimum of minification. So I suggest to just remove --minify 0.

I do plan to use this in the browser, but for very small datasets that take 1-2 ms to run. Blocking the main thread for 1-2 ms is preferable to waiting 50 async ms in my case.

I agree, makes sense!

I didn't know Firefox doesn't support modules in workers. If someone asks for it, that'd be worth down-compiling to CJS again for.

I agree.

* Removed `-s SINGLE_FILE=1`. This saves 11.2 KB and, as above, is better for CSPs without `data:`. It makes it a bit harder to use as the .wasm file has to be served, but encourages better security.

I find it a bit odd to have two (completely identical) wasm files there. Why not change the output for the web target to -o $(DIST_DIR)/lmfit.js? Then it will build the wasm twice and overwrite it but at least both (node & browser) use the same file. What do you think?

So if we can agree on the two tiny issues I am ready to merge, build and do a release. And thanks again - enjoyed working with you!

@zbjornson
Copy link
Collaborator Author

Sorry for the delay. I've been reading up on how other modules distribute the wasm code. There are two small changes I want to make based on that reading (provide an option to specify the location of the wasm file, and probably also provide a single-file build as an option), in addition to addressing your two comments above. Back to you soon!

@jvail
Copy link
Owner

jvail commented Jan 19, 2023

Sorry for the delay.

Thanks for the feedback. No problem, I am not in a hurry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zbjornson @jvail