Improve size tracking table (#3117)

### What - The change diff had the opposite sign - Previous/Current headers now use the base/PR branch names instead - Changed some variable names and re-ordered some code - Added tracking of latest commit hash via an index file that's only updated on `main` ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3117) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/3117) - [Docs preview](https://rerun.io/preview/941ec5ee46923b85efdcb1ecfd654c7ec8e679a0/docs)  - [Examples preview](https://rerun.io/preview/941ec5ee46923b85efdcb1ecfd654c7ec8e679a0/examples)  - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
rerun-io · Aug 29, 2023 · ddf89ca · ddf89ca
1 parent b55e818
commit ddf89ca
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 52 deletions.
diff --git a/.github/workflows/reusable_track_size.yml b/.github/workflows/reusable_track_size.yml
@@ -6,10 +6,6 @@ on:
       CONCURRENCY:
         required: true
         type: string
-      ADHOC_NAME:
-        required: false
-        type: string
-        default: ""
       PR_NUMBER:
         required: false
         type: number
@@ -30,14 +26,7 @@ jobs:
       - name: Get context
         id: context
         run: |
-          echo "head_short_sha=$(echo ${{ github.sha }} | cut -c1-7)" >> "$GITHUB_OUTPUT"
-
-          if [ -n ${{ inputs.PR_NUMBER }} ]; then
-            base_short_sha=$(echo ${{ github.event.pull_request.base.sha }} | cut -c1-7)
-          else
-            base_short_sha=$short_sha
-          fi
-          echo "base_short_sha=$base_short_sha" >> "$GITHUB_OUTPUT"
+          echo "short_sha=$(echo ${{ github.sha }} | cut -c1-7)" >> "$GITHUB_OUTPUT"
 
       - id: "auth"
         uses: google-github-actions/auth@v1
@@ -62,12 +51,29 @@ jobs:
           name: web_demo
           path: web_demo
 
-      - name: Download base branch results
+      - name: Download base results
         run: |
-          file_path="gs://rerun-builds/sizes/commit/${{ steps.context.outputs.base_short_sha }}/data.json"
+          # Get base commit:
+          # 1. From the index file
+          # 2. From the latest commit in the base branch of the PR
+          # 3. From the latest commit in the current branch
+          index_path="gs://rerun-builds/sizes/index"
+          if [ "$(gsutil -q stat $index_path ; echo $?)" = 0 ]; then
+            gsutil cp $index_path "/tmp/base_index"
+            base_commit=$(cat /tmp/base_index)
+          else
+            if [ -n ${{ inputs.PR_NUMBER }} ]; then
+              base_commit=$(echo ${{ github.event.pull_request.base.sha }} | cut -c1-7)
+            else
+              base_commit=${{ steps.context.outputs.short_sha }}
+            fi
+          fi
+          echo "base commit: $base_commit"
 
-          if [ "$(gsutil -q stat $file_path ; echo $?)" = 0 ]; then
-            gsutil cp $file_path "/tmp/prev.json"
+          # Download data for base commit, or default to empty file
+          data_path="gs://rerun-builds/sizes/commit/$base_commit/data.json"
+          if [ "$(gsutil -q stat $data_path ; echo $?)" = 0 ]; then
+            gsutil cp $data_path "/tmp/prev.json"
           else
             echo "[]" > "/tmp/prev.json"
           fi
@@ -87,13 +93,15 @@ jobs:
           done
 
           data=$(python3 scripts/ci/sizes.py measure "${entries[@]}")
-          echo "$data"
-
           echo "$data" > "/tmp/data.json"
 
-          comparison=$(python3 scripts/ci/sizes.py compare --threshold=5 "/tmp/prev.json" "/tmp/data.json")
-          echo "$comparison"
-
+          comparison=$(
+            python3 scripts/ci/sizes.py compare \
+              --threshold=5% \
+              --before-header=${{ (inputs.PR_NUMBER && github.event.pull_request.base.ref) || 'Before' }} \
+              --after-header=${{ github.ref_name }} \
+              "/tmp/prev.json" "/tmp/data.json"
+          )
           {
             echo 'comparison<<EOF'
             echo "$comparison"
@@ -106,22 +114,19 @@ jobs:
             echo "is_comparison_set=false" >> "$GITHUB_OUTPUT"
           fi
 
-      - name: Upload data to GCS (commit)
-        if: inputs.ADHOC_NAME == ''
-        uses: google-github-actions/upload-cloud-storage@v1
-        with:
-          path: /tmp/data.json
-          destination: "rerun-builds/sizes/commit/${{ steps.context.outputs.head_short_sha }}"
+          echo "$entries"
+          echo "previous: $(cat /tmp/prev.json)"
+          echo "current: $(cat /tmp/data.json)"
+          echo "$comparison"
+          echo "is comparison set: $is_comparison_set"
 
-      - name: Upload data to GCS (adhoc)
-        if: inputs.ADHOC_NAME != ''
+      - name: Upload data to GCS (commit)
         uses: google-github-actions/upload-cloud-storage@v1
         with:
           path: /tmp/data.json
-          destination: "rerun-builds/sizes/adhoc/${{ inputs.ADHOC_NAME }}"
+          destination: "rerun-builds/sizes/commit/${{ steps.context.outputs.short_sha }}"
 
       - name: Store result
-        # Only run on `main`
         if: github.ref == 'refs/heads/main'
         # https://github.com/benchmark-action/github-action-benchmark
         uses: benchmark-action/github-action-benchmark@v1
@@ -143,6 +148,18 @@ jobs:
           benchmark-data-dir-path: dev/sizes
           max-items-in-chart: 30
 
+      - name: Create index file
+        if: github.ref == 'refs/heads/main'
+        run: |
+          echo "${{ steps.context.outputs.short_sha }}" > "/tmp/index"
+
+      - name: Upload index
+        if: github.ref == 'refs/heads/main'
+        uses: google-github-actions/upload-cloud-storage@v1
+        with:
+          path: /tmp/index
+          destination: "rerun-builds/sizes"
+
       - name: Create PR comment
         if: inputs.PR_NUMBER != '' && steps.measure.outputs.is_comparison_set == 'true'
         # https://github.com/mshick/add-pr-comment

diff --git a/scripts/ci/sizes.py b/scripts/ci/sizes.py
@@ -53,10 +53,6 @@ def get_divisor(unit: str) -> int:
     return DIVISORS[unit]
 
 
-def cell(value: float, div: float) -> str:
-    return f"{value / div:.2f}"
-
-
 def render_table_dict(data: list[dict[str, str]]) -> str:
     keys = data[0].keys()
     column_widths = [max(len(key), max(len(str(row[key])) for row in data)) for key in keys]
@@ -95,7 +91,13 @@ def render(self, data: list[dict[str, str]]) -> str:
             return render_table_dict(data)
 
 
-def compare(previous_path: str, current_path: str, threshold: float) -> None:
+def compare(
+    previous_path: str,
+    current_path: str,
+    threshold_pct: float,
+    before_header: str,
+    after_header: str,
+) -> None:
     previous = json.loads(Path(previous_path).read_text())
     current = json.loads(Path(current_path).read_text())
 
@@ -109,28 +111,27 @@ def compare(previous_path: str, current_path: str, threshold: float) -> None:
             entries[name] = {}
         entries[name]["previous"] = entry
 
-    headers = ["Name", "Previous", "Current", "Change"]
+    headers = ["Name", before_header, after_header, "Change"]
     rows: list[tuple[str, str, str, str]] = []
     for name, entry in entries.items():
         if "previous" in entry and "current" in entry:
-            previous = float(entry["previous"]["value"]) * DIVISORS[entry["previous"]["unit"]]
-            current = float(entry["current"]["value"]) * DIVISORS[entry["current"]["unit"]]
-
-            min_change = previous * (threshold / 100)
-
-            unit = get_unit(min(previous, current))
+            previous_bytes = float(entry["previous"]["value"]) * DIVISORS[entry["previous"]["unit"]]
+            current_bytes = float(entry["current"]["value"]) * DIVISORS[entry["current"]["unit"]]
+            unit = get_unit(min(previous_bytes, current_bytes))
             div = get_divisor(unit)
 
-            change = ((previous / current) * 100) - 100
-            sign = "+" if change > 0 else ""
-
-            if abs(current - previous) >= min_change:
+            abs_diff_bytes = abs(current_bytes - previous_bytes)
+            min_diff_bytes = previous_bytes * (threshold_pct / 100)
+            if abs_diff_bytes >= min_diff_bytes:
+                previous = previous_bytes / div
+                current = current_bytes / div
+                change_pct = ((current_bytes - previous_bytes) / previous_bytes) * 100
                 rows.append(
                     (
                         name,
-                        f"{cell(previous, div)} {unit}",
-                        f"{cell(current, div)} {unit}",
-                        f"{sign}{change:.2f}%",
+                        f"{previous:.2f} {unit}",
+                        f"{current:.2f} {unit}",
+                        f"{change_pct:+.2f}%",
                     )
                 )
         elif "current" in entry:
@@ -171,6 +172,11 @@ def measure(files: list[str], format: Format) -> None:
     sys.stdout.flush()
 
 
+def percentage(value: str) -> int:
+    value = value.replace("%", "")
+    return int(value)
+
+
 def main() -> None:
     parser = argparse.ArgumentParser(description="Generate a PR summary page")
 
@@ -181,11 +187,25 @@ def main() -> None:
     compare_parser.add_argument("after", type=str, help="Current result .json file")
     compare_parser.add_argument(
         "--threshold",
-        type=float,
+        type=percentage,
         required=False,
         default=20,
         help="Only print row if value is N%% larger or smaller",
     )
+    compare_parser.add_argument(
+        "--before-header",
+        type=str,
+        required=False,
+        default="Before",
+        help="Header for before column",
+    )
+    compare_parser.add_argument(
+        "--after-header",
+        type=str,
+        required=False,
+        default="After",
+        help="Header for after column",
+    )
 
     measure_parser = cmds_parser.add_parser("measure", help="Measure sizes")
     measure_parser.add_argument(
@@ -200,7 +220,13 @@ def main() -> None:
     args = parser.parse_args()
 
     if args.cmd == "compare":
-        compare(args.before, args.after, args.threshold)
+        compare(
+            args.before,
+            args.after,
+            args.threshold,
+            args.before_header,
+            args.after_header,
+        )
     elif args.cmd == "measure":
         measure(args.files, args.format)