Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cairo_native): use contract class manager in papyrus reader #2738

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

avi-starkware
Copy link
Contributor

@avi-starkware avi-starkware commented Dec 17, 2024

  • feat(cairo_native): use contract class manager in papyrus reader

@reviewable-StarkWare
Copy link

This change is Reviewable

@avi-starkware avi-starkware force-pushed the avi/compilation-thread/papyrus-reader branch from a624362 to 0edeedd Compare December 17, 2024 15:36
@avi-starkware avi-starkware changed the title avi/compilation thread/papyrus reader feat(cairo_native): use contract class manager in papyrus reader Dec 17, 2024
Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 7 files at r1, all commit messages.
Reviewable status: 4 of 7 files reviewed, 4 unresolved discussions (waiting on @alonh5, @avi-starkware, @meship-starkware, and @Yoni-Starkware)


crates/blockifier/src/state/contract_class_manager.rs line 60 at r1 (raw file):

        );
        #[cfg(feature = "cairo_native")]
        {

Consider adding an assert verifying that config.run_cairo_native is set to true only if the native feature is on.

Code quote:

        #[cfg(not(feature = "cairo_native"))]
        unimplemented!(
            "Native compilation cannot be enabled when the cairo_native feature is turned off."
        );
        #[cfg(feature = "cairo_native")]
        {

crates/native_blockifier/src/py_block_executor.rs line 368 at r1 (raw file):

        // Clear global class cache, to properly revert classes declared in the reverted block.
        // TODO(Avi, 01/01/2025): Consider what exactly to clear in native compilation context.
        self.contract_class_manager.clear();

I think that we will want to clear everything, given that we first check the native cache.

Code quote:

        // TODO(Avi, 01/01/2025): Consider what exactly to clear in native compilation context.
        self.contract_class_manager.clear();

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

pub struct ContractCaches {
    pub casm_cache: GlobalContractCache<VersionedRunnableCompiledClass>,

WDYT?

Suggestion:

pub compiled_class_cache: GlobalContractCache<VersionedRunnableCompiledClass>,

crates/blockifier/src/state/global_cache.rs line 58 at r1 (raw file):

    pub casm_cache: GlobalContractCache<VersionedRunnableCompiledClass>,
    #[cfg(feature = "cairo_native")]
    pub native_cache: GlobalContractCache<CachedCairoNative>,

Consider defining this field last

Code quote:

    #[cfg(feature = "cairo_native")]
    pub native_cache: GlobalContractCache<CachedCairoNative>,

Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 7 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alonh5, @avi-starkware, @meship-starkware, and @Yoni-Starkware)


crates/blockifier/src/state/global_cache.rs line 58 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider defining this field last

On second though, as we discussed, we don't need the sierra as well, so you can leave it in this order


crates/starknet_batcher/src/batcher.rs line 449 at r1 (raw file):

        wait_on_native_compilation: false,
        contract_cache_size: config.global_contract_cache_size,
    };

?

Suggestion:

    let contract_class_manager_config = ContractClassManagerConfig::default();

crates/native_blockifier/src/py_block_executor.rs line 437 at r1 (raw file):

            wait_on_native_compilation: false,
            contract_cache_size: GLOBAL_CONTRACT_CACHE_SIZE_FOR_TEST,
        };

?

Suggestion:

        let contract_class_manager_config = ContractClassManagerConfig::default();

Copy link
Contributor

@meship-starkware meship-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @alonh5, @avi-starkware, and @Yoni-Starkware)

@avi-starkware avi-starkware force-pushed the avi/compilation-thread/papyrus-reader branch 2 times, most recently from 390ad45 to 8e0c1dc Compare December 18, 2024 13:29
@avi-starkware
Copy link
Contributor Author

crates/native_blockifier/src/py_block_executor.rs line 368 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I think that we will want to clear everything, given that we first check the native cache.

Done.

Copy link
Contributor Author

@avi-starkware avi-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @alonh5, @meship-starkware, @noaov1, and @Yoni-Starkware)


crates/blockifier/src/state/contract_class_manager.rs line 60 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider adding an assert verifying that config.run_cairo_native is set to true only if the native feature is on.

I just made it impossible to run the compilation thread if the feature flag is turned off.
That is, config.run_cairo_native does nothing in that case.


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

Previously, noaov1 (Noa Oved) wrote…

WDYT?

I think this makes it more ambiguous because the native cache also contains compiled classes.


crates/native_blockifier/src/py_block_executor.rs line 437 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

?

Done.

@avi-starkware avi-starkware force-pushed the avi/compilation-thread/papyrus-reader branch from 8e0c1dc to 2785713 Compare December 18, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants