feat: wasm support and tests #32
Conversation
Signed-off-by: Lucas Alessandro do Carmo Lemos <[email protected]>
|
thanks @On0n0k1 we may not have time to review this right away, but I see you manage the dependencies more precisely now |
|
hey @On0n0k1 do you have a way to add a targer in the Makefile to run the wasm tests perhaps ? Or how are we supposed to run the tests ? :) |
|
Hello @IceTDrinker . To run the tests, there are two ways: a simple and another more complicated way.
I recommend the "more complicated" way because it is less of a black box. I can look into the makefile and include the change for the test. Do you have a recommendation or preference on where or how I should make change? |
|
Ok for the more complicated one as I agree wasm pack does a lot of magic somewhere around here Line 107 in 83f2441 I guess with maybe a recipe to easily install the right wasm bindgen if required |
|
and making sure the test_wasm is in the test_all |
|
In what runtime do you use the makefile? One of the issues I've been having deciding is that we need runners for the tests. One of the tests require Node, the other requires a browser like Firefox or Chrome. So we would need these installed in the test environment. |
|
that's a good point, we have some infrastructure in the main TFHE crate, we might use some of that here, we'll keep you posted, it is not currently the highest priority on our end, is it something that you actively need ? |
|
Yes. With this fix we can use the server key in webassembly, including NodeJs and browser. It opens a lot of options for doing things like "adding homomorphic encryption to a NodeJs instance", "adding homomorphic computations to browser libraries". It will be great for refactoring production systems with the technology. |
While I understand you want it, be aware that the 32 bits nature of wasm and the fact it is very slow means it won't be usable in practice. Were you able to test some production-like workflows for testing with this fix ? |
|
Yes. Multiplication and division between ciphers is still too slow to be viable. But, by choosing the right configurations, the timing for scalar computations is still good. Would you like me to prepare a benchmark later to show the results? |
It's not necessary, but if you have some benchmarks with the underlying hardware and the wasm executor we'll take them ! |
|
I don't have it ready here to show at the moment. Will take some time to prepare. But I've been doing computations in hundreds of integers in the browser with acceptable time for a while (1 to 5 seconds). What I'm excited about it, is that computation done in the browser is technically "free" cost. The user may get a loading bar as it is waiting the computation to end in a few seconds, but the backend doesn't need to pay for that cost. No cloud service fees with browser computation. Also, when encrypting data, it can be much faster to encrypt the integer 0, then create copies of it, adding the value we need using the server key. |
|
Could you maybe describe the use case ? I'm a bit surprised that you want FHE computation in the browser instead of a server 🤔 |
|
Imagine if you have a network of browsers communicating with one another using Homomorphic encryption. No server. Just several browsers doing the computations securely between one another. A browser could take the role of the server that way. By arbitrarily doing certain computations on the data without knowing what it is. Imagine the potential that tech like that may have in environments like blockchain... |
does that require a browser though, could be a P2P native client ? Anyways thanks for the insights and what you had in mind |
|
How about I remove the browser test and we just keep the NodeJS one? It will still work in the browser the same way. There will be only need to have NodeJs installed in the test machines, which is a lot easier to handle. |
Should be ok, though jest with puppeteer can manage browser tests, that’s what we have in TFHE-rs when needing actual browser tests and sorry for the late reply we have had a busy release @On0n0k1 |
|
@On0n0k1 any reason you closed the PR ? As I said we have been busy lately we are not against improving platform support but as it's not the priority on our end we dedicate the time we can to it |
|
@IceTDrinker Sorry I thought you didn't consider the update necessary. I was thinking how I could convince you to include the fix. So I was thinking about implementing a browser benchmark of tfhe-rs doing the computations with the server key in wasm. A web page that displays the speed of the computations as a table. I closed it because I would reimplement the changes, maybe by also handling the issue #22. I'm not sure. I don't have much time to work on this either. So if there is anything else I should include in the changes, besides what we already discussed, please let me know. |
|
so issue 22 is more on our end of things, let's keep this PR open, don't hesitate to bump this when you potentially need it more or have some way to update testing, as already indicated we have some wasm tests in the zama-ai/tfhe-rs repo so with the change you proposed we may be able to provide the functionnality and have tests for it Note however that it's not clear that the code will properly work for wasm32 as TFHE-rs is designed for 64 bits systems, even with an fft update it might not work, just keeping expectations reasonable here |
|
we currently have very limited bandwidth for things we don't have on the roadmap, so don't expect automatic progress on our end for now :) |
|
hey, still no bandwidth on our end for now, sorry about that |
| @@ -0,0 +1,23 @@ | |||
| #[cfg(target_family = "wasm")] | |||
There was a problem hiding this comment.
the cfg can be put in src/time/mod.rs over the mod wasm instead of having it in this file
| start: f64, | ||
| } | ||
|
|
||
| #[cfg(target_family = "wasm")] |
| @@ -1057,7 +1057,7 @@ fn bit_rev_twice_inv(nbits: u32, base_nbits: u32, i: usize) -> usize { | |||
| bit_rev(nbits, i_rev) | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| #[cfg(all(test, not(target_arch = "wasm32")))] | |||
There was a problem hiding this comment.
could you remind us what the issue is for tests on wasm ?
There was a problem hiding this comment.
One of the libraries used for generating random values for the tests breaks on my linux machine. I had found the exact dependency that caused it. It was such a long time ago that I forgot which was it. Those tests will run when I test for wasm. Meaning it will start a library that will break on my machine every time. My guess is that it thinks that my Linux machine is a mac.
This is why I had to disable those tests. I apologize for the issue caused by my hardware.
Since those tests couldn't be run in this machine, I can't guarantee that they will run well in your systems compiled to wasm. I will leave that to you.
Making these changes allowed a webpage to generate a server key and do computations on encrypted data. It may be a bit slower, but it didn't seem impactful from the user's perspective.
|
ever managed to discuss with some wasm folks about the infinite loop ? @On0n0k1 |
| @@ -119,6 +119,7 @@ macro_rules! izip { | |||
|
|
|||
| mod fft_simd; | |||
| mod nat; | |||
| pub mod time; | |||
There was a problem hiding this comment.
i don't think this should be pub
I found the exact reason why it was running in an infinite loop a few months ago but I forgot by now. I could check it again then explain directly later. It could be a small video or a meeting if necessary. edit: See below. |
| @@ -59,6 +59,7 @@ pub enum Method { | |||
| } | |||
|
|
|||
| #[cfg(feature = "std")] | |||
| #[cfg_attr(target_family = "wasm", inline(never))] | |||
| fn measure_n_runs( | |||
There was a problem hiding this comment.
This function is what caused the infinite loop. By including the inline(never), the infinite loop stops. This is because the compiler is incorrectly optimizing this function. It assumes that n_runs is a constant. Therefore it will "optimize" (delete) the code that makes the loop work properly. It will be forever running the same instruction over and over again.
There was a problem hiding this comment.
I understand that the optimizer is wrong here so something is likely broken somewhere, as some of those functions at least the time measurement has a side effect and so should not be skipped, so this bug should be reported somewhere if we manage to get a minimal repro
There was a problem hiding this comment.
I agree. While I can assure about how the bug happened. I'm still not sure how to replicate it yet. Basically I was logging variables when it was running and suddenly it was working when I logged the variable n_runs.
Once we managed to replicate it, maybe we should report it to the wasm32-unknown-unknown compiler community? Since it only happens in wasm.
There was a problem hiding this comment.
I understand but yes once we have something that looks like it reproes the issue we should find a way to share it with the rust project, worst case they can dispatch it themselves afterwards
| @@ -59,6 +59,7 @@ pub enum Method { | |||
| } | |||
|
|
|||
| #[cfg(feature = "std")] | |||
| #[cfg_attr(target_family = "wasm", inline(never))] | |||
| fn measure_n_runs( | |||
| n_runs: u128, | |||
There was a problem hiding this comment.
Another way to stop the infinite loop from happening. Just call a console.log inside here printing the value of n_runs. The compiler will be unable to optimize the function. Resulting in it working properly.
There was a problem hiding this comment.
inline never is good enough I would think, but it needs a comment as to why it is required would be welcome
Here is the bug fix for webassembly support to
tfheandconcrete-fftfor server key deserialization.To run the tests, all that is needed is to have
wasm-bindgen-cliinstalled and runcargo test --release --target wasm32-unknown-unknown. This link shows how to setup ci-cd for it: https://rustwasm.github.io/wasm-bindgen/wasm-bindgen-test/continuous-integration.htmlHere is a description of the changes:
.cargo/configtells cargo to use wasm-bindgen runners for the test;rugorrandto avoid a panic;std::time::Instantdoes not compile. Implemented a custom Instant type that will use the typeDateto retrieve the current time.<to<=. Without this change, this conditional would always be false;measure_n_runs. Currently there seems to be a bug with the optimization that causes the variablen_runsto be considered an irrelevant constant that can be optimized. Without this variable, the loop that calls this function will keep running infinitely. Forcing the function to beneverinline fix this issue. Try this yourself. Comment that singleinlineinstruction and run the test, the functions will go back to an infinite loop again.NodeJsandBrowserruntimes. This is enough to prove that the "infinite loop" exists and is fixed by these changes. But I believe you may be interested in adding or change more tests.Please let me know if there is any issue with these changes. I tested locally with the crate
tfhe, and it is able to generate and do computations with the server key properly.