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

micro benchmarks #32

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

micro benchmarks #32

wants to merge 6 commits into from

Conversation

zhenfeizhang
Copy link
Contributor

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

@zhenfeizhang zhenfeizhang self-assigned this Mar 1, 2022
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@zhenfeizhang
Copy link
Contributor Author

zhenfeizhang commented Mar 9, 2022

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.

Comment on lines +3 to +5
rm target/*.txt
rm target/*.log
RAYON_NUM_THREADS=64 cargo bench --features=bench > target/64core.log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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
  2. location of *.txt/log files probably should be target/plonkbench/*.txt/log (Criterion's artifacts are all under target/criterion/*)
  3. cargo bench plonk-benches instead since there's merkle_path bench in primitives/

Comment on lines +303 to +307
let mut f = File::create(format!(
"../target/{}-threads.txt",
rayon::current_num_threads()
))
.expect("Unable to create file");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines +6 to +8
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.

Comment on lines +81 to +91
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;
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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?
  2. If we have proper concurrency control, then FFT timer should never panic, but rather wait on lock to be released instead?

@CLAassistant
Copy link

CLAassistant commented Jun 29, 2022

CLA assistant check
All committers have signed the CLA.

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.

3 participants