Add program analysis tool

[tohrnii]

Partly addresses #138

Adds a utility package to analyse any given script. Takes a source script as a file input and prints the following information.

• Number of VM cycles it takes to execute the script
• Number of NOOPs executed as part of a program

Input:
proc.foo.1 pop.local.0 end begin popw.mem.1 push.17 exec.foo end

Output:

Total Number of VM Cycles: 24
Total Number of NOOPs: 1

[bobbinth]

@tohrnii - thank you for working on this! A few preliminary thoughts:

1. I'd like to make the structure a bit more generic. For example:

• Let's return something like ProgramInfo struct from the analyze() function. For now, this could contain just a single field and we can expand it later to contain more fields.
• I'd like the structure to be generic so that we could use it, for example, from WASM. This way, we'll be able to have a webpage where you could paste in a script and get the stats back.

2. I'd probably prefer tools rather than analysis crate. And the package name itself could be miden-vm-tools.
3. I'm not sure what's the right place to put the code is. On the one hand, it would be nice to have miden-vm-tools compile into a binary (so that we could run it from the command line). On the other hand, I'd like to be able to compile it as a WASM-compliant library. Any suggestions are welcome here.

[grjte]

3. I'm not sure what's the right place to put the code is. On the one hand, it would be nice to have `miden-vm-tools` compile into a binary (so that we could run it from the command line). On the other hand, I'd like to be able to compile it as a WASM-compliant library. Any suggestions are welcome here.

I think it should probably be a library which could then be used for front-end tooling or as a dependency for a binary

[bobbinth]

I think it should probably be a library which could then be used for front-end tooling or as a dependency for a binary

I guess the question is where to put the library portion of the code and where to put the binaries. For example, we could put the actual code into the miden crate and then create tools crate to contain binaries.

Another option is for tools crate be a library crate and come up with some other crate to contain binaries.

[tohrnii]

we could put the actual code into the miden crate and then create tools crate to contain binaries.

This seems like the best way to go to me.

[tohrnii]

Moved the library to miden::utils and binary to tools crate. Does this structure make sense?

[bobbinth]

Looks good! I left a few comments inline.

[tohrnii]

Thank you so much @bobbinth for the review. Made suggested changes. Please let me know if we should add rest of the analysis to the same pr or open another one.

[tohrnii]

On rebasing to latest next branch, the total number of vm cycles have changed. This seems to be related to PR #212 . For example for script use.std::sys begin exec.sys::finalize_stack end, the total number of vm cycles changed from 69 to 72.
Similarly, for script proc.foo.1 pop.local.0 end begin popw.mem.1 push.17 exec.foo end, the total number of vm cycles changed from 23 to 24. @bobbinth Does this look like an intended side effect of PR 212 or should this be investigated further?

[bobbinth]

@tohrnii - yes, this is mostly likely due to #212. The difference in count should come primarily form more NOOP operations executed. We need to execute NOOPs in a few cases, for example:

1. If a number of operation groups in a batch is not a power of two, we execute NOOPs for each missing group (e.g., if there are 3 groups in a batch, we put a single NOOP into the 4th group).
2. If a PUSH operation is the last one in a group, we need to execute a NOOP operation after it.

By the way, it may be a good metric to track: number of NOOPs executed as a part of a program. This could be a good efficiency metric.

[tohrnii]

Thanks for the explanation on PR 212. I'll add NOOPs metric to be added as part of the program analysis tool to issue #138 as a comment.

[bobbinth]

@tohrnii - let's add NOOP tracking in this PR (since I think it should be fairly simple), and then we can do the rest in subsequent PRs. What do you think?

[tohrnii]

@bobbinth - Sounds good!

[bobbinth]

Actually, I should say that counting NOOPs would be pretty difficult right now, but will become very easy once #195 is merges. So, let's wait until I'm done with decoder trace, and then we can finish this PR.

[tohrnii]

@bobbinth - I've added total noops executed as part of the analysis. Also added None operation as part of this PR. VmState at the first row contains the None operation based on your suggestion.

Two pending analysis left to be addressed are:

• Number of program blocks in the script (for each block type)
• Number of cycles per assembly instruction. E.g., a program executed u32add.full operation x times, and this translated to y VM cycles.

I will address these in a separate PR after completing #226.

[bobbinth]

Thank you! I left a few more comments inline.

[bobbinth]

Thank you! A few more comments inline.

[tohrnii]

@bobbinth - Thank you for the review. Made suggested changes.

[bobbinth]

Do you need the unwrap() here? Would something like this work: matches!(vm_state.op, Some(Operation::Noop))?

[bobbinth]

Oh - I think it won't. I'm trying to think of a way to get rid of the unwrap's here - ideally, we shouldn't have them in production code.

[bobbinth]

I guess one thing that this doesn't account for is when a program terminates with an error. We should probably change the return type of analyze() to return a Result<ProgramInfo, ExecutionError> and then if we encounter an error it would be just returned instead of ProgramInfo. What do you think?

[tohrnii]

I'm trying to think of a way to get rid of the unwrap's here - ideally, we shouldn't have them in production code.

Just to clarify, the reason is proper error propagation right?
Can we use something like:

for state in vm_state_iterator {
    let vm_state = match state {
        Ok(vm_state) => vm_state,
        Err(err) => return Err(err)
    };
    if matches!(vm_state.op, Some(Operation::Noop)) {
        noop_count += 1;
    }
    total_vm_cycles = vm_state.clk;
}

[tohrnii]

I guess one thing that this doesn't account for is when a program terminates with an error. We should probably change the return type of analyze() to return a Result<ProgramInfo, ExecutionError> and then if we encounter an error it would be just returned instead of ProgramInfo. What do you think?

Makes sense to propagate error here. Since there are two possible errors here, AssemblyError while compiling the script and ExecutionError while executing the script, should we add a new enum called AnalysisError with those two variants and return that instead? That doesn't look very clean though. Is there a better (more idiomatic) way to handle that? Thanks.

[bobbinth]

Yes - the reason is proper error propagation, but also, as a general rule, I try not to have unwrap's in non-test code.

Yes, let's add a new enum. We did something like this here. Though I don't love the name AnalysisError. One other option that came to mind is MidenError - but I don't love this one too much either.

The code snippet above looks right - though, I'd prefer to use map_err() instead of match. For example, something like:

for state in vm_state_iterator {
    let vm_state = state.map_err(...)?;
    if matches!(vm_state.op, Some(Operation::Noop)) {
        noop_count += 1;
    }
    total_vm_cycles = vm_state.clk;
}

[bobbinth]

Maybe ProgramError is better than the other options?

[tohrnii]

@bobbinth - ProgramError sounds good to me. For future, in general should we lean more towards verbosity or succinctness in naming things?

[tohrnii]

@bobbinth - Thank you for the review. Made suggested changes.
ProgramError enum variants take error as argument instead of string. The thinking is that this would allow access to properties of the error in case this enum was to be used outside of program analysis and in general to avoid loss of information. Please let me know if this should take a string instead.

[bobbinth]

All looks good! Thank you!