Eliminate pie deep copies in cairo program runner

[ormarx-starkware]

This change is 

[YairVaknin-starkware]

@YairVaknin-starkware reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ormarx-starkware and @Yael-Starkware).

---

a discussion (no related file):
Looks good, but no need for an atomic RC here, since this is a single threaded program. Use RC instead of ARC. Will be a bit cheaper.

---

crates/cairo-program-runner-lib/src/hints/types.rs line 387 at r1 (raw file):

impl PartialEq for TaskSpec {
    fn eq(&self, other: &Self) -> bool {
        *self.task == *other.task && self.program_hash_function == other.program_hash_function

This was need to make the tests pass, I assume?

Code quote:

impl PartialEq for TaskSpec {
    fn eq(&self, other: &Self) -> bool {
        *self.task == *other.task && self.program_hash_function == other.program_hash_function

[Yael-Starkware]

@Yael-Starkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ormarx-starkware and @YairVaknin-starkware).

---

a discussion (no related file):

Previously, YairVaknin-starkware wrote…

Looks good, but no need for an atomic RC here, since this is a single threaded program. Use RC instead of ARC. Will be a bit cheaper.

Not sure that you need to use RC here. will a simple reference work as well?

[Yael-Starkware]

@Yael-Starkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ormarx-starkware and @YairVaknin-starkware).

---

a discussion (no related file):

Previously, Yael-Starkware (YaelD) wrote…

Not sure that you need to use RC here. will a simple reference work as well?

after discussing this, checked again and it seems that multiple references(mutable and immutable) are taken from the exec_scopes at the same time. With Rc this behaviour is allowed by the rust compiler while with regular reference it is not allowed.
bottom line, you can't change it to a simple reference with current code structure.

[ormarx-starkware]

@ormarx-starkware made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @YairVaknin-starkware).

---

a discussion (no related file):

Previously, Yael-Starkware (YaelD) wrote…

after discussing this, checked again and it seems that multiple references(mutable and immutable) are taken from the exec_scopes at the same time. With Rc this behaviour is allowed by the rust compiler while with regular reference it is not allowed.
bottom line, you can't change it to a simple reference with current code structure.

changed to RC

---

crates/cairo-program-runner-lib/src/hints/types.rs line 387 at r1 (raw file):

Previously, YairVaknin-starkware wrote…

This was need to make the tests pass, I assume?

Yes. when wrapping Task in RC, PartialEq would compare pointer equality, not the actual data inside

[YairVaknin-starkware]

@YairVaknin-starkware reviewed 4 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ormarx-starkware).

[Yael-Starkware]

@Yael-Starkware reviewed 1 file and made 6 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ormarx-starkware and @YairVaknin-starkware).

---

crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 130 at r2 (raw file):

) -> Result<(), HintError> {
    // Get Rc<Task> here (cheap clone) since we need mutable access to exec_scopes for
    // fact_topologies.

remove

Code quote:

    // Get Rc<Task> here (cheap clone) since we need mutable access to exec_scopes for
    // fact_topologies.

---

crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 405 at r2 (raw file):

) -> Result<HintExtension, HintError> {
    // Get Rc<Task> (cheap clone) since we need mutable access to exec_scopes throughout this
    // function.

remove

Code quote:

    // Get Rc<Task> (cheap clone) since we need mutable access to exec_scopes throughout this
    // function.

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 71 at r2 (raw file):

    // Tasks are accessed directly via SIMPLE_BOOTLOADER_INPUT.tasks when needed.
    Ok(())
}

this seems risky.
@ormarx-starkware @YairVaknin-starkware so you understand why it's ok the remove the function logic?

Code quote:

///
/// Note: The TASKS variable is not actually read anywhere in the Rust implementation.
/// Tasks are accessed directly from SIMPLE_BOOTLOADER_INPUT when needed.
/// This function exists only to maintain compatibility with the Cairo hint interface.
pub fn set_tasks_variable(_exec_scopes: &mut ExecutionScopes) -> Result<(), HintError> {
    // Intentionally not cloning tasks - the TASKS variable is never read.
    // Tasks are accessed directly via SIMPLE_BOOTLOADER_INPUT.tasks when needed.
    Ok(())
}

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 144 at r2 (raw file):

        vars::TASK,
        simple_bootloader_input.tasks[task_id].task.clone(),
    );

I dont think that simple_bootloader_input.tasks[task_id].task is Rc , I think it just Task, so no cheap clone here.

Code quote:

    // Clone the Rc<Task> (cheap reference count increment) instead of cloning the entire Task.
    exec_scopes.insert_value(
        vars::TASK,
        simple_bootloader_input.tasks[task_id].task.clone(),
    );

---

crates/cairo-program-runner-lib/src/hints/load_cairo_pie.rs line 163 at r2 (raw file):

/// Makes it more convenient to access values in the Cairo PIE memory.
fn build_cairo_pie_memory_map(memory: &CairoPieMemory) -> HashMap<Relocatable, &MaybeRelocatable> {
    // Pre-allocate HashMap with known capacity to avoid repeated rehashing.

remove

Code quote:

    // Pre-allocate HashMap with known capacity to avoid repeated rehashing.

---

crates/cairo-program-runner-lib/src/hints/types.rs line 269 at r2 (raw file):

pub struct TaskSpec {
    /// Task wrapped in Rc to avoid expensive clones when storing in execution scopes.
    /// CairoPie tasks can be several GB in size.

Suggestion:

    /// Task wrapped in Rc to avoid expensive clones when reading it from execution scopes.
    /// CairoPie tasks can be several GB in size.

[Yael-Starkware]

@Yael-Starkware made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ormarx-starkware and @YairVaknin-starkware).

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 144 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I dont think that simple_bootloader_input.tasks[task_id].task is Rc , I think it just Task, so no cheap clone here.

my bad, please ignore this comment

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 140 at r2 (raw file):

            };
    }
    // Clone the Rc<Task> (cheap reference count increment) instead of cloning the entire Task.

remove

Code quote:

// Clone the Rc<Task> (cheap reference count increment) instead of cloning the entire Task.

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 144 at r2 (raw file):

        vars::TASK,
        simple_bootloader_input.tasks[task_id].task.clone(),
    );

why not clone task as before? it is noe Rc

Suggestion:

    // Clone the Rc<Task> (cheap reference count increment) instead of cloning the entire Task.
    exec_scopes.insert_value(
        vars::TASK,
        task.clone(),
    );

[Yael-Starkware]

@Yael-Starkware made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ormarx-starkware and @YairVaknin-starkware).

---

crates/cairo-program-runner-lib/src/hints/types.rs line 388 at r2 (raw file):

    fn eq(&self, other: &Self) -> bool {
        *self.task == *other.task && self.program_hash_function == other.program_hash_function
    }

are you sure this is required? I think Rust compares Rc by it's underlying value. please check.

this is very risky in case someone adds another field to the object.

Code quote:

impl PartialEq for TaskSpec {
    fn eq(&self, other: &Self) -> bool {
        *self.task == *other.task && self.program_hash_function == other.program_hash_function
    }

[YairVaknin-starkware]

@YairVaknin-starkware made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ormarx-starkware).

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 71 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

this seems risky.
@ormarx-starkware @YairVaknin-starkware so you understand why it's ok the remove the function logic?

Yes, it wasn't used, but we need to have a mapping to the %{ tasks = simple_bootloader_input.tasks %} hint. We should probably remove it from the bl and this function altogether.

[ormarx-starkware]

@ormarx-starkware made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Yael-Starkware).

---

crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 130 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

remove

Done.

---

crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 405 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

remove

Done.

---

crates/cairo-program-runner-lib/src/hints/load_cairo_pie.rs line 163 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

remove

Done.

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 140 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

remove

Done.

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 144 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

why not clone task as before? it is noe Rc

AFAIU this clones the Rc, and task.clone() would clone the entire Task

---

crates/cairo-program-runner-lib/src/hints/types.rs line 388 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

are you sure this is required? I think Rust compares Rc by it's underlying value. please check.

this is very risky in case someone adds another field to the object.

Removing this causes compile error: ```binary operation == cannot be applied to type `Vec````
We need it to compare the actual Task contents rather than the pointer

---

crates/cairo-program-runner-lib/src/hints/types.rs line 269 at r2 (raw file):

pub struct TaskSpec {
    /// Task wrapped in Rc to avoid expensive clones when storing in execution scopes.
    /// CairoPie tasks can be several GB in size.

Done.

[Yael-Starkware]

@Yael-Starkware reviewed 2 files, made 2 comments, and resolved 5 discussions.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @ormarx-starkware and @YairVaknin-starkware).

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 71 at r2 (raw file):

Previously, YairVaknin-starkware wrote…

Yes, it wasn't used, but we need to have a mapping to the %{ tasks = simple_bootloader_input.tasks %} hint. We should probably remove it from the bl and this function altogether.

I see multiple places where we read the task from exec_scopes:
https://github.com/starkware-libs/bootloader-hints/blob/275ccb96d766a9829cfb9afabd2b1d8c6d1e595f/crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs#L168
https://github.com/starkware-libs/bootloader-hints/blob/275ccb96d766a9829cfb9afabd2b1d8c6d1e595f/crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs#L307

where do we assign it but here?

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 144 at r2 (raw file):

Previously, ormarx-starkware wrote…

AFAIU this clones the Rc, and task.clone() would clone the entire Task

Task is &Rc , you can change load_task() function signature to reflect that.

[Yael-Starkware]

@Yael-Starkware made 1 comment.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @ormarx-starkware and @YairVaknin-starkware).

---

crates/cairo-program-runner-lib/src/hints/types.rs line 388 at r2 (raw file):

Previously, ormarx-starkware wrote…

Removing this causes compile error: ```binary operation == cannot be applied to type `Vec````
We need it to compare the actual Task contents rather than the pointer

did you try adding derive PartialEq on top of TaskSpec?

[YairVaknin-starkware]

@YairVaknin-starkware made 1 comment.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @ormarx-starkware and @Yael-Starkware).

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 71 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I see multiple places where we read the task from exec_scopes:
https://github.com/starkware-libs/bootloader-hints/blob/275ccb96d766a9829cfb9afabd2b1d8c6d1e595f/crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs#L168
https://github.com/starkware-libs/bootloader-hints/blob/275ccb96d766a9829cfb9afabd2b1d8c6d1e595f/crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs#L307

where do we assign it but here?

This is for the current (single) task, pulled from the TASK (inserted into exec scopes when we start handling the current task). The removed code inserted all tasks into the TASKS variable, which isn't used anymore and accessed through the input directly when needed as documented by Or.

[Yael-Starkware]

@Yael-Starkware made 1 comment.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @ormarx-starkware and @YairVaknin-starkware).

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 71 at r2 (raw file):

Previously, YairVaknin-starkware wrote…

This is for the current (single) task, pulled from the TASK (inserted into exec scopes when we start handling the current task). The removed code inserted all tasks into the TASKS variable, which isn't used anymore and accessed through the input directly when needed as documented by Or.

got it - TASK vs TASKS, ok, so please also remove the hint from the bootloader code.

[Yael-Starkware]

@Yael-Starkware reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ormarx-starkware).

[ormarx-starkware]

@ormarx-starkware made 2 comments.
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @Yael-Starkware).

---

crates/cairo-program-runner-lib/src/hints/simple_bootloader_hints.rs line 144 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Task is &Rc , you can change load_task() function signature to reflect that.

Done.

---

crates/cairo-program-runner-lib/src/hints/types.rs line 388 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

did you try adding derive PartialEq on top of TaskSpec?

Done.

[Yael-Starkware]

@Yael-Starkware reviewed 2 files and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ormarx-starkware).