From 652a7e9a187c35a3e0b3e33d68ffc9df6fe12e70 Mon Sep 17 00:00:00 2001 From: Julien Cretin Date: Fri, 12 Jan 2024 17:25:27 +0100 Subject: [PATCH] Add review with footprint comparison (#370) --- .github/workflows/ci.yml | 25 +++++- crates/xtask/src/footprint.rs | 156 +++++++++++++++++++++++----------- crates/xtask/src/main.rs | 23 ++--- scripts/footprint.sh | 21 +++-- 4 files changed, 153 insertions(+), 72 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7f35f1a2a..784237af1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,11 +14,12 @@ concurrency: group: ci-${{ github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} +permissions: {} + jobs: changelog: if: github.event_name == 'pull_request' runs-on: ubuntu-latest - permissions: {} steps: - uses: actions/checkout@v4 with: @@ -27,7 +28,6 @@ jobs: copyright: if: github.event_name == 'pull_request' runs-on: ubuntu-latest - permissions: {} steps: - uses: actions/checkout@v4 - run: ./scripts/ci-copyright.sh @@ -61,3 +61,24 @@ jobs: with: name: footprint path: footprint-push.toml + - uses: actions/github-script@v7 + if: github.event_name == 'pull_request' + id: main-run + with: + script: | + const { data } = await github.rest.actions.listWorkflowRuns({ + owner: context.repo.owner, + repo: context.repo.repo, + workflow_id: 'ci.yml', + head_sha: '${{ github.event.pull_request.base.sha }}', + }); + return data.workflow_runs[0]?.id ?? 0; + - uses: actions/download-artifact@v4 + if: github.event_name == 'pull_request' + with: + name: footprint + github-token: ${{ secrets.GITHUB_TOKEN }} + run-id: ${{ steps.main-run.outputs.result }} + continue-on-error: true + - run: cargo xtask footprint "$GITHUB_STEP_SUMMARY" + if: github.event_name == 'pull_request' diff --git a/crates/xtask/src/footprint.rs b/crates/xtask/src/footprint.rs index c7eab58bc..5db839f34 100644 --- a/crates/xtask/src/footprint.rs +++ b/crates/xtask/src/footprint.rs @@ -12,57 +12,97 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::{HashMap, HashSet}; +use std::collections::hash_map::Entry; +use std::collections::{BTreeSet, HashMap}; +use std::fs::{File, OpenOptions}; +use std::io::Write; -use anyhow::{ensure, Context, Result}; +use anyhow::{anyhow, ensure, Context, Result}; use serde::{Deserialize, Serialize}; use crate::{fs, output_command, wrap_command}; -#[derive(Serialize, Deserialize)] +#[derive(Default, Serialize, Deserialize)] struct Footprint { - version: usize, - measurements: Vec, + row: Vec, } #[derive(Serialize, Deserialize)] -struct Measurement { - key: Vec, - value: usize, +struct Row { + config: String, + #[serde(flatten)] + value: HashMap, +} + +pub fn update_applet(config: &str, value: usize) -> Result<()> { + update("applet", config, value) } -pub fn update(key: &str, value: usize) -> Result<()> { +pub fn update_runner(config: &str, value: usize) -> Result<()> { + update("runner", config, value) +} + +fn update(key: &str, config: &str, value: usize) -> Result<()> { const PATH: &str = "footprint.toml"; - let mut footprint = Footprint::read(PATH)?; - let key = key.split_whitespace().map(|x| x.to_string()).collect(); - let measurement = Measurement { key, value }; - match footprint.measurements.iter_mut().find(|x| x.key == measurement.key) { - Some(x) => x.value = value, - None => footprint.measurements.push(measurement), - } + let mut footprint = match fs::exists(PATH) { + true => fs::read_toml(PATH)?, + false => Footprint::default(), + }; + let idx = match footprint.row.binary_search_by_key(&config, |x| &x.config) { + Ok(x) => x, + Err(x) => { + let row = Row { config: config.to_string(), value: HashMap::new() }; + footprint.row.insert(x, row); + x + } + }; + ensure!( + footprint.row[idx].value.insert(key.to_string(), value).is_none(), + "{key} is already defined for {config:?}" + ); fs::write_toml(PATH, &footprint)?; Ok(()) } -pub fn compare() -> Result<()> { - let base = Footprint::read_measurements("footprint-push.toml")?; - let head = Footprint::read_measurements("footprint-pull_request.toml")?; - let keys: HashSet<&Vec> = base.keys().chain(head.keys()).collect(); - let mut keys: Vec<&Vec> = keys.into_iter().collect(); - keys.sort(); - let print = |k, x| match x { - Some(x) => print!(" {x}"), - None => print!(" no-{k}"), +pub fn compare(output: &str) -> Result<()> { + let mut output = OpenOptions::new().create(true).append(true).open(output)?; + let base = Footprint::read("footprint-push.toml"); + let head = Footprint::read("footprint-pull_request.toml"); + let configs: BTreeSet<&String> = base.keys().chain(head.keys()).collect(); + writeln!(output, "### Footprint impact\n")?; + writeln!(output, "| Config | Key | Base | Head | Diff | Ratio |")?; + writeln!(output, "| --- | --- | ---: | ---: | ---: | ---: |")?; + let write = |output: &mut File, x| match x { + Some(x) => write!(output, " {x} |"), + None => write!(output, " |"), }; - for key in keys { - print!("{key:?}"); - let base = base.get(key).cloned(); - let head = head.get(key).cloned(); - let diff = base.and_then(|base| head.map(|head| head - base)); - print("base", base); - print("head", head); - print("diff", diff); - println!(); + for config in configs { + for key in ["total", "applet", "runner"] { + let base = base.get(config).map(|x| x[key]); + let head = head.get(config).map(|x| x[key]); + let diff = base.and_then(|base| head.map(|head| head - base)); + write!(output, "| {config} | {key} |")?; + write(&mut output, base)?; + write(&mut output, head)?; + write(&mut output, diff)?; + match diff { + Some(diff) => { + let base = base.unwrap(); + let ratio = (diff as f64 / base as f64 * 100.) as i64; + let emoji = match ratio { + i64::MIN ..= -10 => ":+1: ", + -9 ..= 29 => "", + 30 ..= i64::MAX => ":-1: ", + }; + if !emoji.is_empty() { + println!("::notice::{config:?} {key} {emoji}{ratio}%"); + } + write!(output, " {emoji}{ratio}% |")?; + } + None => write!(output, " |")?, + } + writeln!(output)?; + } } Ok(()) } @@ -79,24 +119,38 @@ pub fn rust_size(elf: &str) -> Result { } impl Footprint { - fn read(path: &str) -> Result { - let footprint = if fs::exists(path) { - fs::read_toml(path)? - } else { - Footprint { version: VERSION, measurements: Vec::new() } - }; - ensure!(footprint.version == VERSION); - Ok(footprint) - } - - fn read_measurements(path: &str) -> Result, usize>> { + fn read(path: &str) -> HashMap> { let mut result = HashMap::new(); - let footprint = Self::read(path)?; - for Measurement { key, value } in footprint.measurements { - ensure!(result.insert(key, value).is_none(), "duplicate key in {path}"); + let footprint = fs::read_toml(path).unwrap_or_else(|err| { + println!("::warning::{err}"); + Footprint::default() + }); + for Row { config, mut value } in footprint.row { + let value: Result<_> = try { + let missing = |key| format!("Missing {key} for {config:?} in {path}"); + let applet = value.remove("applet").with_context(|| missing("applet"))?; + let runner = value.remove("runner").with_context(|| missing("runner"))?; + if !value.is_empty() { + Err(anyhow!("Extra keys for {config:?} in {path}"))?; + } + let mut value = HashMap::new(); + value.insert("applet", applet as i64); + value.insert("total", runner as i64); + value.insert("runner", runner as i64 - applet as i64); + value + }; + let value = match value { + Ok(x) => x, + Err(err) => { + println!("::warning::{err}"); + continue; + } + }; + match result.entry(config) { + Entry::Occupied(x) => println!("::warning::Duplicate {:?} in {path}", x.key()), + Entry::Vacant(x) => drop(x.insert(value)), + } } - Ok(result) + result } } - -const VERSION: usize = 1; diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs index cc10b4bae..86f55d813 100644 --- a/crates/xtask/src/main.rs +++ b/crates/xtask/src/main.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +#![feature(try_blocks)] + use std::cell::Cell; use std::cmp::Reverse; use std::collections::BinaryHeap; @@ -69,7 +71,7 @@ struct MainOptions { #[clap(long)] size: bool, - /// Updates footprint.toml to make the measured footprint to the provided key. + /// Updates footprint.toml with the measured footprint for the provided key. /// /// The key is a space-separated list of strings. #[clap(long)] @@ -91,10 +93,13 @@ enum MainCommand { features: Vec, }, - /// Dumps a table comparison between footprint-base.toml and footprint-pull_request.toml. + /// Appends a comparison between footprint-base.toml and footprint-pull_request.toml. /// - /// If footprint-base.toml is missing, it is omitted from the table. - Footprint, + /// If any file is missing, it is assumed to have no measurements. + Footprint { + /// The markdown table is written to this file. + output: String, + }, } #[derive(clap::Args)] @@ -279,7 +284,7 @@ impl Flags { cargo.arg(format!("--output=examples/{lang}/api.{ext}")); execute_command(&mut cargo)?; } - MainCommand::Footprint => footprint::compare()?, + MainCommand::Footprint { output } => footprint::compare(&output)?, } Ok(()) } @@ -386,8 +391,7 @@ impl AppletOptions { } } if let Some(key) = &main.footprint { - let value = footprint::rust_size(applet)?; - footprint::update(key, value)?; + footprint::update_applet(key, footprint::rust_size(applet)?)?; } } if native.is_none() && changed { @@ -447,7 +451,7 @@ impl AppletOptions { } } if let Some(key) = &main.footprint { - footprint::update(key, fs::metadata(wasm)?.len() as usize)?; + footprint::update_applet(key, fs::metadata(wasm)?.len() as usize)?; } Ok(()) } @@ -607,8 +611,7 @@ impl RunnerOptions { execute_command(&mut size)?; } if let Some(key) = &main.footprint { - let value = footprint::rust_size(&elf)?; - footprint::update(key, value)?; + footprint::update_runner(key, footprint::rust_size(&elf)?)?; } if let Some(stack_sizes) = self.stack_sizes { let elf = fs::read(&elf)?; diff --git a/scripts/footprint.sh b/scripts/footprint.sh index 8f2e7a85c..dfcd36a95 100755 --- a/scripts/footprint.sh +++ b/scripts/footprint.sh @@ -20,18 +20,21 @@ set -e # different configurations. The continuous integration will use this to show # footprint impact in pull requests. +[ -e footprint.toml ] && e "footprint.toml already exists" + bool() { if [ -n "$1" ]; then echo "$2"; else echo "$3"; fi; } +applet=exercises/part-7-sol runner=nordic -for applet in hello hsm; do - for release in '' --release; do - for native in '' --native-target=thumbv7em-none-eabi; do - config="$(bool "$release" release debug)" - config="$config $(bool "$native" native wasm)" - x cargo xtask --footprint="applet $applet $config" \ - $release $native applet rust $applet - x cargo xtask --footprint="runner $runner $config" \ - $release ${native:+--native} runner $runner +for native in '' --native-target=thumbv7em-none-eabi; do + for opt_level in '' --opt-level=z; do + for release in '' --release; do + config="$(bool "$release" release debug) $(bool "$native" native wasm)" + config="$config $(bool "$opt_level" small fast)" + x cargo xtask --footprint="$config" \ + $release $native applet rust $applet $opt_level + x cargo xtask --footprint="$config" \ + $release ${native:+--native} runner $runner $opt_level done done done