feat(core,ffi): Add Swift tests for MSM benchmarking #56
Conversation
FoodChain1028
changed the title
|
Not quite sure why flag isn't respected when running tests. I mentioned this in DM, but the easiest workaround is probably to hardcode the flag while testing. Did you try this? When I do this I don't get the above error. |
Yes, it worked. I will add the content in README.md for people who want to run the test again making |
| @@ -1,4 +1,6 @@ | |||
| uniffi::build_foreign_language_testcases!( | |||
| // "tests/bindings/test_mopro_gen_benchmarks_report.swift", // uncomment this to generate the benchmarks report in swift | |||
| "tests/bindings/test_mopro_gpu_benchmarks.swift", | |||
There was a problem hiding this comment.
I think you can gate this whole block with the feature flag
There was a problem hiding this comment.
Thank you. I added a feature flag for this function. (seems like it is not allow to add a feature flag inside the function parameters)
|
Btw, seems like previous PR introduced this error when running Issue: #61 |
|
I found that the Check the demo branch with To reproduce error: To reproduce success compilation: |
|
Moreover, the The tests in With the Check the demo branch with |
FoodChain1028
changed the title
mopro-ffi/Cargo.toml
Outdated
| @@ -14,7 +14,8 @@ name = "uniffi-bindgen" | |||
| path = "uniffi-bindgen.rs" | |||
|
|
|||
| [features] | |||
| default = [] | |||
| # default = [] | |||
| default = ["gpu-benchmarks"] | |||
There was a problem hiding this comment.
Not sure we want to keep this on by default, might interfere with other Anon-Aadhaar integration work
There was a problem hiding this comment.
Sure, we can keep the default clean and comment on the one with "gpu-benchmarks" feature at this moment.
We found that the external swift and kotlin tests mopro-ffi/tests/bindings only take the default features in mopro-ffi/Cargo.toml. If we can find a way to have the tests in bindings take --feature flags, we will remove the # default = ["gpu-benchmarks"].
| // "tests/bindings/test_mopro.py", | ||
| "tests/bindings/test_mopro_keccak.swift", | ||
| // "tests/bindings/test_mopro_keccak.kts", // FIXME: java.lang.OutOfMemoryError: Java heap space | ||
| "tests/bindings/test_mopro_keccak2.swift", |
There was a problem hiding this comment.
What are all these other stuff? Are you using more than "tests/bindings/test_mopro_gpu_benchmarks.swift",?
There was a problem hiding this comment.
I think we can keep these two msm-related test under #[cfg(feature = "gpu-benchmarks")] and other swift tests under #[cfg(not(feature = "gpu-benchmarks"))]. What do you think? @FoodChain1028
#[cfg(feature = "gpu-benchmarks")]
uniffi::build_foreign_language_testcases!(
"tests/bindings/test_mopro_gen_benchmarks_report.swift",
"tests/bindings/test_mopro_gpu_benchmarks.swift",
);
#[cfg(not(feature = "gpu-benchmarks"))]
uniffi::build_foreign_language_testcases!(
// OTHERS
);There was a problem hiding this comment.
I'll make it like this.
| @@ -1,3 +1,21 @@ | |||
| #[cfg(feature = "gpu-benchmarks")] | |||
| uniffi::build_foreign_language_testcases!( | |||
| // "tests/bindings/test_mopro_gen_benchmarks_report.swift", // Uncomment to generate benchmarks report, takes nearly 10 minutes | |||
There was a problem hiding this comment.
Shouldn't need to do uncomment if gated by feature flag afaik
| do { | ||
| // for tracking the progress | ||
| print("Running benchmark with \(each) MSMs...") | ||
| let benchData: BenchmarkResult = try runMsmBenchmark(numMsm: each) |
There was a problem hiding this comment.
So for 10k MSMs it will do all these in Rust and only return results once done, right? Just trying to understand if it is going back and forth between swift and rust or something
There was a problem hiding this comment.
It seems like the test script is running directly through swift command according to this.
mopro-ffi/README.md
Outdated
| ## Generate MSM benchmark report in swift on laptop | ||
|
|
||
| * Enable `gpu-benchmarks` feature as default in `Cargo.toml` | ||
| * Uncomment the 3rd line in `mopro-ffi/tests/test_generated_bindings.rs` to enable report generation |
There was a problem hiding this comment.
I don't think this is needed any more
There was a problem hiding this comment.
I reckon the first bullet should be kept since the default is now default=[], but I will change the description to better understand the meaning
mopro-ffi/README.md
Outdated
|
|
||
| * Enable `gpu-benchmarks` feature as default in `Cargo.toml` | ||
| * Uncomment the 3rd line in `mopro-ffi/tests/test_generated_bindings.rs` to enable report generation | ||
| * run `cargo test --test test_generated_bindings --features gpu-benchmarks --release` |
mopro-ffi/README.md
Outdated
|
|
||
| ## Generate MSM benchmark report in swift on laptop | ||
|
|
||
| * Enable `gpu-benchmarks` feature as default in `Cargo.toml` |
There was a problem hiding this comment.
To clarify, after adding feature flag around test_generate_bindings.rs this is still needed? Can't just pass flag as argument?
There was a problem hiding this comment.
No, we cannot. The feature flag for now doesn't work though, we can only enable the feature through the deafult flag in Cargo.toml
mopro-ffi/README.md
Outdated
| * Enable `gpu-benchmarks` feature as default in `Cargo.toml` | ||
| * Uncomment the 3rd line in `mopro-ffi/tests/test_generated_bindings.rs` to enable report generation | ||
| * run `cargo test --test test_generated_bindings --features gpu-benchmarks --release` | ||
| * The report would be generated at `mopro-core/benchmarks/gpu_explorations/msm_bench_swift_laptop.csv` |
There was a problem hiding this comment.
I'd add link to hackmd report maybe, can also add image easier to see (unless too big)
There was a problem hiding this comment.
I just updated the README.md, please have a look, thanks
| @@ -389,10 +388,6 @@ mod tests { | |||
| "└─ Average msm time: {:.5} ms\n└─ Overall processing time: {:.5} ms", | |||
| benchmarks.avg_processing_time, benchmarks.total_processing_time | |||
| ); | |||
| println!( | |||
There was a problem hiding this comment.
Nit: on L229 num_msm should be _num_msm for lint not to complain
warning: unused variable: `num_msm`
--> src/lib.rs:229:26
|
229 | pub fn run_msm_benchmark(num_msm: Option<u32>) -> Result<BenchmarkResult, MoproError> {
| ^^^^^^^ help: if this is intentional, prefix it with an underscore: `_num_msm```
```
|
Rebase on master please and we can merge |
mopro-core/Cargo.toml
Outdated
| default = ["wasmer/dylib"] | ||
| dylib = [] # NOTE: can probably remove this if we use env config instead | ||
| gpu-benchmarks = ["jemalloc-ctl", "jemallocator"] | ||
| default = [] |
There was a problem hiding this comment.
You need to rebase on master, this is not the latest version
There was a problem hiding this comment.
sorry I'll reset and rebase again.
oskarth
changed the title
Related Issue
#21
Main modification
Issue met now
feature flag does not work in
swiftafter building with--features gpu-benchmarks.to reproduce (in
mopro-ffi):1. cargo build --release --features gpu-benchmarks 2. cargo test --test test_generated_bindings --features gpu-benchmakscreenshot:
to fix this, I had tried to modify
Makefileinmopro-ffito:which would encounter the below issue while running
cargo build --release --target aarch64-apple-ios-sim --features gpu-benchmarks.screenshot (put on the full trace just in case)