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

[PoC] feat: moving to shared store references #680

Merged
merged 13 commits into from
Sep 26, 2023
Merged

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Sep 18, 2023

#629 needs some modularity in order to be carried out safely, but both implementations of Multiframe (in Lurk and in the LEM module) embark an &'a Store reference. https://github.com/huitseeker/lurk-rs/tree/lem-integration-review points the direction to suh modularity, but shows that we like to interleave evaluation (needs a &mut Store) and proving (needs a &'a Store), which makes defining any function doing both (requires &'a mut Store), i.e. lifetimed mutable store references unwieldy.

In a nutshell:

  • if you do evaluation (requires &mut Store) and proving (requires &'a Store) simultaneously (requires &'a mut Store),
  • you can do it only once OR be an owner of the Store.

To circumvent this limitation, we implement a sync variant of elsa's IndexMap, which only requires shared references for an append-only structure.

Edit: benches show 20% 15% slowdown on evaluation (predictably: everything in the store is boxed), hydration is of course worse, no impact on proof/verif/store benches
https://gist.github.com/huitseeker/3c7205dd6c9e1025f7dace4a6c6e5b73
https://gist.github.com/huitseeker/250e5fde3d72efdd9337e3606b1ca148

@huitseeker huitseeker requested review from a team as code owners September 18, 2023 14:03
@huitseeker huitseeker requested review from porcuquine and a team as code owners September 18, 2023 17:00
@huitseeker huitseeker force-pushed the immut_store branch 2 times, most recently from 62e0390 to aafda56 Compare September 18, 2023 17:39
@huitseeker
Copy link
Contributor Author

!benchmark

@huitseeker
Copy link
Contributor Author

!benchmark

@github-actions
Copy link
Contributor

Benchmark for e9bfd6b

Click to view benchmark
Test Base PR %
end2end_benchmark/end2end_go_base_nova/_10_0 1808.2±57.59ms 1807.4±51.95ms -0.04%
eval_benchmark/eval_go_base_bls12/_10_16 564.4±43.50µs 669.2±26.99µs +18.57%
eval_benchmark/eval_go_base_bls12/_10_160 5.4±0.34ms 6.5±0.27ms +20.37%
eval_benchmark/eval_go_base_pallas/_10_16 636.1±31.03µs 790.3±82.35µs +24.24%
eval_benchmark/eval_go_base_pallas/_10_160 6.2±0.58ms 6.7±0.57ms +8.06%
hydration_benchmark/hydration_go_base_bls12/_10_16 34.6±5.49ns 322.3±16.62ns +831.50%
hydration_benchmark/hydration_go_base_bls12/_10_160 28.0±3.21ns 318.5±9.81ns +1037.50%
hydration_benchmark/hydration_go_base_pallas/_10_16 29.9±2.04ns 342.3±17.95ns +1044.82%
hydration_benchmark/hydration_go_base_pallas/_10_160 30.2±2.12ns 354.4±21.22ns +1073.51%
prove_benchmark/prove_go_base_nova/_10_0 1893.7±48.12ms 1844.1±73.64ms -2.62%
prove_compressed_benchmark/prove_compressed_go_base_nova/_10_0 7.6±0.26s 7.6±0.22s 0.00%
store_benchmark/store_go_base_bls12/_10_16 215.2±13.44µs 232.8±14.98µs +8.18%
store_benchmark/store_go_base_bls12/_10_160 216.2±8.65µs 253.6±18.36µs +17.30%
store_benchmark/store_go_base_pallas/_10_16 221.5±28.91µs 223.1±10.94µs +0.72%
store_benchmark/store_go_base_pallas/_10_160 214.3±8.68µs 229.1±13.75µs +6.91%
verify_benchmark/verify_go_base_nova/_10_0 241.8±17.51ms 235.0±20.07ms -2.81%
verify_compressed_benchmark/verify_compressed_go_base_nova/_10_0 228.5±11.91ms 236.8±22.57ms +3.63%

@huitseeker huitseeker force-pushed the immut_store branch 2 times, most recently from 499841d to bee158f Compare September 24, 2023 23:20
@huitseeker
Copy link
Contributor Author

benchmark!

@arthurpaulino
Copy link
Contributor

!benchmark

@arthurpaulino
Copy link
Contributor

Are we going to depend on that branch of our elsa fork?

@arthurpaulino
Copy link
Contributor

Wait, this branch lost my last commit

@winston-h-zhang
Copy link
Contributor

!benchmark

@github-actions
Copy link
Contributor

Benchmark for 55c17c5

Click to view benchmark
Test Base PR %
end2end_benchmark/end2end_go_base_nova/_10_0 2.2±0.09s 2.1±0.09s -4.55%
eval_benchmark/eval_go_base_bls12/_10_16 692.1±93.10µs 880.9±142.39µs +27.28%
eval_benchmark/eval_go_base_bls12/_10_160 6.8±0.47ms 7.5±0.39ms +10.29%
eval_benchmark/eval_go_base_pallas/_10_16 692.4±36.49µs 850.0±55.40µs +22.76%
eval_benchmark/eval_go_base_pallas/_10_160 8.4±0.87ms 9.2±0.99ms +9.52%
hydration_benchmark/hydration_go_base_bls12/_10_16 40.9±5.06ns 368.8±12.58ns +801.71%
hydration_benchmark/hydration_go_base_bls12/_10_160 36.1±5.93ns 394.7±15.72ns +993.35%
hydration_benchmark/hydration_go_base_pallas/_10_16 30.4±1.47ns 350.7±17.21ns +1053.62%
hydration_benchmark/hydration_go_base_pallas/_10_160 31.6±1.48ns 341.1±9.62ns +979.43%
prove_benchmark/prove_go_base_nova/_10_0 2.1±0.08s 2.1±0.09s 0.00%
prove_compressed_benchmark/prove_compressed_go_base_nova/_10_0 9.0±0.34s 8.7±0.30s -3.33%
store_benchmark/store_go_base_bls12/_10_16 248.5±13.03µs 253.5±24.16µs +2.01%
store_benchmark/store_go_base_bls12/_10_160 256.3±24.13µs 262.5±17.41µs +2.42%
store_benchmark/store_go_base_pallas/_10_16 269.2±32.84µs 283.3±37.97µs +5.24%
store_benchmark/store_go_base_pallas/_10_160 239.7±10.46µs 256.0±30.62µs +6.80%
verify_benchmark/verify_go_base_nova/_10_0 293.8±32.68ms 290.3±32.63ms -1.19%
verify_compressed_benchmark/verify_compressed_go_base_nova/_10_0 264.8±20.67ms 286.1±35.41ms +8.04%

- Removed the `src/cache_map.rs` file and `cache_map` module from the codebase.
- Replaced the use of `CacheMap` with `FrozenMap` from the `elsa` crate in the `PoseidonCache` and `Store` structures across multiple files.
- Added `elsa` dependency, version `1.9.0`, with `indexmap` feature to the `Cargo.toml` file.
- `CacheMap` imports throughout the codebase have been removed and replaced with imports of `elsa::sync::FrozenMap`.
- Removed mutability modifier from `store` variable
@huitseeker huitseeker disabled auto-merge September 26, 2023 19:44
@huitseeker
Copy link
Contributor Author

This line won't let us merge w/o override, I'll take it upon myself to bypass reqs:
https://github.com/lurk-lab/lurk-rs/blob/d65a71ced96b190c31dd1d19ad1f58fed3b9f4aa/.github/CODEOWNERS#L25

@huitseeker huitseeker merged commit a5101fd into master Sep 26, 2023
6 checks passed
@huitseeker huitseeker deleted the immut_store branch September 26, 2023 19:44
huitseeker added a commit to huitseeker/lurk-rs that referenced this pull request Sep 27, 2023
Mutability leftovers (see lurk-lab#680)

- Updated benchmarking scripts to no longer mutate the `store` variable during evaluation frames retrieval.
- Reformed `get_evaluation_frames` method across several files to use immutable references to `store` instead of mutable references.

Multiframe definition

- Introduced several new traits - `CEKState`, `FrameLike`, `EvaluationStore` and `MultiFrameTrait` to manage complex evaluation and circuit proof operations in the `proof/mod.rs` file.
- Detailed implementations of the above traits introduced in `circuit/circuit_frame.rs`, providing methods for handling evaluation frames, synthesizing data from multiple frames and more.
huitseeker added a commit to huitseeker/lurk-rs that referenced this pull request Sep 27, 2023
Mutability leftovers (see lurk-lab#680)

- Updated benchmarking scripts to no longer mutate the `store` variable during evaluation frames retrieval.
- Reformed `get_evaluation_frames` method across several files to use immutable references to `store` instead of mutable references.

Multiframe definition

- Introduced several new traits - `CEKState`, `FrameLike`, `EvaluationStore` and `MultiFrameTrait` to manage complex evaluation and circuit proof operations in the `proof/mod.rs` file.
- Detailed implementations of the above traits introduced in `circuit/circuit_frame.rs`, providing methods for handling evaluation frames, synthesizing data from multiple frames and more.
huitseeker added a commit that referenced this pull request Sep 28, 2023
* feat: Introduce MultiFrame trait

Mutability leftovers (see #680)

- Updated benchmarking scripts to no longer mutate the `store` variable during evaluation frames retrieval.
- Reformed `get_evaluation_frames` method across several files to use immutable references to `store` instead of mutable references.

Multiframe definition

- Introduced several new traits - `CEKState`, `FrameLike`, `EvaluationStore` and `MultiFrameTrait` to manage complex evaluation and circuit proof operations in the `proof/mod.rs` file.
- Detailed implementations of the above traits introduced in `circuit/circuit_frame.rs`, providing methods for handling evaluation frames, synthesizing data from multiple frames and more.

* refactor: make nova proofs use the generic trait

- uses the generic MultiFrame trait introduced in the prior commit to generically prove using nova,
- For references on the latest work defining this trait, see #629 (and its history, incl. #642, #707) and #677.

- make the benches use the generic trait, so LEM can bench similarly,
- make the examples use the generic trait, so LEM can example similarly,
- make Supernova use the generic trait, so LEM can NIVC similarly,

* use a type alias to simplify test calls

---------

Co-authored-by: Arthur Paulino <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants