fix(analyzing-weights): bind weight changes to the correct function via -W#58
Merged
Merged
Conversation
…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
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
analyzing-weights'sanalyze-weight-diff.pywas 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:impl ... WeightInfoblock, never afn.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_fnstops advancing and changes from several consecutive functions collapse into one bucket;parse_weight_blockthen keeps only the last value per variable.Observed impact on a full runtime weight regen (moonbeam #3767):
per-y 3.9K → 18.8K (+386.9%)change was reported againstpay_one_collator_reward_best, but it actually belongs todelegate_with_auto_compound(verified in source:3_861 → 18_800).Fix
git diff -W(--function-context) so thefnsignature is always present in the diff. Documented in both the script docstring andSKILL.md(step 2), with an explanation of why it's needed.Minimum execution timeline per bucket — the tell-tale of a diff produced without-W) instead of emitting wrong numbers.Verification
-W: prints the new warning (33 merged buckets on #3767).-W: no warning, 1,037 functions, correct per-function attribution (spot-checked against source).