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

Web demo #45

Open
bminixhofer opened this issue Feb 26, 2021 · 21 comments
Open

Web demo #45

bminixhofer opened this issue Feb 26, 2021 · 21 comments
Labels
help wanted Extra attention is needed P3 Low Priority

Comments

@bminixhofer
Copy link
Owner

A web demo running client-side via WebAssembly would be really cool and useful. Ideally this should have:

The website should live in another repository and be hosted via GH pages. It should already be possible to implement with the library in its current state.

It's completely open how this is implemented (could be yew, or vuejs / react with a JS interface to nlprule made with wasm-pack, or something else).

It's quite a piece of work but it would be amazing to have. I want to focus on the other things first since they are more important for the core library.

Here I would appreciate contributions a lot!

@bminixhofer bminixhofer added help wanted Extra attention is needed P3 Low Priority labels Feb 26, 2021
@bminixhofer bminixhofer mentioned this issue Feb 26, 2021
5 tasks
@shybyte
Copy link
Contributor

shybyte commented Jun 7, 2021

I would like to work on this feature. As a first step I have tried to run the nlprule example from the README.md as WASM.
Unfortunately I spend some hours but still without success. The source of all problems seems to be the onig_sys dependency.
Currently the "wasm-pack build" fails because of:

rust-lld: error: duplicate symbol: OnigEncodingASCII
          >>> defined in /home/marco/workspace/private/wasm-game-of-life-2/target/wasm32-unknown-unknown/release/deps/libonig_sys-e54047c943a981c2.rlib(regexec.o)
          >>> defined in /home/marco/workspace/private/wasm-game-of-life-2/target/wasm32-unknown-unknown/release/deps/libonig_sys-e54047c943a981c2.rlib(regerror.o)

Do you know an example WASM project that successfully uses nlprule?
I have also added a comment here rust-onig/rust-onig#153.

@shybyte
Copy link
Contributor

shybyte commented Jun 8, 2021

After some hours of sleep, I have discovered that it seems to be possible to feature switch to "fancy-regex" in nlprule. I will try that next.

@shybyte
Copy link
Contributor

shybyte commented Jun 8, 2021

I have tried to use fancy-regexp instead of nlprule using

[dependencies]
nlprule = { version = "0.6.4", default-features = false, features = ["fancy-regex"] }

but it still tries to compile onig_sys.
Have I done something wrong?

@bminixhofer
Copy link
Owner Author

Hi, thanks for taking a shot at this!

onig-rs will not work with WASM, that's the reason the fancy-regex backend exists. But

[dependencies]
nlprule = { version = "0.6.4", default-features = false, features = ["fancy-regex"] }

looks right, I wouldn't expect onig to be used with that configuration.. Could you maybe check this into a repository so I can try compiling it?

FYI, I also test compilation to WASM in CI e.g. https://github.com/bminixhofer/nlprule/runs/2761121829 using cargo build --target wasm32-unknown-unknown --no-default-features --features regex-fancy -p nlprule but this may be subtly different to what wasm-pack does.

@shybyte
Copy link
Contributor

shybyte commented Jun 8, 2021

I have followed this tutorial https://rustwasm.github.io/docs/book/game-of-life/hello-world.html and after just adding nlprule to the cargo.toml, it fails to compile because it tries to compile onig_sys.
You can find the example repo here: https://github.com/shybyte/nplrule-wasm-example (oopsie, npl instead of nlp, don't get confused by this typo!)
This commit has added nlprule: shybyte/nplrule-wasm-example@eb1764b

It doesn't matter whether I try to compile it using

cargo build --target wasm32-unknown-unknown

or

 wasm-pack build 

both fails because of onig_sys.

@bminixhofer
Copy link
Owner Author

bminixhofer commented Jun 8, 2021

Ok, thanks! So there were two issues:

  1. nlprule-build uses the onig backend, so it doesn't work. I'm working to deprecate nlprule-build anyway, so I think the easiest way is for you to remove the dependency and just manually load the binaries (you can download them from https://github.com/bminixhofer/nlprule/releases/tag/0.6.4, then unzip and load using e.g. Rules::from_reader).
  2. the correct feature flag is regex-fancy so that all backends have uniform names (feature flags are regex-fancy, regex-onig and regex-all-test). So the correct dependency is
[dependencies]
nlprule = { version = "0.6.4", default-features = false, features = ["regex-fancy"] }

@shybyte
Copy link
Contributor

shybyte commented Jun 8, 2021

I have tried to follow your advice and now it compiles, but this function https://github.com/shybyte/nplrule-wasm-example/blob/master/src/lib.rs throws in the browser console:

bootstrap.js:5 Error importing `index.js`: RuntimeError: unreachable
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[1505]:0x6f80c
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[632]:0x5967f
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[857]:0x62886
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[1458]:0x6f0c0
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[1370]:0x6e0f3
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[1460]:0x6f124
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[949]:0x652ae
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[652]:0x5a60f
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[1482]:0x6f4c3
    at http://localhost:5000/82a50776f64bbf6b60fa.module.wasm:wasm-function[1227]:0x6ba52

Apparently already the line

    let tokenizer =
        Tokenizer::from_reader(&mut tokenizer_bytes).expect("tokenizer binary is valid");

is causing this exception.

Interestingly this line causes no exception:

let rules = Rules::from_reader(&mut rules_bytes).expect("rules binary is valid");

@bminixhofer
Copy link
Owner Author

OK, sorry, seems a bit bumpy to get this working but we'll get there :)

I just looked into this: it's a consequence of using rayon for parallelism. it is possible to make this work in the browser (https://rustwasm.github.io/wasm-bindgen/examples/raytrace.html) but quite complicated (and I'm not sure it would work with how parallelism is used in nlprule) so I believe the best option is to disable parallelism.

Unfortunately disabling parallelism is handled via an env variable at the moment (NLPRULE_PARALLELISM) which doesn't work in WASM. I'll think about how to fix this, in the meantime you can get around it by

pub fn get_parallelism() -> bool {
-    match std::env::var(ENV_VARIABLE) {
-        Ok(mut v) => {
-            v.make_ascii_lowercase();
-            !matches!(v.as_ref(), "" | "off" | "false" | "f" | "no" | "n" | "0")
-        }
-        Err(_) => true, // If we couldn't get the variable, we use the default
-    }
+    false
}

in https://github.com/bminixhofer/nlprule/blob/main/nlprule/src/utils/parallelism.rs#L26-L34 and depending on a local copy of nlprule.

@bminixhofer
Copy link
Owner Author

bminixhofer commented Jun 8, 2021

Oh and also you should probably do

utils::set_panic_hook();

at the start of your code so errors panics propagate properly.

@shybyte
Copy link
Contributor

shybyte commented Jun 8, 2021

Thank you for your help. By the way, this is my first encounter with Wasm.
Now it works and I managed to send a string from JavaScript to Wasm and get Suggestions back.
This means I have everything in place to start the actual work on a web demo. I guess it makes sense to start with the live text correction tool, because this is finally the main end-user use case.

@bminixhofer
Copy link
Owner Author

That's great! If there's anything else, please ask.

I've found for example https://quilljs.com useful for this kind of thing in the past, but I'm not that up to date with web development.

@shybyte
Copy link
Contributor

shybyte commented Jun 9, 2021

https://quilljs.com is a HTML editor. I don't know its API but usually handling HTML increases complexity because we would

  • need to parse the HTML, keeping an domnode/offset mapping to the original HTML document in order to add issue highlighting and replacements later
  • detect sentences in the HTML correctly, because some HTML tags indicate a sentence border but others normally not and there is not always a dot at the end.
  • if we highlight issues in the text, these highlighting can interfere with the HTML of the document done by the user.

All of this is solvable, but I would propose to start simple with simple plain text.

@bminixhofer
Copy link
Owner Author

I agree, an HTML editor would definitely be overkill. It shouldn't only be "start with plaintext", but "keep plaintext" in my opinion.

In the past I've disabled all of the HTML functionality and just used Quill to create some highlights in the text, that worked quite nicely. I'm also using Quill for the demo here for example: https://bminixhofer.github.io/nnsplit/

But there's definitely also other solutions. Keep me updated :)

@shybyte
Copy link
Contributor

shybyte commented Jun 19, 2021

@bminixhofer Caused by unexpected private events, I hadn't done anything for 9 day.
However yesterday and today I've found some time to

Unfortunately the results are a bit disappointing in regard to performance:

  • Loading all the code over the wire needs some time (this was expected)
  • Initializing the Tokenizer and Rules need 5 seconds, in which the browser UI thread is blocked.
  • Checking a short sentence like "She was not been here since Monday." needs 250 ms for the first check and 70ms for subsequent checks. For only 35 characters!
  • Checking the English 624 byte example text from https://languagetool.org/ needs 4 seconds for the first check and 400ms for subsequent checks.
  • The speed in chrome is 20x slower than the native code.
  • In Firefox everything needs even twice as much time.

(My CPU is a Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz)

Originally I had implemented a "Check as you type" experience (without an explicit check button) but this does not work, even for small texts, because the UI thread is blocked so long.
In order to improve the user experience I plan to move the wasm call into a web- or service worker, so they are at least not blocking the browsers UI thread.
However I wonder whether this poor performance is expected and can be improved?

My first suspicion was, that it's caused by a debug build, but "wasm-pack build" should build a release build by default and also adding the --release flag changes nothing.

@bminixhofer
Copy link
Owner Author

Hi, great work!! It's really cool to see this running in the browser :)

However I wonder whether this poor performance is expected and can be improved?

No, WASM should run at near-native performance. My first thought also was that it might be a debug build but it seems you checked that.

I think it might have to do with running on the main thread, maybe you could try running it in a web worker since as you said, it would be better anyway not to block the UI.

On my machine (Intel(R) Core(TM) i5-8600K CPU @ 3.60GHz), a check of the sample text usually takes 27ms, but interestingly after refreshing the page a couple of times, sometimes consistently only 5ms. 5ms are actually somewhat reasonable, so that may be another hint that the issue is running on the main thread.

If running in a web worker doesn't solve the issue, I can do some profiling to check where the slowdown is coming from.

Also, 8.66MB wire size for the WASM is surprisingly good, I guess the code adds almost no overhead to the raw binary size.

@shybyte
Copy link
Contributor

shybyte commented Jun 20, 2021

I have now implemented a webworker based version: https://github.com/shybyte/nlprule-web-demo/tree/main/webworker-example
This version is now deployed to https://shybyte.github.io/nlprule-web-demo/

The unblocked UI thread is a bit nicer in regard to UX, but unfortunately the actual performance is equally bad 😢

This isn't a urgent blocker for further development of the web demo, but the result won't be very impressive with the current wasm performance. I doesn't matter so much for a short demo text, but if you want to check a medium size blog article and it needs 15 seconds for a check and re-check, than it becomes unwieldy.
There are for sure ways to improve the experienced performance on the JavaScript side by tricks like:

  • Split into sentences first and then parallelize by using multiple webworkers.
  • Use caching on sentence-level for faster re-checks.
  • Stream the results sentencewise while incremental checking, to minimize the time until first visible results.

However, these are tricks that should be in best case not be necessary, if wasm would deliver what it promises.

In order to verify that it's not caused by a debug build, I have recompiling it with wasm-pack build --dev and the result was even much slower than wasm-pack build --release or wasm-pack build (which should default to --release).
I'm not sure what else could go wrong in this regard,

All this said, I think for now performance is OKish for a web demo (assuming that we don't want to demo performance itself 😄 ).
There are for sure other more important areas of possible improvement, like for example a spellchecker or the possibility to write rules in rust.

@shybyte
Copy link
Contributor

shybyte commented Jun 25, 2021

@bminixhofer
I have deployed a first end-user-usable version of the web demo here https://shybyte.github.io/nlprule-web-demo/ .
The code is here: https://github.com/shybyte/nlprule-web-demo/tree/main/webworker-example-solid

It's based on https://codemirror.net/ and https://github.com/solidjs/solid.
I use codemirror because I have good experiences with it and I use solid-js because have no experience with it, but was looking for a reason to try it out 😄

The current state should look like this:

image

The corrections are marked in the CodeMirror editor and also listed as cards on the right.
Clicking on the cards selects the corresponding issue text in the editor and vice versa.

It's possible to apply the replacements by clicking on them.

It's far from perfect, but we are not so far from an usable open source Grammarly replacement, running locally in a web browser with full privacy, because nothing is sent to a server. How cool is that!

@bminixhofer
Copy link
Owner Author

Really, really, cool. Great work.

I knew neither codemirror nor solid.js, but seems like good choices 🙂

I've thought a bit about the speed issue. Without taking a closer look, I guess there's probably two reasons:

  1. WASM is optimized for size, not speed.
  2. nlprule uses some semi-heavy vector calculations in the chunker, ndarray might optimize this better for native code than for WASM.

With (1) you could actually play around a bit if you haven't already. You could try another opt-level besides s in the Cargo.toml and check how that influences the size / speed tradeoff. There's also lto = true and codegen-units = 1 which might improve speed a bit.
There's nothing we can really do about (2), but it would be interesting how German compares to English in comparison to native speed, since German does not use a chunker.

That said, with nlprule running in a WebWorker now the demo actually feels very smooth to me. I think the biggest improvement to speed is as you said splitting into sentences first and caching the sentence-level results. That will actually be easily possible with #72, so another reason to try to get that merged :)

@shybyte
Copy link
Contributor

shybyte commented Jun 28, 2021

I have now enabled all possible optimize-for-speed knobs I could find:

[profile.release]
opt-level = 3
lto = true
codegen-units = 1

# https://rustwasm.github.io/docs/wasm-pack/cargo-toml-configuration.html
[package.metadata.wasm-pack.profile.release]
wasm-opt = ['-O4']

and the resulting WASM is around 10% faster with more or less equal size.

@shybyte
Copy link
Contributor

shybyte commented Jul 1, 2021

I'm currently experimenting with different ways to optimize the end user performance on the JavaScript side like:

  • Split into sentences first and then parallelize by using multiple webworkers.
  • Use caching on sentence-level for faster re-checks.
  • Stream the results sentencewise while incremental checking, to minimize the time until first visible results.

As an side-effect, the main demo is not suited anymore for performance test of the base nlprule-wasm code.

However I have done (using https://github.com/shybyte/nlprule-web-demo/tree/main/webworker-example) some unscientific manual raw-performance measurements which might be interesting:

Test/Mode Native WASM chrome Native 1. execution WASM chrome 1. execution
Initialization 420 2200
correct 350 1730 400 4000
tokenize 87 490 100 700
sentencize 27 207 32 210

Correct includes tokenize and tokenize includes sentencize.
All duration are in ms for a 1000 word text.
I have disabled parallelism for Native to be fair.

All code was compiled with:

[profile.release]
opt-level = 3
lto = true
codegen-units = 1

From these results I would conclude that WASM (chrome) is around 5-10 times slower in all areas, so there is no obvious bottleneck (sentencize/tokenize/correct) to fix, but executing the rules dominates the costs, especially for the first execution. (But please note, that these measurements are very unscientific)

@bminixhofer
Copy link
Owner Author

Interesting. Thanks!

The demo feels very smooth now! The numbers are a bit disappointing though. Others apparently found a ~50% decrease compared to native (https://www.usenix.org/conference/atc19/presentation/jangda) but 5-10 times slower is another big jump. I still feel it's likely that there is some issue either in nlprule or in the WASM <-> JS interaction that makes it that slow.

If you have time and interest to continue in this direction, you could try replacing the sentencize function with https://docs.rs/srx/0.1.3/srx/. This is actually used under the hood in nlprule, but sentencize also does e.g. chunking so it's more difficult to reason about performance, while the SRX sentencizer only executes a couple of regular expressions. If the 5-10 times slower perf persists with SRX it would be easier to track down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed P3 Low Priority
Projects
None yet
Development

No branches or pull requests

2 participants