Skip to content

Commit 605b4b6

Browse files
authored
fix(analyzing-weights): bind weight changes to the correct function via -W (#58)
## What & why `analyzing-weights`'s `analyze-weight-diff.py` was mis-attributing weight changes to the wrong functions and silently dropping flags. The parser identifies each change's function from (1) the git `@@` hunk header and (2) `fn NAME(...)` lines in the diff context. For Substrate weight files **both fail**: - Hunk headers show the enclosing `impl ... WeightInfo` block, never a `fn`. - The `fn NAME(...)` signature sits ~5 lines above the first changed line (`// Minimum execution time` / `Weight::from_parts`), outside the default 3-line context — so it usually isn't in the diff at all. With neither available, `current_fn` stops advancing and changes from several consecutive functions collapse into one bucket; `parse_weight_block` then keeps only the last value per variable. **Observed impact** on a full runtime weight regen (moonbeam #3767): - A `per-y 3.9K → 18.8K (+386.9%)` change was reported against `pay_one_collator_reward_best`, but it actually belongs to `delegate_with_auto_compound` (verified in source: `3_861 → 18_800`). - Reported **50** changed functions / 7 Section-3 flags; the real numbers are **~1,040** functions / 22 flags. ## Fix - **Require `git diff -W` (`--function-context`)** so the `fn` signature is always present in the diff. Documented in both the script docstring and `SKILL.md` (step 2), with an explanation of why it's needed. - **Warn loudly** when a bucket merges multiple functions (detected via >1 `Minimum execution time` line per bucket — the tell-tale of a diff produced without `-W`) instead of emitting wrong numbers. ## Verification - Without `-W`: prints the new warning (33 merged buckets on #3767). - With `-W`: no warning, 1,037 functions, correct per-function attribution (spot-checked against source).
1 parent 9543931 commit 605b4b6

2 files changed

Lines changed: 55 additions & 5 deletions

File tree

moonbeam-engineering/skills/analyzing-weights/SKILL.md

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,27 @@ Analyzes weight file changes between the current branch and a base branch to det
2525

2626
### 2) Generate and parse the weight diff
2727

28-
Run the analysis script piping the git diff of weight files:
28+
Run the analysis script piping the git diff of weight files. **Always pass
29+
`-W` (`--function-context`)** — without it, attribution is wrong (see note
30+
below):
2931

3032
```bash
31-
git diff $(git merge-base <base-branch> HEAD)..HEAD -- '*/weights/*' \
33+
git diff -W $(git merge-base <base-branch> HEAD)..HEAD -- '*/weights/*' \
3234
| python3 scripts/analyze-weight-diff.py --threshold 50
3335
```
3436

3537
The script accepts:
3638
- `--file <path>` or stdin (piped diff)
3739
- `--threshold <N>` percentage threshold for flagging (default: 50)
3840

41+
> **Why `-W` is required:** weight files put the `fn NAME(...)` signature ~5
42+
> lines above the first changed line, and git's hunk headers show the enclosing
43+
> `impl ... WeightInfo` block, not the `fn`. With the default 3-line context the
44+
> parser can't see where one function ends and the next begins, so it attributes
45+
> changes to the wrong function and drops some flags entirely. `-W` includes the
46+
> signature lines. The script prints a loud warning if it detects merged
47+
> functions (the tell-tale of a diff generated without `-W`).
48+
3949
### 3) Interpret the report
4050

4151
The script produces 7 sections:

moonbeam-engineering/skills/analyzing-weights/scripts/analyze-weight-diff.py

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,23 @@
22
"""
33
Analyze git diff of Substrate weight files and flag significant changes.
44
5+
IMPORTANT: always generate the diff with `-W` (--function-context).
6+
Weight files place the `fn NAME(...)` signature ~5 lines above the first
7+
changed line, and git's hunk headers show the enclosing `impl ... WeightInfo`
8+
block rather than the `fn`. Without function context the parser cannot tell
9+
where one function ends and the next begins, so changes get attributed to the
10+
wrong function (and some are dropped). `-W` includes the signature lines so
11+
each weight block is bound to the correct function.
12+
513
Usage:
614
# Compare current branch against a base branch:
7-
git diff $(git merge-base <base-branch> HEAD)..HEAD -- '*/weights/*' | python3 scripts/analyze-weight-diff.py
15+
git diff -W $(git merge-base <base-branch> HEAD)..HEAD -- '*/weights/*' | python3 scripts/analyze-weight-diff.py
816
9-
# Or from a saved diff file:
17+
# Or from a saved diff file (must have been produced with `git diff -W`):
1018
python3 scripts/analyze-weight-diff.py --file weight_diff.txt
1119
1220
# Adjust the threshold for flagging changes (default: 50%):
13-
python3 scripts/analyze-weight-diff.py --threshold 30
21+
git diff -W ... | python3 scripts/analyze-weight-diff.py --threshold 30
1422
"""
1523

1624
import argparse
@@ -25,6 +33,10 @@ def parse_weight_block(lines):
2533
"base_ref": 0,
2634
"base_proof": 0,
2735
"min_execution_time": None,
36+
# Number of "Minimum execution time" lines seen in this bucket. >1 means
37+
# several functions were merged into one bucket (insufficient diff
38+
# context) and attribution can no longer be trusted.
39+
"min_exec_count": 0,
2840
"ref_multipliers": {},
2941
"proof_multipliers": {},
3042
"db_reads_base": 0,
@@ -39,6 +51,7 @@ def parse_weight_block(lines):
3951
)
4052
if min_match:
4153
result["min_execution_time"] = int(min_match.group(1).replace("_", ""))
54+
result["min_exec_count"] += 1
4255
continue
4356

4457
# Base Weight::from_parts — first occurrence NOT inside a saturating_add
@@ -239,6 +252,33 @@ def main():
239252
print(f"WEIGHT DIFF ANALYSIS (threshold: {threshold:.0f}%)")
240253
print(sep)
241254

255+
# Guard against insufficient diff context: if any bucket captured more than
256+
# one "Minimum execution time" line, several functions were merged together
257+
# and the diff was almost certainly generated without `-W`. Attribution is
258+
# unreliable in that case, so warn loudly rather than print wrong numbers.
259+
merged = [
260+
c
261+
for c in all_changes
262+
if c["old"]["min_exec_count"] > 1 or c["new"]["min_exec_count"] > 1
263+
]
264+
if merged:
265+
print()
266+
print("!" * 120)
267+
print(
268+
"WARNING: multiple functions were merged into a single block "
269+
f"({len(merged)} affected). The diff was likely produced WITHOUT "
270+
"function context."
271+
)
272+
print(
273+
" Per-function attribution below is UNRELIABLE. Regenerate "
274+
"the diff with `-W`:"
275+
)
276+
print(
277+
" git diff -W $(git merge-base <base> HEAD)..HEAD -- "
278+
"'*/weights/*' | python3 analyze-weight-diff.py"
279+
)
280+
print("!" * 120)
281+
242282
# ------------------------------------------------------------------
243283
# OVERALL STATS
244284
# ------------------------------------------------------------------

0 commit comments

Comments
 (0)