Add review with footprint comparison (#370)

.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'

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<Measurement>,
+    row: Vec<Row>,
 }
 
 #[derive(Serialize, Deserialize)]
-struct Measurement {
-    key: Vec<String>,
-    value: usize,
+struct Row {
+    config: String,
+    #[serde(flatten)]
+    value: HashMap<String, usize>,
+}
+
+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<String>> = base.keys().chain(head.keys()).collect();
-    let mut keys: Vec<&Vec<String>> = 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<usize> {
 }
 
 impl Footprint {
-    fn read(path: &str) -> Result<Self> {
-        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<HashMap<Vec<String>, usize>> {
+    fn read(path: &str) -> HashMap<String, HashMap<&'static str, i64>> {
         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;

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<String>,
     },
 
-    /// 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)?;

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