refactor: separate compile and execute functions in sdk

[gdmlcjs]

Splits the GenericSdk::execute_* methods into GenericSdk::compile_* and GenericSdk::execute_compiled_* methods for pure, metered and metered cost. This is to be able to reuse the "compiled" instance which, for rvr, takes a long time to create.

closes INT-7842

[ghost]

Claude finished @gdmlcjs's task in 2m 59s —— View job

---

Code Review: Separate compile and execute functions in SDK

• Gather context and read changed files
• Analyze changes against base branch
• Provide code review feedback

---

Summary: This PR splits each of the three execution methods (execute, execute_metered, execute_metered_cost) into separate compile and execute phases, enabling users to compile once and run multiple times with different inputs. The type definitions are extracted into a new compiled.rs module.

Overall Assessment

Clean refactor with a good design pattern. The original convenience methods are preserved as wrappers (compile + execute_compiled), so this is fully backwards-compatible. The feature-gated type aliases in compiled.rs are well-organized.

Suggestions

1. Missing doc comment on CompiledExeMeteredCost (crates/sdk/src/compiled.rs:32-35)

   CompiledExeMetered has a doc comment explaining the bundling pattern, but CompiledExeMeteredCost is undocumented. For consistency, add a similar brief doc comment.

   /// Bundles a [`MeteredCostInstance`] with a precomputed [`MeteredCostCtx`] so each execution
   /// just clones the ctx instead of rebuilding it.
   pub struct CompiledExeMeteredCost {

2. Asymmetry between compile_pure and compile_metered/compile_metered_cost

   compile_pure uses self.executor.instance(&exe) directly, while compile_metered and compile_metered_cost go through self.app_prover(app_exe)?, which lazily generates the AppProvingKey — an expensive operation. This isn't a regression (the old code did the same), but it's worth considering whether the metered compile paths could avoid proving key generation in a future follow-up. A brief doc note on compile_metered/compile_metered_cost mentioning that they trigger key generation would help users understand the cost.

3. Consider adding MeteredInstance and MeteredCostInstance to the re-exports (crates/sdk/src/lib.rs:90-92)

   Currently only CompiledExePure, CompiledExeMetered, and CompiledExeMeteredCost are re-exported. If downstream code needs to name MeteredInstance or MeteredCostInstance types directly (e.g., for storing compiled state in custom structs), they'd have to reach into compiled::. Consider whether these should also be in the top-level re-export:

   pub use compiled::{
       CompiledExeMetered, CompiledExeMeteredCost, CompiledExePure,
       MeteredInstance, MeteredCostInstance,
   };

   If callers are only expected to use the CompiledExe* wrapper types, then current re-exports are fine.

Nits

• The compiled.rs imports MeteredCostCtx and MeteredCtx at line 1 unconditionally, but these are only used inside the struct definitions at the bottom of the file (and inside cfg_if! for MeteredInstance/MeteredCostInstance). This works because the structs are not behind cfg, but it's worth noting the import is correct.

Verdict

Looks good to merge. The only actionable item is the missing doc comment on CompiledExeMeteredCost — the other points are optional considerations.

---

[ghost]

Code Review: Separate compile and execute functions in SDK

• Gather context and read changed files
• Analyze changes against base branch
• Provide code review feedback

---

Summary: This PR splits each of the three execution methods (execute, execute_metered, execute_metered_cost) into separate compile and execute phases, enabling callers to compile once and run multiple times with different inputs. The type definitions are cleanly extracted into a new compiled.rs module. The original methods are preserved as wrappers, so this is fully backwards-compatible.

Overall Assessment

Clean, well-structured refactor. The compile/execute split is a natural decomposition and the backwards-compatible wrapper pattern is the right call. No correctness issues found.

Suggestions

1. Missing doc comment on CompiledExeMeteredCost (crates/sdk/src/compiled.rs:32)

   CompiledExeMetered has a doc comment explaining the bundling pattern, but CompiledExeMeteredCost is undocumented. Add a parallel doc comment for consistency:

   /// Bundles a [`MeteredCostInstance`] with a precomputed [`MeteredCostCtx`] so each execution
   /// just clones the ctx instead of rebuilding it.
   pub struct CompiledExeMeteredCost {

   Fix this →

2. compile_metered / compile_metered_cost silently trigger proving key generation

   These go through self.app_prover(app_exe)? which lazily generates the AppProvingKey — an expensive one-time cost that compile_pure avoids. This is inherited behavior (not a regression), but since the point of this PR is to make compile cost visible and reusable, a brief doc note would help callers understand what they're paying for:

   /// Compile `app_exe` for metered execution. The returned [`CompiledExeMetered`] bundles
   /// a precomputed [`MeteredCtx`] so subsequent runs just clone it.
   ///
   /// Note: this lazily generates the [`AppProvingKey`] if it hasn't been created yet.
   

3. Type erasure inconsistency across feature flags (crates/sdk/src/compiled.rs:7-23)

   Under rvr, CompiledExePure is a concrete struct (RvrPureInstance) without a generic parameter, while under aot and default it's a type alias with <F>. The SDK methods use CompiledExePure<F> (e.g., lib.rs:330), which works because under rvr the <F> is a concrete type param on RvrPureInstance<F> re-exported directly. This is correct but subtle — the naming convention makes it look like CompiledExePure under rvr should be used without a type parameter (since it's a pub use, not a pub type). No action needed, just calling attention to it.

Verdict

Looks good to merge. The only actionable item is the missing doc comment on CompiledExeMeteredCost (#1) — the rest are optional polish.

---

View job run

[ghost]

Claude finished @gdmlcjs's task in 1m 50s —— View job

---

Code Review: Separate compile and execute functions in SDK

• Gather context and read changed files
• Analyze changes against base branch
• Provide code review feedback

---

Summary: This PR splits the three SDK execution methods (execute, execute_metered, execute_metered_cost) into separate compile and execute phases, allowing callers to compile once and execute many times with different inputs. A new compiled.rs module provides feature-gated type aliases for the compiled instance types.

Overall Assessment

Clean, well-structured refactor. The original convenience methods are preserved as thin wrappers (compile + execute_compiled), making this fully backwards-compatible. The feature-gated type aliases in compiled.rs correctly handle rvr, aot, and default backends. All referenced types (RvrPureInstance, RvrMeteredInstance, RvrMeteredCostInstance) are properly exported from openvm_circuit::arch::rvr. Existing callers in crates/cli/src/commands/run.rs continue to work without changes.

Findings

1. Missing doc comment on CompiledExeMeteredCost (crates/sdk/src/compiled.rs:32-35) — Low severity

CompiledExeMetered has a doc comment explaining the bundling pattern, but CompiledExeMeteredCost is undocumented. For consistency:

/// Bundles a [`MeteredCostInstance`] with a precomputed [`MeteredCostCtx`] so each execution
/// just clones the ctx instead of rebuilding it.
pub struct CompiledExeMeteredCost {

Fix this →

2. Naming asymmetry in execute methods — Nit

The pure variant is execute_compiled while the metered variants are execute_compiled_metered / execute_compiled_metered_cost. This mirrors the existing naming (execute vs execute_metered), so it's consistent with the existing convention — but execute_compiled_pure would make the trio more self-documenting. No action needed if you prefer the current style.

Observations (no action required)

• compile_metered / compile_metered_cost trigger key generation via self.app_prover(app_exe)? — same as the original code, not a regression. Worth noting for callers expecting a lightweight compile step: the first call lazily generates the AppProvingKey.
• CompiledExeMetered / CompiledExeMeteredCost have all-pub fields, which is fine for flexibility but means callers can construct them directly. If you want to enforce that they're only built via compile_*, you could make fields pub(crate).
• Context cloning (compiled.ctx.clone()) in the execute methods is the correct approach for re-use — each execution consumes the ctx, so cloning it from the compiled bundle is necessary.

Verdict

Looks good to merge. The only actionable item is the missing doc comment on CompiledExeMeteredCost.

---

[ghost]

group	app.proof_time_ms	app.cycles	leaf.proof_time_ms
fibonacci	3,735	12,000,265	917
keccak	18,495	18,655,329	3,277
sha2_bench	10,169	14,793,960	1,457
regex	1,385	4,137,067	351
ecrecover	602	123,583	258
pairing	900	1,745,757	259
kitchen_sink	1,891	2,579,903	409

Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights.

Commit: 21987f7

Benchmark Workflow