feat(cairo_native): add batcher compiler struct

[avi-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/execution/native/batcher_compiler.rs line 15 at r1 (raw file):

struct BatcherCompiler {
    contract_cache_manager: GlobalContractCacheManager,

Move this file some place else, since non-native code is going to use it.
Maybe under state dir.

Suggestion:

struct ContractClassManager {
    contract_cache_manager: ContractClassCaches,

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/execution/native/batcher_compiler.rs line 31 at r1 (raw file):

        for (class_hash, sierra_contract_class) in self.compilation_request_receiver.iter() {
            if self.contract_cache_manager.get_native_contract_executor(&class_hash).is_some() {
                // Skip the compilation if the contract class is already compiled to native.

Suggestion:

// The contract class is already compiled to native - skip the compilation.

---

crates/blockifier/src/execution/native/batcher_compiler.rs line 42 at r1 (raw file):

                    self.contract_cache_manager
                        .set_native_contract_executor(class_hash, Some(contract_executor));
                }

Whay did we say about the sierra cache? we don't need to update it here?

Code quote:

                Ok(contract_executor) => {
                    self.contract_cache_manager
                        .set_native_contract_executor(class_hash, Some(contract_executor));
                }

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/execution/native/batcher_compiler.rs line 29 at r1 (raw file):

    }
    pub fn run(&self) {
        for (class_hash, sierra_contract_class) in self.compilation_request_receiver.iter() {

Cool

Code quote:

for (class_hash, sierra_contract_class) in self.compilation_request_receiver.iter() {

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/execution/native/batcher_compiler.rs line 44 at r1 (raw file):

                }
                Err(err) => {
                    eprintln!("Error compiling contract class: {}", err);

Why not log::error? what's the diff?

Code quote:

eprintln!(

[meship-starkware]

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: 4 of 5 files reviewed, 5 unresolved discussions (waiting on @avi-starkware and @noaov1)

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.19%. Comparing base (e3165c4) to head (3f7ad1e).
Report is 626 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2187       +/-   ##
===========================================
+ Coverage   40.10%   77.19%   +37.09%     
===========================================
  Files          26      387      +361     
  Lines        1895    40795    +38900     
  Branches     1895    40795    +38900     
===========================================
+ Hits          760    31492    +30732     
- Misses       1100     6975     +5875     
- Partials       35     2328     +2293     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

Reviewed 1 of 7 files at r2, all commit messages.
Reviewable status: 5 of 8 files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)

[avi-starkware]

Reviewable status: 5 of 8 files reviewed, 2 unresolved discussions (waiting on @meship-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/execution/native/contract_class_manager.rs line 33 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Not good enough. If the channel is empty, it will never reach to your check.
Please impl the drop as well

Done.

---

crates/blockifier/src/state/contract_class_manager.rs line 69 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Also, add a TODO to check for duplicated requests before filling the queue.
If native compile time can be 10 secs, we might blast this channel with the same request over and over again

Done.

[meship-starkware]

Reviewed 6 of 7 files at r2, 4 of 6 files at r3, 1 of 2 files at r4, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/state/global_cache.rs line 55 at r12 (raw file):

#[cfg(feature = "cairo_native")]
pub struct ContractClassCaches {

Should it be changed to Compiled class as well? If so, I'll just put it here to see if it changed after a rebase.

Code quote:

pub struct ContractClassCaches 

---

crates/blockifier/src/state/contract_class_manager.rs line 213 at r12 (raw file):

    fn drop(&mut self) {
        self.stop();
        let join_handle = self.join_handle.lock().unwrap().take().expect(

Why is it ok to unwrap here? I understand only one thread should access this handle, so there should be no problem, but I am not sure that is enough. Consider implementing a self.lock_join_handle() that will expect with an error message and use it here.

Code quote:

let join_handle = self.join_handle.lock().unwrap().take().expect(

[Yoni-Starkware]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/state/contract_class_manager.rs line 66 at r12 (raw file):

        // TODO(Avi, 15/12/2024): Add the size of the channel to the config.
        let (sender, receiver) = sync_channel(CHANNEL_SIZE);
        let compiler_config = SierraToCasmCompilationConfig::default();

SierraToCasm ???
Add a TODO to fix this, and to pass compilation config through the manager config

Code quote:

let compiler_config = SierraToCasmCompilationConfig::default();

---

crates/blockifier/src/state/contract_class_manager.rs line 91 at r12 (raw file):

            .expect("No other thread should access the join handle.");
        *mutex_guard = Some(join_handle);
        drop(mutex_guard);

Why?

Code quote:

        drop(mutex_guard);

---

crates/blockifier/src/state/contract_class_manager.rs line 118 at r12 (raw file):

                // When sending a compilation request, send the request without blocking the sender.
                // TODO(Avi, 15/12/2024): Check for duplicated requests.
                self.sender.try_send(request).map_err(|err| match err {

Hey, why not use the blocking send? it's perfect for us

Code quote:

self.sender.try_send(request).map_err(|err| match err {

[Yoni-Starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/state/contract_class_manager.rs line 118 at r12 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Hey, why not use the blocking send? it's perfect for us

Oh, it might block the main thread

[avi-starkware]

crates/blockifier/src/state/global_cache.rs line 55 at r12 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Should it be changed to Compiled class as well? If so, I'll just put it here to see if it changed after a rebase.

Done.
Let me know what you think

[avi-starkware]

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @meship-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/state/contract_class_manager.rs line 66 at r12 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

SierraToCasm ???
Add a TODO to fix this, and to pass compilation config through the manager config

Right now all this config holds is the max_bytecode limit argument sent to the cairo lang compiler.
I will add the config in the PR that adds the resource limits to the compilation process.

---

crates/blockifier/src/state/contract_class_manager.rs line 91 at r12 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why?

If I don't drop it, it doesn't let me return the contract_class_manager at the end, because the mutex guard borrows the join_handle field. Here's the error I get from clippy:

   cannot move out of `contract_class_manager` because it is borrowed

---

crates/blockifier/src/state/contract_class_manager.rs line 213 at r12 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Why is it ok to unwrap here? I understand only one thread should access this handle, so there should be no problem, but I am not sure that is enough. Consider implementing a self.lock_join_handle() that will expect with an error message and use it here.

Done.

I don't think this needs another function. It doesn't save much and it forces the message to be the same in both places the mutex is taken.

[Yoni-Starkware]

Reviewed 1 of 1 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

crates/blockifier/src/execution/native/contract_class_manager.rs line 33 at r2 (raw file):

Previously, avi-starkware wrote…

Done.

The termination mechanism and drop might not be strictly necessary, because when the Sender end is dropped the channel closes (unless there are additional clones of the Sender...). When the channel closes, receiver.iter() ends without error.

See here. When a sender is dropped the function disconnect_senders is called, which calls disconnect_receivers if there are 0 senders.

So perhaps we can assume dropping the contract class manager automatically stops the thread since it drops the sender and therefore the receiver. WDYT?

[Yoni-Starkware]

Reviewed 1 of 3 files at r15, 1 of 2 files at r16, 1 of 1 files at r20, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/state/global_cache.rs line 56 at r20 (raw file):

#[cfg(feature = "cairo_native")]
pub struct ContractCaches {
    pub casm_cache: GlobalContractCache<RunnableCompiledClass>,

For now, be consistent with native's type here

Suggestion:

CompiledClassV1

[avi-starkware]

crates/blockifier/src/state/global_cache.rs line 56 at r20 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

For now, be consistent with native's type here

But where will we cache Cairo 0 contracts? Do you want to add another global cache for that?

[Yoni-Starkware]

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

---

crates/blockifier/src/state/global_cache.rs line 56 at r20 (raw file):

Previously, avi-starkware wrote…

But where will we cache Cairo 0 contracts? Do you want to add another global cache for that?

Hmmm you are right. I think it's a good idea to separate the caches - but in a separate PR.
Resolved

[avi-starkware]

Reviewed 1 of 2 files at r4, 1 of 3 files at r15, 1 of 2 files at r16, 1 of 1 files at r20, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)