Add ExternalCodeSizeEstimator.

[ilyalesokhin-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[orizi]

Reviewed 2 of 7 files at r1, all commit messages.
Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions

---

crates/cairo-lang-lowering/src/inline/mod.rs line 95 at r1 (raw file):

    let weight_of_blocks = db.estimate_size(function_id)?;
    eprintln!("estimate_size: {}: {weight_of_blocks}", function_id.full_path(db));

remove print.

---

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

    fn get_db(&self) -> &dyn LoweringGroup;
}

Suggestion:

/// A trait for estimation of the code size of a function.
pub trait ExternalCodeSizeEstimator {
    /// Returns the extimated size of the function.
    fn estimate_size(&self, function_id: ConcreteFunctionWithBodyId) -> Maybe<isize>;
}

/// Marker trait for using the approximated casm inline weight method for size estimation.
pub trait UseApproxCodeSizeEstimator: Upcast<dyn LoweringGroup>;
impl<T: UseApproxCodeSizeEstimator + Sized?> ExternalCodeSizeEstimator for T {
    fn estimate_size(&self, function_id: ConcreteFunctionWithBodyId) -> Maybe<isize> {
        let db = self.upcast();
        let lowered = db.inlined_function_with_body_lowered(function_id)?;
        Ok(ApproxCasmInlineWeight::new(db, &lowered).lowered_weight(&lowered))
    }
}

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

    fn get_db(&self) -> &dyn LoweringGroup;
}

error[E0391]: cycle detected when computing the super predicates of db::LoweringGroup
--> crates/cairo-lang-lowering/src/db.rs:56:49
|
56 | SemanticGroup + Upcast + ExternalCodeSizeEstimator
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: ...which requires computing the super predicates of db::ExternalCodeSizeEstimator...
--> crates/cairo-lang-lowering/src/db.rs:44:1
|
44 | pub trait ExternalCodeSizeEstimator: Upcast {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: ...which again requires computing the super predicates of db::LoweringGroup, completing the cycle

[orizi]

Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

error[E0391]: cycle detected when computing the super predicates of db::LoweringGroup
--> crates/cairo-lang-lowering/src/db.rs:56:49
|
56 | SemanticGroup + Upcast + ExternalCodeSizeEstimator
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: ...which requires computing the super predicates of db::ExternalCodeSizeEstimator...
--> crates/cairo-lang-lowering/src/db.rs:44:1
|
44 | pub trait ExternalCodeSizeEstimator: Upcast {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: ...which again requires computing the super predicates of db::LoweringGroup, completing the cycle

What if you move the Upcast requirement from the trait, just for the impl (in addition to the marker)

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

Previously, orizi wrote…

What if you move the Upcast requirement from the trait, just for the impl (in addition to the marker)

I don't follow, where would I implement the default estimate_size?

[ilyalesokhin-starkware]

• Enable function specialization. #7720
• Add SpecializedFunction. #7718
• Add ExternalCodeSizeEstimator. #7698 👈 (View in Graphite)
• Use different cycle prevention in inlining. #7742
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/inline/mod.rs line 75 at r2 (raw file):

            /// The default threshold for inlining small functions. Decided according to sample
            /// contracts profiling.
            const DEFAULT_INLINE_SMALL_FUNCTIONS_THRESHOLD: usize = 55;

minimum size for the stacking contract is at 25, but it makes the gas costs in test_validate_gas_cost much higher:

Code quote (i):

 55;

Code snippet (ii):

   cairo_level_tests::abi_dispatchers_tests::test_validate_gas_cost - Panicked with "Unexpected gas_usage:
     call_building: `4950`.
     serialization: `40270`.
     entry_point: `173930`.".

[ilyalesokhin-starkware]

Reviewable status: 2 of 70 files reviewed, 3 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-lowering/src/inline/mod.rs line 95 at r1 (raw file):

Previously, orizi wrote…

remove print.

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 2 of 70 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I don't follow, where would I implement the default estimate_size?

in an impl that requests both ExternalCodeSizeEstimator and Upcast<dyn Lowering>.
ExternalCodeSizeEstimator won't require it.

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

Previously, orizi wrote…

in an impl that requests both ExternalCodeSizeEstimator and Upcast<dyn Lowering>.
ExternalCodeSizeEstimator won't require it.

but then I would have to write the default implementation in each impl.

[orizi]

Reviewed 1 of 1 files at r3.
Reviewable status: 3 of 70 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

but then I would have to write the default implementation in each impl.

you just need a single implementation - i don't really understand.

---

crates/cairo-lang-compiler/src/db.rs line 59 at r3 (raw file):

        let casm = builder.casm_program();
        let total_size = casm.instructions.iter().map(|inst| inst.body.op_size()).sum::<usize>();
        Ok((total_size - (n_dummy_functions * 3)).try_into().unwrap_or(0))

can you doc the 3 * explanation? i don't understand this.

Code quote:

k((total_size - (n_dummy_functions * 3)).try_into().unwrap_or(0))

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

Previously, orizi wrote…

you just need a single implementation - i don't really understand.

There are 2 places where I use the default implementation

1. LoweringDatabaseForTesting
2. SierraGenDatabaseForTesting

[orizi]

Reviewed 1 of 7 files at r1, 6 of 66 files at r2, 6 of 109 files at r4, all commit messages.
Reviewable status: 14 of 133 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-lowering/src/db.rs line 53 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

There are 2 places where I use the default implementation

1. LoweringDatabaseForTesting
2. SierraGenDatabaseForTesting

f2f.

---

crates/cairo-lang-runner/src/profiling_test_data/major_test_cases line 394 at r5 (raw file):

  test::validate_call -> test::test_contract::__wrapper____validate__ -> core::array::deserialize_array_helper::<core::starknet::account::Call, core::starknet::account::CallSerde, core::starknet::account::CallDrop> -> core::array::deserialize_array_helper::<core::starknet::account::Call, core::starknet::account::CallSerde, core::starknet::account::CallDrop> -> core::array::deserialize_array_helper::<core::starknet::account::Call, core::starknet::account::CallSerde, core::starknet::account::CallDrop>: 14
Weight by Cairo stack trace:
  test::validate_call: 305

probably needs some more fine tuning of the default.

Code quote:

  test::validate_call: 305

[orizi]

Reviewed 69 of 70 files at r6, all commit messages.
Reviewable status: 80 of 133 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-lowering/src/db.rs line 51 at r6 (raw file):

}

// Marker trait for using an approximate code size estimator.

Suggestion:

/// Marker trait for using an approximate code size estimator.

---

crates/cairo-lang-lowering/src/db.rs line 55 at r6 (raw file):

impl<T: UseApproxCodeSizeEstimator> ExternalCodeSizeEstimator for T {
    /// Returns the virtual file matching the external id.

[orizi]

Reviewed 1 of 66 files at r2, 1 of 70 files at r6.
Reviewable status: 82 of 133 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-lowering/src/inline/mod.rs line 75 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

minimum size for the stacking contract is at 25, but it makes the gas costs in test_validate_gas_cost much higher:

should probably still make it much larger - look at the major_test_cases file.

[ilyalesokhin-starkware]

Reviewable status: 82 of 133 files reviewed, 4 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-lowering/src/db.rs line 51 at r6 (raw file):

}

// Marker trait for using an approximate code size estimator.

Done.

---

crates/cairo-lang-lowering/src/db.rs line 55 at r6 (raw file):

impl<T: UseApproxCodeSizeEstimator> ExternalCodeSizeEstimator for T {
    /// Returns the virtual file matching the external id.

Done.

[ilyalesokhin-starkware]

crates/cairo-lang-runner/src/profiling_test_data/major_test_cases line 394 at r5 (raw file):

Previously, orizi wrote…

probably needs some more fine tuning of the default.

I had to increase the threshold to 120.
to get
test::validate_call: 268
here
and at the point the improvement in the statking contract is only

55616 -> 55109.

At 130.
I get back to 244, but the size of the staking contract increases rather than decreases.

[orizi]

Reviewable status: 82 of 133 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-lowering/src/db.rs line 51 at r6 (raw file):

Previously, ilyalesokhin-starkware wrote…

Done.

missing push probably

[orizi]

Reviewed 20 of 55 files at r7, all commit messages.
Reviewable status: 95 of 133 files reviewed, all discussions resolved

---

crates/cairo-lang-runner/src/profiling_test_data/major_test_cases line 394 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

I had to increase the threshold to 120.
to get
test::validate_call: 268
here
and at the point the improvement in the statking contract is only

55616 -> 55109.

At 130.
I get back to 244, but the size of the staking contract increases rather than decreases.

probably after the consts thing smaller programs will improve.
we'd play more with the exact threshold later.

[orizi]

Reviewed 3 of 109 files at r4, 35 of 55 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[orizi]

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

[orizi]

Reviewed 49 of 49 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[orizi]

Reviewed 59 of 59 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[orizi]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)