Skip to content

fix(analyzing-weights): bind weight changes to the correct function via -W#58

Merged
RomarQ merged 1 commit into
mainfrom
manuel/fix-analyze-weight-diff-function-context
May 27, 2026
Merged

fix(analyzing-weights): bind weight changes to the correct function via -W#58
RomarQ merged 1 commit into
mainfrom
manuel/fix-analyze-weight-diff-function-context

Conversation

@manuelmauro

Copy link
Copy Markdown
Contributor

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

…ia -W

The analyze-weight-diff parser identified each change's function from git
hunk headers and the default 3-line context window. For Substrate weight
files both fail: hunk headers show the enclosing `impl ... WeightInfo`
block (never a `fn`), and the `fn NAME(...)` signature sits ~5 lines above
the first changed line, outside the default context. Consecutive functions
then collapse into one bucket, mis-attributing per-variable and min-exec
changes and silently dropping flags (e.g. ~50 functions reported instead
of ~1040 on a full runtime regen).

- Document and require `git diff -W` (--function-context) so the `fn`
  signature is always present in the diff.
- Warn loudly when a bucket merges multiple functions (the tell-tale of a
  diff produced without -W) instead of emitting wrong numbers.
@RomarQ RomarQ merged commit 605b4b6 into main May 27, 2026
4 checks passed
@RomarQ RomarQ deleted the manuel/fix-analyze-weight-diff-function-context branch May 27, 2026 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants