Added option to print used resources for tests.

[orizi]

Stack:

• Removing elements from the list earlier. #5292
• Updated UnorderedHashMap::merge. #5291
• Added option to print used resources for tests. #5266 ⬅

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

---

This change is 

[yuvalsw]

Reviewed 9 of 23 files at r2.
Reviewable status: 9 of 28 files reviewed, 2 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 251 at r2 (raw file):

        Infos: Iterator<Item = &'b Self::Info> + Clone,
    {
        let arm_demands = zip_eq(match_info.arms(), infos.clone())

can't you use .iter() here and below to avoid this clone?

Code quote:

clone()

---

crates/cairo-lang-utils/src/unordered_hash_map.rs line 296 at r2 (raw file):

            match self.0.entry(key.clone()) {
                Entry::Occupied(mut e) => {
                    merge(e.get_mut(), value);

wouldn't it make more sense to let the function only output the value to use and not also insert it?
Also - I think it makes sense for the merge function to get the key as input for the decision.

[orizi]

Reviewable status: 0 of 28 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)

---

crates/cairo-lang-lowering/src/optimizations/match_optimizer.rs line 251 at r2 (raw file):

Previously, yuvalsw wrote…

can't you use .iter() here and below to avoid this clone?

it is a clone of the iterator - not of the iterators content.

---

crates/cairo-lang-utils/src/unordered_hash_map.rs line 296 at r2 (raw file):

Previously, yuvalsw wrote…

wouldn't it make more sense to let the function only output the value to use and not also insert it?
Also - I think it makes sense for the merge function to get the key as input for the decision.

1. it is an edit for the merge state - if we extract it, and add again it will be less efficient for no good reason.
2. Probably makes sense - but for now not really needed, fn is really easily adapted to support.

[yuvalsw]

Reviewed 5 of 5 files at r1, 10 of 10 files at r3, 13 of 13 files at r4, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-runner/src/lib.rs line 106 at r4 (raw file):

/// The used resources in a run.
#[derive(Debug, Eq, PartialEq, Clone, Default)]
pub struct UsedResources {

IIUC the diff between UsedResources and ExecutionResource is the syscalls, that is, starknet. So I would call one this one StarknetExecutionResources, and specify starknet in the doc too.

---

crates/cairo-lang-runner/src/lib.rs line 114 at r4 (raw file):

impl UsedResources {
    /// Adds the resources of `other` to `self`.
    pub fn add(&mut self, other: Self) {

Can you impl Add/AddAssign?

---

crates/cairo-lang-runner/src/lib.rs line 251 at r4 (raw file):

            syscalls_used_resources: Default::default(),
        };
        let RunResult { gas_counter, memory, value, used_resources, profiling_info } = self

it's confusing that it's called used_resources when its type isn't UsedResources but ExecutionResources. (unless you rename to UsedResources and StarknetUsedResources/SyscallsUsedResources)

Code quote:

used_resources

---

crates/cairo-lang-runner/src/casm_run/mod.rs line 102 at r4 (raw file):

    /// The starknet state.
    pub starknet_state: StarknetState,
    /// Maintains the resources of the run.

the VM resources?
Worth making the difference clearer.

Code quote:

ins the resources of the run.

---

crates/cairo-lang-runner/src/casm_run/mod.rs line 2092 at r4 (raw file):

}

type RunFunctionRes = (Vec<Option<Felt252>>, usize, ExecutionResources);

This may be a good time to make it named.

---

crates/cairo-lang-runner/src/casm_run/mod.rs line 2155 at r4 (raw file):

    let used_resources = runner
        .get_execution_resources(vm)
        .expect("Failed to get execution resources, but the run was successful.");

so we don't want to panic, right?

---

crates/cairo-lang-runner/src/casm_run/test.rs line 114 at r4 (raw file):

fn test_runner(function: CasmContext, n_returns: usize, expected: &[i128]) {
    let (hints_dict, string_to_hint) = build_hints_dict(function.instructions.iter());
    let mut hint_processor = CairoHintProcessor {

Maybe add a new function from a string_to_hint?

---

crates/cairo-lang-test-runner/src/lib.rs line 443 at r4 (raw file):

    };
    let summary = wrapped_summary.as_mut().unwrap();
    let (res_type, status_str, gas_usage, used_resources, profiling_info) = if let Some(x) = status

please rename

Code quote:

x

---

crates/cairo-lang-test-runner/src/lib.rs line 468 at r4 (raw file):

    }
    if let Some(used_resources) = used_resources {
        let filtered = used_resources.execution_resources.filter_unused_builtins();

I can't find this method.

Code quote:

filter_unused_builtins

---

crates/cairo-lang-test-runner/src/lib.rs line 469 at r4 (raw file):

    if let Some(used_resources) = used_resources {
        let filtered = used_resources.execution_resources.filter_unused_builtins();
        println!("    steps: {}", filtered.n_steps);

can you add an example of the output?

---

crates/cairo-lang-test-runner/src/lib.rs line 470 at r4 (raw file):

        let filtered = used_resources.execution_resources.filter_unused_builtins();
        println!("    steps: {}", filtered.n_steps);
        println!("    memory holes: {}", filtered.n_memory_holes);

I can't find these members of the result struct. too.

Code quote:

n_memory_holes

---

crates/cairo-lang-test-runner/src/lib.rs line 475 at r4 (raw file):

                println!(
                    "    {name}: ({})",
                    m.into_iter().sorted().map(|(k, v)| format!("{k:?}: {v}")).join(", ")

Is this :? needed?

Code quote:

:?

[yuvalsw]

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-utils/src/unordered_hash_map.rs line 296 at r2 (raw file):

Previously, orizi wrote…

1. it is an edit for the merge state - if we extract it, and add again it will be less efficient for no good reason.
2. Probably makes sense - but for now not really needed, fn is really easily adapted to support.

1. I meant that the merge function here chooses the value, and the editing is done here in the case of Occupied.

---

crates/cairo-lang-utils/src/unordered_hash_map.rs line 286 at r4 (raw file):

    /// Merges the map with another map. If a key is present in both maps, the given merge function
    /// is used to combine the values.
    pub fn merge<M>(&mut self, other: &Self, merge: M)

Suggest to rename BTW, it has the same name as the function itself.

Code quote:

merge: M

[orizi]

Reviewable status: 22 of 28 files reviewed, 13 unresolved discussions (waiting on @yuvalsw)

---

crates/cairo-lang-runner/src/lib.rs line 106 at r4 (raw file):

Previously, yuvalsw wrote…

IIUC the diff between UsedResources and ExecutionResource is the syscalls, that is, starknet. So I would call one this one StarknetExecutionResources, and specify starknet in the doc too.

Done.

---

crates/cairo-lang-runner/src/lib.rs line 114 at r4 (raw file):

Previously, yuvalsw wrote…

Can you impl Add/AddAssign?

Done - but just add_assign as only it was needed.

---

crates/cairo-lang-runner/src/lib.rs line 251 at r4 (raw file):

Previously, yuvalsw wrote…

it's confusing that it's called used_resources when its type isn't UsedResources but ExecutionResources. (unless you rename to UsedResources and StarknetUsedResources/SyscallsUsedResources)

the type can hold general execution resources - but the meaning of it in this case is the actual used resources.

---

crates/cairo-lang-runner/src/casm_run/mod.rs line 102 at r4 (raw file):

Previously, yuvalsw wrote…

the VM resources?
Worth making the difference clearer.

I don't know what the first one means - it is unrelated to this PR.
it seems to count steps across multiple runners.

---

crates/cairo-lang-runner/src/casm_run/mod.rs line 2155 at r4 (raw file):

Previously, yuvalsw wrote…

so we don't want to panic, right?

this cannot actually happen - as all sierra runs should complete.

---

crates/cairo-lang-runner/src/casm_run/test.rs line 114 at r4 (raw file):

Previously, yuvalsw wrote…

Maybe add a new function from a string_to_hint?

maybe - but unrelated to the PR.

---

crates/cairo-lang-test-runner/src/lib.rs line 443 at r4 (raw file):

Previously, yuvalsw wrote…

please rename

Done.

---

crates/cairo-lang-test-runner/src/lib.rs line 468 at r4 (raw file):

Previously, yuvalsw wrote…

I can't find this method.

this is in the cairo-vm repo.

---

crates/cairo-lang-test-runner/src/lib.rs line 469 at r4 (raw file):

Previously, yuvalsw wrote…

can you add an example of the output?

Done.

---

crates/cairo-lang-test-runner/src/lib.rs line 470 at r4 (raw file):

Previously, yuvalsw wrote…

I can't find these members of the result struct. too.

this is in the cairo-vm repo.

---

crates/cairo-lang-test-runner/src/lib.rs line 475 at r4 (raw file):

Previously, yuvalsw wrote…

Is this :? needed?

used longer form instead.
(basically we print the same way as foundry does)

---

crates/cairo-lang-utils/src/unordered_hash_map.rs line 296 at r2 (raw file):

Previously, yuvalsw wrote…

1. I meant that the merge function here chooses the value, and the editing is done here in the case of Occupied.

1. it is already inserted - so i need to edit it - or to do awful lifetime stuff.
   https://reviewable.io/reviews/starkware-libs/cairo/5291

---

crates/cairo-lang-utils/src/unordered_hash_map.rs line 286 at r4 (raw file):

Previously, yuvalsw wrote…

Suggest to rename BTW, it has the same name as the function itself.

https://reviewable.io/reviews/starkware-libs/cairo/5291

[yuvalsw]

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

---

crates/cairo-lang-test-runner/src/lib.rs line 474 at r5 (raw file):

        //     steps: 42
        //     memory holes: 20
        //     builtins: ("range_check_builtin": 3)

IIUC with another builtin it would be:
builtins: ("range_check_builtin": 3, "another_builtin": 1)

Are the parentheses and the quotes needed then?

Suggestion:

 builtins: range_check_builtin: 3

---

crates/cairo-lang-utils/src/unordered_hash_map.rs line 296 at r2 (raw file):

Previously, orizi wrote…

1. it is already inserted - so i need to edit it - or to do awful lifetime stuff.
   https://reviewable.io/reviews/starkware-libs/cairo/5291

Replied there.

[yuvalsw]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)

---

crates/cairo-lang-test-runner/src/lib.rs line 474 at r5 (raw file):

Previously, yuvalsw wrote…

IIUC with another builtin it would be:
builtins: ("range_check_builtin": 3, "another_builtin": 1)

Are the parentheses and the quotes needed then?

took the option people are happy with in foundry.
these encapsulate the cases where more than one is used quite comfortably.

[yuvalsw]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)

[yuvalsw]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @orizi)