micro benchmarks #32

zhenfeizhang · 2022-03-01T16:02:03Z

Description

closes: #XXXX

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (main)
Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.

This PR adds micro benchmarks for subroutines within proving and verification. It is enabled with --features=bench

~~Wrote unit tests~~
Updated relevant documentation in the code
Added a relevant changelog entry to the Pending section in CHANGELOG.md
Re-reviewed Files changed in the GitHub PR explorer

alxiong

Question: instead of declaring one bencher function for each (msm, fft, poly_eval), have you tried ark_std::start_timer and ark_std::end_timer (another usage example)?

they use concrete struct instance to avoid the necessity of locking? (need to double check)
Do your bencher profile more accurately under multi-threaded run?

zhenfeizhang · 2022-03-04T13:05:36Z

Ha, good idea! I didn't know about their timer... I will switch to theirs...

zhenfeizhang · 2022-03-09T17:17:32Z

Did some digging in the ark_std's timer. Those are macros that get you the running time within a function. We need to benchmark FFTs and MSMs across multiple functions so we will still need to declare global variables to store the result. Given this I am inclined to keep the current design.

they use concrete struct instance to avoid the necessity of locking? (need to double check)

I think they don't need to lock because it is within a single function.

alxiong · 2022-03-15T04:03:17Z

scripts/run_mt_bench.sh

+rm target/*.txt
+rm target/*.log
+RAYON_NUM_THREADS=64 cargo bench --features=bench > target/64core.log 


mt in the file name is ambiguous (my first reaction: "merkle tree? where?" 😆 then I realize it means to say "multithreading")? plus it's a bit confusing with the co-existence of ./scripts/run_benchmark.sh

location of *.txt/log files probably should be target/plonkbench/*.txt/log (Criterion's artifacts are all under target/criterion/*)

cargo bench plonk-benches instead since there's merkle_path bench in primitives/

alxiong · 2022-03-15T07:27:35Z

plonk/benches/bench.rs

+    let mut f = File::create(format!(
+        "../target/{}-threads.txt",
+        rayon::current_num_threads()
+    ))
+    .expect("Unable to create file");


Suggested change

let mut f = File::create(format!(

"../target/{}-threads.txt",

rayon::current_num_threads()

))

.expect("Unable to create file");

let mut path = PathBuf::new();

path.push(env!("CARGO_MANIFEST_DIR"));

path.push(format!("target/plonkbench/{}-threads", rayon::current_num_threads()));

path.set_extension("txt");

let mut f = OpenOptions::new()

.write(true)

.create(true)

.truncate(true)

.open(path.clone())

.unwrap();

You can try using something like this to overwrite the file instead of appending to it in case the file already exists (which File::create does)

alxiong · 2022-03-15T07:48:43Z

plonk/src/bencher.rs

+thread_local!(static FFT_START_TIME: RefCell<Instant> = RefCell::new(Instant::now()));
+thread_local!(static FFT_TIMER_LOCK: RefCell<bool> = RefCell::new(false));
+thread_local!(static FFT_TOTAL_TIME: RefCell<Duration> = RefCell::new(Duration::ZERO));


I'm not sure if there's better method here, basically we are dealing with a global time counter that accumulates time spent across code snippets in many functions.

I wonder if we can get rid of the time lock, and start time -- they can be local variable, right? only the total time needs to be globally accessible.

alxiong · 2022-03-15T07:52:59Z

plonk/src/bencher.rs

+        if FFT_TIMER_LOCK.with(|lock| *lock.borrow()) {
+            panic!("another FFT timer has already started somewhere else");
+        }
+
+        FFT_START_TIME.with(|timer| {
+            *timer.borrow_mut() = Instant::now();
+        });
+
+        FFT_TIMER_LOCK.with(|lock| {
+            *lock.borrow_mut() = true;
+        })


this logic is slightly baffling to me, so if FFT is locked, then we panic?

it seem to me that our timer will never be used by 2 threads at the same time? because the multi-threaded code is in-between the fft_start() and fft_end() (e.g. inside fn compute_selector_polynomials), so maybe we don't need this locking to begin with?

If we have proper concurrency control, then FFT timer should never panic, but rather wait on lock to be released instead?

CLAassistant · 2022-06-29T23:11:05Z

All committers have signed the CLA.

micro benchmarks

3e00820

zhenfeizhang self-assigned this Mar 1, 2022

zhenfeizhang added 4 commits March 1, 2022 12:21

add quotient poly eval to fft bench

b68b1f3

refine msm bench

7921af5

optimize tests

a727877

optimize bench and add logs

a004078

zhenfeizhang requested review from alxiong and chancharles92 March 3, 2022 20:53

clean up

8f9f115

alxiong reviewed Mar 4, 2022

View reviewed changes

alxiong reviewed Mar 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

micro benchmarks #32

micro benchmarks #32

-    let mut f = File::create(format!(
-        "../target/{}-threads.txt",
-        rayon::current_num_threads()
-    ))
-    .expect("Unable to create file");
+        let mut path = PathBuf::new();
+        path.push(env!("CARGO_MANIFEST_DIR"));
+        path.push(format!("target/plonkbench/{}-threads", rayon::current_num_threads()));
+        path.set_extension("txt");
+        let mut f = OpenOptions::new()
+            .write(true)
+            .create(true)
+            .truncate(true)
+            .open(path.clone())
+            .unwrap();

micro benchmarks #32

Are you sure you want to change the base?

micro benchmarks #32

Conversation

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment