-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add a setting to choose which backend to use (javascript or libzim wasm) #870
Comments
Interesting. Do you see this option as a drop-in replacement for our current backend? Can we use it (initially) the way we use our current |
I'm sure it won't be that easy. But the prototype is encouraging: as you can see in https://github.com/kiwix/kiwix-js/pull/766/files#diff-484c9e8aa09e09b8eb1fafaf55e8eadcd0eaab230e38d02bb9440841bfa090a4R1579 , I could somewhat easily replace the backend. It can certainly be modified to support both backends. Currently, this branch still uses our javascript backend for everything except browsing through the ServiceWorker: at least reading the ZIM metadata, finding the welcome page, searching, going to a random article AND all the jQuery mode I suspect searching could be a more difficult part, because it has been optimized quite a bit, and the libzim API might not correspond to the way we handled pagination for example Regarding the jQuery mode, I'd suggest to leave it as it is (on our javascript backend) to save some work (at least at the beginning). |
It never is... But if we can start to use it experimentally in a (sort of) drop-in manner, then it will be much easier for us to develop it over a period of time, optimize it, in the same way that Service Worker mode was developed from a buggy start to production-ready code now. We have to manage the risk of proliferating "modes", I think.... (xz-zstd-asm, xz-zstd-wasm, libzim-wasm, libzim-asm). I agree that we should work with wasm only first. |
Now that I have integrated search, it should be relatively easy to integrate getting BLOBs from the libzim version instead of our custom back end. I think it shouldn't be the default at first, but an experimental option (under Expert Settings) that a developer or expert user can turn on in order to experiment. The reason for this is that it is largely untested, and the prototype was producing memory leaks (#872). Integrating this as an Expert Setting would allow us to test more easily. N.B. we won't be able to replace our current back end completely, because the WASM version works only with the latest Chrome, Edge and Firefox, and in particular it currently requires Atomic Operations, which are not available even in earlier WASM versions that were otherwise very stable (e.g. Edge Legacy), let alone in browsers that only support ASM. So, although we can easily compile to ASM, and the ASM version works well (and quite fast for search), it's not actually supported on older browsers unless I can find a way to turn off pthread support in the C++ code at compile time. See openzim/javascript-libzim#17. |
@Jaifroid What are the browser most recent versions which are not able to work with javascript-libzim. |
@kelson42 According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics#browser_compatibility, the versions that work would be Chrome 68+, Edge 79+, Firefox 78+. Emscripten uses Of less relevance for our target apps (though it is of relevance for the PWA version), Chrome/Edge for Android does not work (not because Atomics are not supported, but possibly because of problems transferring the ArrayBuffer to the Web Worker), and Firefox for Android has a different issue that also affects our current backend: it tries to copy the entire ZIM File into memory, and fails with archives larger than a couple of gigabytes. |
@Rishabhg71 As discussed, we have a "working" prototype version in #766, but without any UI option to select between backends, and relatively limited (as per discussion above) in terms of what libzim is actually used for. I think this issue could be tackled first in a "light" way. The current code in #766 is monkey-wrenched, so some (but not too much at this stage, see below) code re-organization would be needed to separate the pathways a bit more and to make use of libzim for article loading agnostic as to which mode the user has requested (Service Worker or JQuery). The first step would be to add the UI option, so that we can more easily "road test" libzim reading. I've already uncovered a blocker with an internal WASM crash systematically produced on a number of ZIMs, seemingly (but not clear) due to lack of error handling when some ZIM assets are missing. See openzim/javascript-libzim#64. Note that in current main branch, libzim is already loaded and inititalized shortly after an archive is loaded (in JavaScript) by the code in The reason why this issue should be done in a "light" way is that the most logical next step would be #853, which would be a larger undertaking. But that would depend on fixing blocking issues with the libzim WASM first. |
@Jaifroid I have already started working on this issue. These are things that I have already accomplished
I do have a few questions which I will ask later on. Here are the next steps which would be much better.
adding more code or using the current code doesn't seem like a good idea to me It's bound to break some day. I will keep my changes on other branches and after everything is done it will be merged |
@Rishabhg71 Compiling libzim with WASM is an extremely complicated endeavour, and the main work has already been done in https://github.com/openzim/javascript-libzim/, as I mentioned to you. Any work you do on improving the compilation should build on the existing WASM-Emscripten compilation and be submitted as a PR to https://github.com/openzim/javascript-libzim/. While it's great that you're trying to understand the technology, I suggest you first start with this issue on Kiwix JS, as we discussed, and when that is done, we will have a firm base for road-testing the WASM and ASM builds produced by javascript/libzim. Only then will it become clear where the build code needs to be improved. That's my recommendation! |
I am using the code that is already written in |
OK, understood! Of course, take the time you need to understand the codebase, and let me know via Slack if you have any queries I can help with. |
Fixed in #1160. |
It should be automatically set to libzim wasm by default.
There should be a warning displayed to the user if it's not supported, and fallback to our current javascript backend
For a first version, I'd recommend to use only the wasm binary generated by emscripten.
It should allow us to more quickly release a first version with libzim backend for recent browsers: https://caniuse.com/wasm
We'll have time afterwards to consider if we try to compile libzim with asm.js too, to be able to support older browsers
The text was updated successfully, but these errors were encountered: