Skip to content

feat(bench): generate histogram of executed instructions#214

Draft
mooori wants to merge 6 commits intonear:mooori/bench_mvpfrom
mooori:bench_inst
Draft

feat(bench): generate histogram of executed instructions#214
mooori wants to merge 6 commits intonear:mooori/bench_mvpfrom
mooori:bench_inst

Conversation

@mooori
Copy link

@mooori mooori commented Feb 9, 2024

This is still WIP. I'm pushing the code anyway since we should decide if such visualizations are helpful before iterating on the implementation.

The Makefile contains commands to:

  • gen: generate instrumented zkASM
  • run: run an instrumented zkASM program, which stores a trace to a file
  • bench: generates the histogram based on the trace stored in the file

Implements part of MVP outlined in #197.


fn print_name(&self) -> String {
let name = match self {
&Inst::Nop => "Nop",
Copy link

@nagisa nagisa Feb 13, 2024

Choose a reason for hiding this comment

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

This is kinda neat, but I think we should also consider reusing this for print_with_state to maintain consistent names across various places where we print these instructions. This shouldn't be too hard (although the diff will not be minimal) and can be achieved roughly through:

let mut output = String::with_capacity(32);
output.push_str(self.print_name());
match self {
    Inst::Udf { trap_code } => write!(&mut output, "##trap_code={trap_code}"),
    ...
}

This should also reveal that e.g. Inst::Select is actually considered multiple different instructions depending on a ty field. It might or might not be useful to split them out for histogram purposes as well.

Copy link
Author

Choose a reason for hiding this comment

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

Good point and thanks for the hints! The benchmarking infra MVP should produce tables instead of histogram graphs and a number of related changes have landed on main in the meantime. Opening a new PR will probably be simpler than refactoring this one, so I've created #217 for this.

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.

2 participants