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

Improve cycles output in analyze tool #1099

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Oct 9, 2023

This PR adds extended information about traces lengths of VM components to the analyze tool output.

@Fumuran Fumuran requested a review from bobbinth October 9, 2023 15:55
miden/src/tools/mod.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-improve-analyze branch 2 times, most recently from 72a0a00 to 4afbbe8 Compare October 10, 2023 13:10
@Fumuran Fumuran requested a review from bobbinth October 10, 2023 13:21
processor/src/lib.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran requested a review from bobbinth October 10, 2023 19:12
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a couple of minor comments inline. After these are addressed, we can merge.

Also, could you paste a screenshot of how the output for this looks like?

processor/src/debug.rs Outdated Show resolved Hide resolved
processor/src/trace/utils.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-improve-analyze branch from 4e953d3 to c6d314c Compare October 10, 2023 19:52
@Fumuran
Copy link
Contributor Author

Fumuran commented Oct 10, 2023

Now analyze output for the merkle_store example looks like this:

Screenshot 2023-10-10 at 9 52 53 PM

and similar output for the fibonacci example:

Screenshot 2023-10-10 at 9 55 02 PM

I added variable width for the first column, so operations names will always fit (for example, before that push.*.*.* operations from merkle_store example were breaking the table). I also added the line to highlight the table header.

@Fumuran Fumuran force-pushed the andrew-improve-analyze branch from c6d314c to 26eed21 Compare October 10, 2023 20:41
@bobbinth
Copy link
Contributor

Looks great! A couple of minor comments on the output formatting:

  1. Let's move "Total number of NOOPs executed" after the table.
    a. Also, let's not capitalize "number".
  2. For the table, let's rename "AsmOp" title into "Assembly instruction"
  3. Above the "VM cycles" section, let's add a header "Analyzed [program name] with [input name]".

Basically, the output could look like:

============================================================
Analyzed fib.masm program with fib.inputs

VM cycles: ...

Assembly Instruction ..
--------------------------

Total number of NOOPs executed: 

@Fumuran
Copy link
Contributor Author

Fumuran commented Oct 10, 2023

After last refactoring output looks like this:

Screenshot 2023-10-11 at 1 00 56 AM Screenshot 2023-10-11 at 1 01 30 AM

@bobbinth
Copy link
Contributor

Looks good! Thank you! One last thing: could we add:

============================================================

Before the Analyzed... line?

@Fumuran Fumuran force-pushed the andrew-improve-analyze branch from 81667b4 to bf4dbdf Compare October 10, 2023 23:12
@Fumuran
Copy link
Contributor Author

Fumuran commented Oct 10, 2023

The final view:

Screenshot 2023-10-11 at 1 23 26 AM

@bobbinth
Copy link
Contributor

All looks good! Thank you!

@bobbinth bobbinth merged commit 85b6c80 into next Oct 10, 2023
15 checks passed
@bobbinth bobbinth deleted the andrew-improve-analyze branch October 10, 2023 23:55
@bobbinth bobbinth mentioned this pull request Oct 10, 2023
11 tasks
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