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

Add cargo risczero benchmark command #1302

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
3cf5675
feat: Add `cargo risczero benchmark` command
Cardosaum Jan 8, 2024
4d9460b
Merge branch 'main' into cardosaum/add-cargo-risczero-benchmark
Cardosaum Jan 8, 2024
81515a7
chore: format and sort dependencies
Cardosaum Jan 8, 2024
5e90ce1
Refactor benchmark command to handle iterations
Cardosaum Jan 10, 2024
11c9cbf
Add risc-zkvm-methods crate to workspace
Cardosaum Jan 12, 2024
6baaca7
feat(cargo-build): Add ability to compile specified binaries.
Cardosaum Jan 22, 2024
f37a441
Add tracing, sha2, and serde_json dependencies
Cardosaum Jan 22, 2024
e13ac03
Add logic to download specified ELF binary from GitHub
Cardosaum Jan 22, 2024
80a8ba8
Add words to cSpell dictionary
Cardosaum Jan 22, 2024
852500c
Merge branch 'main' into cardosaum/add-cargo-risczero-benchmark
Cardosaum Jan 22, 2024
e652c76
chore: sort dependencies
Cardosaum Jan 22, 2024
e1a1bf0
create `risc0-zkvm-methods-core`
Cardosaum Jan 23, 2024
25079ef
Remove unused code and logging initialization
Cardosaum Jan 23, 2024
2856c23
Fix commit_slice argument in multi_test.rs
Cardosaum Jan 23, 2024
945f73d
Merge branch 'main' into cardosaum/add-cargo-risczero-benchmark
Cardosaum Jan 24, 2024
cff8bcd
Revert "feat(cargo-build): Add ability to compile specified binaries."
Cardosaum Jan 24, 2024
153b412
Add xtask to compile benchmark elf
Cardosaum Jan 24, 2024
fb37fcb
Remove unused dependencies in Cargo.toml
Cardosaum Jan 24, 2024
9351187
Add empty line between imports and struct definition
Cardosaum Jan 24, 2024
1ac3c65
cargo fmt
Cardosaum Jan 24, 2024
fb76458
cargo sort
Cardosaum Jan 24, 2024
671a6bf
Add logic to generate `benchmark_elf.rs` during build time
Cardosaum Jan 24, 2024
3f8833c
update cspell
Cardosaum Jan 24, 2024
4572b4e
fix import for risc0-zkvm-methods-core in multiple crates
Cardosaum Jan 24, 2024
09a25f0
cargo sort
Cardosaum Jan 24, 2024
3f38719
Merge branch 'main' into cardosaum/add-cargo-risczero-benchmark
Cardosaum Jan 24, 2024
735a6e6
fix path for risc0-zkvm-methods-core
Cardosaum Jan 25, 2024
20ff703
Fix extra bracket in executor.rs example
Cardosaum Jan 25, 2024
9486165
Update import statement in guest_run.rs
Cardosaum Jan 25, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,11 @@
"risc0/zkvm/methods/guest/Cargo.toml",
"risc0/zkvm/methods/std/Cargo.toml",
"tools/crates-validator/Cargo.toml",
],
"cSpell.words": [
"canonicalize",
"risczero",
"tempdir",
"tmpdir"
]
}
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ risc0-sys = { version = "0.21.0-alpha.1", default-features = false, path = "risc
risc0-zkp = { version = "0.21.0-alpha.1", default-features = false, path = "risc0/zkp" }
risc0-zkvm = { version = "0.21.0-alpha.1", default-features = false, path = "risc0/zkvm" }
risc0-zkvm-platform = { version = "0.21.0-alpha.1", default-features = false, path = "risc0/zkvm/platform" }
risc0-zkvm-methods = { version = "0.21.0-alpha.1", default-features = false, path = "risc0/zkvm/methods" }

[profile.bench]
lto = true
Expand Down
25 changes: 23 additions & 2 deletions risc0/build/src/docker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ const DOCKER_IGNORE: &str = r#"
const TARGET_DIR: &str = "target/riscv-guest/riscv32im-risc0-zkvm-elf/docker";

/// Build the package in the manifest path using a docker environment.
pub fn docker_build(manifest_path: &Path, src_dir: &Path, features: &[String]) -> Result<()> {
pub fn docker_build(
manifest_path: &Path,
src_dir: &Path,
binaries: &[String],
features: &[String],
) -> Result<()> {
let manifest_path = manifest_path
.canonicalize()
.context(format!("manifest_path: {manifest_path:?}"))?;
Expand Down Expand Up @@ -69,14 +74,24 @@ pub fn docker_build(manifest_path: &Path, src_dir: &Path, features: &[String]) -
let temp_dir = tempdir()?;
let temp_path = temp_dir.path();
let rel_manifest_path = manifest_path.strip_prefix(&src_dir)?;
create_dockerfile(&rel_manifest_path, temp_path, pkg_name.as_str(), features)?;
create_dockerfile(
&rel_manifest_path,
temp_path,
pkg_name.as_str(),
binaries,
features,
)?;
build(&src_dir, temp_path)?;
}
println!("ELFs ready at:");

let target_dir = src_dir.join(TARGET_DIR);
for target in root_pkg.targets.iter() {
if target.is_bin() {
if !binaries.is_empty() && !binaries.contains(&target.name) {
println!("Skipping target: {}", target.name);
continue;
}
let elf_path = target_dir.join(&pkg_name).join(&target.name);
let image_id = compute_image_id(&elf_path)?;
let rel_elf_path = Path::new(TARGET_DIR).join(&pkg_name).join(&target.name);
Expand All @@ -94,6 +109,7 @@ fn create_dockerfile(
manifest_path: &Path,
temp_dir: &Path,
pkg_name: &str,
binaries: &[String],
features: &[String],
) -> Result<()> {
let manifest_env = &[("CARGO_MANIFEST_PATH", manifest_path.to_str().unwrap())];
Expand All @@ -111,6 +127,11 @@ fn create_dockerfile(
];

let mut build_args = common_args.clone();
let binaries_str = binaries.join(",");
if !binaries.is_empty() {
build_args.push("--bin");
build_args.push(&binaries_str);
}
let features_str = features.join(",");
if !features.is_empty() {
build_args.push("--features");
Expand Down
9 changes: 9 additions & 0 deletions risc0/build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,11 @@ fn build_guest_package<P>(
cargo_command("build", &[])
};

let binaries_str = guest_opts.bin.join(",");
if !guest_opts.bin.is_empty() {
cmd.args(&["--bin", &binaries_str]);
}

let features_str = guest_opts.features.join(",");
if !features_str.is_empty() {
cmd.args(&["--features", &features_str]);
Expand Down Expand Up @@ -500,6 +505,9 @@ pub struct GuestOptions {
/// Features for cargo to build the guest with.
pub features: Vec<String>,

/// Binary names to build.
pub bin: Vec<String>,

Cardosaum marked this conversation as resolved.
Show resolved Hide resolved
/// Use a docker environment for building.
pub use_docker: Option<DockerOptions>,
}
Expand Down Expand Up @@ -557,6 +565,7 @@ pub fn embed_methods_with_options(mut guest_pkg_to_options: HashMap<&str, GuestO
docker_build(
guest_pkg.manifest_path.as_std_path(),
&src_dir,
&guest_opts.bin,
&guest_opts.features,
)
.unwrap();
Expand Down
6 changes: 5 additions & 1 deletion risc0/cargo-risczero/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,17 @@ reqwest-middleware = "0.2"
reqwest-retry = "0.3"
risc0-build = { workspace = true }
risc0-r0vm = { workspace = true, optional = true }
risc0-zkvm = { workspace = true, optional = true }
risc0-zkvm = { workspace = true, default-features = false, features = ["std", "getrandom"] }
risc0-zkvm-methods = { workspace = true, default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be dropped, it can't be published

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

I was thinking in moving the dependencies we have on risc0-zkvm-methods to a new crate risc0-zkvm-methods-core that we could publish, this way we would solve the issue. That way both risc0-zkvm-methods and cargo-risczero could depend on risc0-zkvm-methods-core.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think any of the benchmark binaries need any 'core' crates because the interface is very simple. We pass in the number of iterations to run, so there's no types that need to be shared between host and guest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see... I followed this approach because this would be similar to what we do under /examples having a core that can be used both in guest and host.

I did the change before seeing this comment. Please let me know if you'd like to revert the new risc0-zkvm-methods-core and I'll change it 🙇‍♂️

serde = { version = "1", features = ["derive"] }
serde_json = "1.0.96"
sha2 = "0.10.8"
syn = "2.0.38"
tar = "0.4"
tempfile = "3"
text_io = "0.1.12"
tokio = { version = "1", features = ["rt"] }
tracing = { version = "0.1", features = ["log"] }
tracing-subscriber = { version = "0.3", features = ["env-filter"] }
zip = { version = "0.6", optional = true }

Expand Down
1 change: 1 addition & 0 deletions risc0/cargo-risczero/src/bin/cargo-risczero.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ fn main() -> Result<()> {
RisczeroCmd::BuildToolchain(cmd) => cmd.run(),
RisczeroCmd::Install(cmd) => cmd.run(),
RisczeroCmd::New(cmd) => cmd.run(),
RisczeroCmd::Benchmark(cmd) => cmd.run(),
#[cfg(feature = "experimental")]
RisczeroCmd::BuildCrate(build) => build.run(BuildSubcommand::Build),
#[cfg(feature = "experimental")]
Expand Down