add run_with_libfunc_profile + AotWithProgram variant for ContractExecutor#1599
add run_with_libfunc_profile + AotWithProgram variant for ContractExecutor#1599avi-starkware wants to merge 3 commits into
Conversation
Adds an adapter type and feature flag that lets a cairo-native
StarknetSyscallHandler drive the sierra-emu virtual machine, removing
the need for downstream consumers (e.g. blockifier in starknet-libs/sequencer)
to maintain their own copy of the conversion glue between the two crates.
- New `sierra-emu` feature flag in Cargo.toml that enables `dep:sierra-emu`
and the new `sierra_emu_bridge` module. `with-trace-dump` now depends on
the new feature instead of pulling in `dep:sierra-emu` directly.
- `SierraEmuSyscallBridge<'a, H>` wraps any `&mut H` where
`H: cairo_native::starknet::StarknetSyscallHandler` and implements
`sierra_emu::starknet::StarknetSyscallHandler` for it.
- Conversion helpers (`convert_u256`, `convert_secp_*_point`,
`convert_execution_info{,_v2}`, etc.) handle the type-level translation
between the two crates' overlapping but distinct syscall surfaces.
- The Secp256k1 / Secp256r1 conversion treats `(0, 0)` as the point at
infinity when going from sierra-emu (which has no `is_infinity` flag)
to cairo-native — sierra-emu represents the identity element as `(0, 0)`
but cairo-native expects an explicit flag.
Adds a public dispatch enum that lets a single call site choose between
the AOT executor and the sierra-emu interpreter at runtime, without
forcing every caller to maintain its own match.
- `pub enum ContractExecutor { Aot(AotContractExecutor), Emu(EmuContractInfo) }`,
with `Emu` gated on the `sierra-emu` feature added in the previous PR.
- `From` impls for both variants.
- `ContractExecutor::run` dispatches: the `Aot` arm calls
`AotContractExecutor::run` directly; the `Emu` arm constructs a
`sierra_emu::VirtualMachine`, wraps the cairo-native syscall handler in
`SierraEmuSyscallBridge`, and converts the result.
- `EmuContractInfo` carries `Arc<Program>` so the program is shared across
invocations rather than cloned per call.
This is the same dispatch shape that downstream consumers (e.g. the
blockifier in starkware-libs/sequencer) currently maintain locally; with
this PR they can drop their copy and import `cairo_native::ContractExecutor`.
…cutor
Exposes the libfunc-profiling primitives that downstream consumers (e.g. the
blockifier in starkware-libs/sequencer) currently maintain locally. Makes the
profile-collection pattern callback-driven so the per-call key (tx hash, etc.)
stays out of cairo-native.
- `Profile` (HashMap<ConcreteLibfuncId, LibfuncProfileData>) is now public.
- `AotContractExecutor::run_with_libfunc_profile<H, F>` (gated on
`with-libfunc-profiling`) wraps `run`: allocates a unique trace ID, points
the executor's `cairo_native__profiler__profile_id` symbol at it, drains
the resulting `Profile` after `run` returns, and hands it to a caller-
supplied `FnOnce(Profile)`. A `ProfilerGuard` restores the previous trace
ID and drops the LIBFUNC_PROFILE slot on both the success and unwind paths.
- `ContractExecutor::AotWithProgram(AotWithProgram { executor, program })`
is a new variant that bundles an AOT executor with the Sierra program it
was built from. `From<AotWithProgram>` is provided.
- `ContractExecutor::run` dispatches the new variant via
`run_with_libfunc_profile` with a no-op profile callback.
- `ContractExecutor::run_with_profile<H, F>` is the profile-capturing
counterpart of `run`; for non-`AotWithProgram` variants it falls through
to `run` (callback never fires).
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on avi-starkware, TomerStarkware, and Yoni-Starkware).
src/sierra_emu_bridge.rs line 23 at r1 (raw file):
pub struct SierraEmuSyscallBridge<'a, H>(pub &'a mut H); impl<H: native::StarknetSyscallHandler> emu::StarknetSyscallHandler
sounds like this trait should be in a base crate that both native and emu depends on - instead of a full fuplication.
Code quote:
StarknetSyscallHandler
|
Previously, orizi wrote…
The main issue is that sierra-emu has its own versions of basic types (SyscallResult, ExecutionInfo, ExecutionInfoV2, U256), and uses Vec instead of slices for arrays of Felts and u64. Moreover, the StarknetSyscallHandler trait is different for cairo_native and emu in several small ways (e.g., cairo-native has get_exuction_info_v3 while sierra-emu doesn't; sierra-emu has cheatcode while cairo-native doesn't; the signature of sha256_process_block is different). I think the way to cleanly share code between the two would be to create a base types crates (similar to starknet-api) and have both crates depend on it. It is a much bigger refactor than what I am proposing here though. |
orizi
left a comment
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on avi-starkware, TomerStarkware, and Yoni-Starkware).
src/sierra_emu_bridge.rs line 23 at r1 (raw file):
Previously, avi-starkware (Avi Cohen) wrote…
The main issue is that sierra-emu has its own versions of basic types (SyscallResult, ExecutionInfo, ExecutionInfoV2, U256), and uses Vec instead of slices for arrays of Felts and u64. Moreover, the StarknetSyscallHandler trait is different for cairo_native and emu in several small ways (e.g., cairo-native has get_exuction_info_v3 while sierra-emu doesn't; sierra-emu has cheatcode while cairo-native doesn't; the signature of sha256_process_block is different).
I think the way to cleanly share code between the two would be to create a base types crates (similar to starknet-api) and have both crates depend on it. It is a much bigger refactor than what I am proposing here though.
i do not agree it is a bigger refactor. this refactor is just made obsolete by it.
it would require:
- PR to make the traits exactly the same structure-wise.
- PR to create a new crate.
- PRs to use that new crate.
|
Previously, orizi wrote…
Changing the traits might lead to many other small changes in the crates' implementation, as opposed to just using the crates as they are already implemented. That is why I said it is a bigger refactor. Anyway, you are right that it is the better way. I will give it a try. |
2 participants
Summary
Exposes the libfunc-profiling primitives that downstream consumers (e.g. the blockifier in
starkware-libs/sequencer) currently maintain locally. The profile-collection pattern is callback-driven so the per-call key (tx hash, etc.) stays out of cairo-native.Important
Stacked on #1597 and #1598. Merge those first; this PR's diff will narrow once they land.
Changes
metadata::profiler::Profileis nowpub(was a private type alias).AotContractExecutor::run_with_libfunc_profile<H, F>(gated onwith-libfunc-profiling, in new filesrc/executor/libfunc_profile.rs) wrapsrun: allocates a unique trace ID, points the executor'scairo_native__profiler__profile_idsymbol at it, drains the resultingProfileafterrunreturns, and hands it to a caller-suppliedFnOnce(Profile). AProfilerGuardrestores the previous trace ID and drops theLIBFUNC_PROFILEslot on both the success and unwind paths.ContractExecutor::AotWithProgram(AotWithProgram { executor, program })is a new variant that bundles an AOT executor with the Sierra program it was built from.From<AotWithProgram>is provided.ContractExecutor::rundispatches the new variant viarun_with_libfunc_profilewith a no-op profile callback.ContractExecutor::run_with_profile<H, F>is the profile-capturing counterpart ofrun; for non-AotWithProgramvariants it falls through torun(callback never fires).Test plan
--features with-libfunc-profiling--features sierra-emu,with-libfunc-profilingContext
Companion sequencer PR that adopts these APIs and drops the locally-maintained profiling code: starkware-libs/sequencer#13880.
This change is