Recompile Emscripten binaries, implement WASM with fallback, and add instructions and scripts

[Jaifroid]

This PR is for implementing #739.

I've done the Windows implementation and documentation, with a short PowerShell script that runs everything. It works like a dream -- so much easier than what I had to do before!

@mossroy Would you be able to do the equivalent linux script (no rush)? It's probably just a one-liner and very easy for you to test on your linux machines. Take a look at the script Compile-Zstddec.ps1 in this PR.

I'm not sure whether we should add equivalent xzdec compilation to this PR to make it generic. For the moment it's just zstddec.

[mossroy]

I'm on the bash script

[mossroy]

I've commited my script.
It's reproducible : if I run the build several times, and from different computers, it gives the same result

[Jaifroid]

Great! I'll try to reproduce mine in Windows sandbox, as the only other machine I have is very ancient and would get clogged up if I try to install docker desktop on it.

[Jaifroid]

I'll do some testing of these versions, and in particular try to get some results for #652 (comparing the wasm and the asm versions). I needed to recompile due to the memory issue with the Promise polyfill branch (as I wanted to test with that version which is faster). On a quick test with the old wasm from last year (in one browser), just judging by how it "feels" as a user browsing a ZIM, the wasm version is still not any faster than asm, but this might have changed with the newer Emscripten.

However, note that if we don't merge the Polyfill PR, then we probably don't need to compile these with the higher memory value I've included in this PR, as we only need more Emscripten memory for stability with that PR.

So long as the wasm version isn't slower, I could push my code that selects wasm if the browser supports it, or asm if it doesn't. It's not complicated, a couple of lines. Would it be best to push that to this PR (and change title) or to separate one? I think that one would depend on this one (and they both to some extent depend on the Promise Polyfill PR).

Final step would be to clone these scripts to recompile xzdec and maybe include a wasm version of that too.

[Jaifroid]

Newly compiled zstddec-asm.js is failing on IE11 due to a (probably) trivial issue (see screenshot). May need to open an issue at Emscripten, but should check first that the flags haven't changed. I definitely included -s MIN_IE_VERSION=11 in the ASM version (I removed it for the wasm version).

[Jaifroid]

I opened emscripten-core/emscripten#14700. However, that GitHub is swamped with issues and PRs, so it wouldn't surprise me if this is overlooked. In the meantime, we can easily polyfill the startsWith method from https://developer.mozilla.org/en-US/docs/web/javascript/reference/global_objects/string/startswith#polyfill.

[Jaifroid]

I confirm that the only polyfill needed to allow this asm to run on IE11 is:

if (!String.prototype.startsWith) {
    Object.defineProperty(String.prototype, 'startsWith', {
        value: function(search, rawPos) {
            var pos = rawPos > 0 ? rawPos|0 : 0;
            return this.substring(pos, pos + search.length) === search;
        }
    });
}

I'll add it to this PR in zstddec_wrapper.js, pending any comments from Kripken.

[Jaifroid]

Kripken did reply on that issue, suggesting I make a PR for the polyfill... I don't know whether I can do that (I've actually suggested a simpler alternative), but in any case it looks like I should add the very short polyfill -- maybe to zimfile.js -- with a note to remove it if it's not needed in a future Emscripten release.

[Jaifroid]

I've added in the newly compiled wasm and asm modules (zstandard only, not xzdec yet), and I've made them conditionally loadable, so that IE11 still works and uses ASM, and browsers capable of running WebAssembly use the WASM. The WASM version is not any slower than ASM (in any one browser) on my quick subjective tests, and may be a little faster. What's sure is that it is a much smaller module to load and should run on a dedicated low-level system (though ASM apparently runs on its own precompiled thread too).

Just bear in mind that this branch doesn't have the Promise polyfill yet (that's a separate PR), so it's still using Q, and doesn't benefit from the speedup we see in that branch.

Now, should we try to do the same for xzdec? The compile string will need to be changed, because we last compiled on an older version of EMSDK, and some of the options have changed names.

[Jaifroid]

@mossroy Just to let you know, we need to modularize xzdec, which also means refactoring xzdec_wrapper, to get it to work in the same way as zstddec. I need to push a change to the EMCC compile command to do that.

[mossroy]

@Jaifroid : OK sorry I leave the keyboard for now, then.
I've commited the corresponding bash script for xzdec, that seems to work well
I had also commited the output asm and wasm files, feel free to regenerate after your changes

[Jaifroid]

No problem, your changes won't conflict with what I'm doing, but we will need to recompile xzdec again when I've pushed the relevant changes (but that's easy now).

[mossroy]

Congrats for finding and reporting a bug in emscripten! If you don't manage to do the PR they suggest, I might help

[Jaifroid]

Feel free! I won't be working on that (handling what needs to be done with our code is plenty enough for me!!). On that issue, there is an Emscripten dev who seems to have taken an interest, but it's not clear if they are proposing to do the work or not...

[Jaifroid]

Hmm. Latest commits are failing on one browser (Chrome 58, Window 7) in compiling the new xzdec WASM -- see below. This is probably one of the first browsers to provide WASM support and it may well have been an incomplete implementation in that version. How should we handle this? Some kind of fallback to ASM if WASM fails to compile? Might be hard to code for that...

Chrome 58.0.3029.81 (Windows 7) WARN: 'failed to asynchronously prepare wasm: CompileError: Compiling WASM function #23:<?> failed:Result = inconsistent arity in br_table target 9 (previous was 0, this one 1) @+2624
'
......................
Chrome 58.0.3029.81 (Windows 7) WARN: CompileError: Compiling WASM function #23:<?> failed:Result = inconsistent arity in br_table target 9 (previous was 0, this one 1) @+2624

CompileError: Compiling WASM function #23:<?> failed:Result = inconsistent arity in br_table target 9 (previous was 0, this one 1) @+2624

    at http://localhost:9876/base/www/js/lib/xzdec-wasm.js:14:530
    at <anonymous>

[Jaifroid]

I thought I'd be clever and get round the Chrome 58 WASM compile error by detecting it and reloading the app in ASM mode. It worked, but Sauce Labs doesn't like it and complains (see screenshot)...

So now I'm stumped. I downloaded a version of Chrome 58 and it indeed cannot compile the WASM. The problem is with that early version of Chrome, not with our modern WASM. My solution works for the user: they just get a momentary reload during the load sequence, which is hardly noticeable. But it doesn't work for testing. Any ideas as to how I can abort and change the RequireJS definition "on the fly", in a generic way, without doing window.reload()?

Last resort might be to change the Chrome version we're testing against? I don't suppose we should introduce some specific test for a specific build of this browser in the code (if that's possible), just to get around testing?

[Jaifroid]

OK, I ditched the reload idea, and fixed it instead with a fallback require() statement. I still set a value in localStorage to make sure ASM is used next time with that module. Code may need some refinement, but the principle works.
Not sure if I should do the same for zstddec (that one seems to load without problem in that browser, though we don't actually test decoding with it).

[mossroy]

Your latest approach (use requireJS to load ASM if loading WASM fails) looks the best one to me.
But I don't think it's a good idea to store a "boot-with-asm" flag. Think of the scenario of a Chrome 58 on which you open kiwix-js, and then Chrome is upgraded to latest : it will keep using the ASM version instead of the WASM.
I understand you try to avoid useless CPU cycles, but we're talking about a very rare case (only one specific version of Chrome) : I think it's better to let it try to tun the WASM version every time

[mossroy]

I would be in favor of adding the same fallback to ASM if loading the WASM file fails

[Jaifroid]

But I don't think it's a good idea to store a "boot-with-asm" flag. Think of the scenario of a Chrome 58 on which you open kiwix-js, and then Chrome is upgraded to latest : it will keep using the ASM version instead of the WASM.

I agree. It has drawbacks, so better to test each time.

[mossroy]

I did not have the time to work on emscripten-core/emscripten#14700
The PR code is probably not complicated to do.
What would take more time is to test it does what we need. It involves setting up an emscripten development environment, and being able to run our emscripten compilation with a newly-compiled emscripten binary.
As they say they don't have any CI on IE11, I suppose it's not possible to add an automatic test

[mossroy]

I could not test, just added a few comments in the code

[Jaifroid]

I've had a look in settings.js and found

• MINIMAL_RUNTIME_STREAMING_WASM_COMPILATION = 0; and
• MINIMAL_RUNTIME_STREAMING_WASM_INSTANTIATION = 0;.

However, oddly, it says that streaming instantiation is disabled by default. The only thing I can think is that we have not set the option for the MINIMAL_RUNTIME (above those two options).

It could be worth experimenting with? I don't know if it's necessary just to eliminate a fallback warning in browser consoles, and of course our code might not work with the minimal runtime as I think we possibly need XHR to load the WASM binary.

[mossroy]

This branch works fine on my Firefox OS device. It chooses the ASM versions (as WASM is unsupported) and does not complain. I did not see any obvious performance difference.

It might be worth testing the "minimal runtime" option you suggest.
Anyway, I don't consider the console warning to be a blocker, if we don't manage to get rid of it.
If you prefer, this PR could be merged, and we might investigate further with another issue/PR.

Another improvement could be to display in the Settings section :

• if WASM is supported by the browser
• which version (ASM or WASM) is used for each decoder
  But it could be a different issue too, and is not a blocker at all

[Jaifroid]

Yes, populating the API panel could provide good diagnostic info to savvy users.

[Jaifroid]

I've added code to populate the API Status Panel. It was trickier than I thought to get right given the async nature of the errors happening. I've included in zstddec-wrapper and xzdec-wrapper some throw statements that can be uncommented to simulate what would happen if there is an error loading one or other of the decoders. To test error reporting, uncomment both statements in one or both of the files, save and reload fully (usually ctrl-shift-r in the dev panel, though sometimes even that doesn't defeat caching...).

Some images below of what would be seen in a) an error situation, and b) normal operation. Please note that the actual decoder being used ('ZSTD' or 'XZ') in square brackets after the machine type ('WASM' or 'ASM') will only be displayed once a ZIM file has been loaded. It will always report the last decompression type used. Though extremely unlikely in practice, a ZIM could hypothetically use ZSTD for some clusters and XZ for some others. The only way to know the decompression type is at the moment a Decompressor is requested. Code to deal with any particular decompression type is set up in its wrapper, so the code should be extensible to future codecs.

EDIT: Once an error has been detected, it will always be shown in the API panel during that session, even if the user is using the other decoder and that loaded normally. The error occurs for any particular decoder only if all attempts to fall back to asm have failed, as well as wasm. IMHO it's better to show that there's a problem, than to hide it just because the needed decompressor is working. Otherwise the user won't know why the app hangs when opening some ZIMs and not others. In practice it would be rare for one decoder to load and the other not to, but we did come across a situation above that was similar in an old Chrome.

[Jaifroid]

I've run out of time to deal with the console warning, i.e. to experiment with different values when compiling xzdec and zstddec. We've made it fairly easy to do that (it's the testing that would be time-consuming). So I think it's best to set that as a new issue.

@mossroy Please let me know if the implementation of the Status Panel code is satisfactory for merging (in due course). I have tested it on IE11, Firefox latest, Edge Chromium. I have yet to test on Edge Legacy or UWP.

[Jaifroid]

Just noticed a silly minor error... Will push a very small change.

[Jaifroid]

@mossroy Apart from the minor changes I've indicated in comments above, are you happy for me to squash-merge?
No rush -- it's August!
PS I've now tested on UWP (which is same browser engine as Edge Legacy).

[mossroy]

It seems to work well on my FirefoxOS device :-)
I did not look at the code yet

[mossroy]

I did not have a very deep look, but it looks good to me

[mossroy]

It's working fine on Firefox OS (orange) and latest Firefox/Chromium (green).
Ready to be merged for me

[Jaifroid]

OK, will do.