CI: fail on a RAM regression, not only on running out of RAM - #186
Merged
Conversation
The per-codec ceilings were sized as platform budgets - what the target can afford - so they sit two to eight times above what the code actually uses. That catches a catastrophe and nothing smaller. cd_cdlz used 99014 bytes before the in-place CD spread and uses 57905 now; the whole 41% win could be handed back and the job would stay green, because 99014 is still under the 200000 ceiling. So there are two thresholds per codec now, because there are two questions. REGRESSION sits about 10% over what the code uses today and answers "did we just lose ground". PLATFORM stays where it was and answers "does it still fit". Both have to hold; the output names which one failed. The 10% is not a guess. All seven codecs were measured on all three targets: rv32imafc, cortex-m33 and hazard3 agree exactly on five of them and differ by 8 bytes on the other two, so peak heap here is a property of the allocation sizes rather than of the ABI. The headroom is for allocator and toolchain drift, not for design changes - raise one deliberately, in the commit that spends the memory, and say why. Checked against the real qemu logs from all three targets, and against a log edited to put cd_cdlz back at its pre-spread 99014: the job now fails with REGRESSION where it previously passed.
They joined the budget firmware with the STREAMINFO block-size fix and had platform ceilings but no regression threshold, so giving those bytes back would not have failed the job - which is the whole point of this change. Baselines taken on rv32imafc; the three targets agree to within 8 bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01846EhHuAFk5qvxwEA5Gq6y
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.
The per-codec RAM ceilings in the three
check_budget.pyscripts were sized as platform budgets — what the target can afford — so they sit two to eight times above what the code actually uses. That catches a catastrophe and nothing smaller.Concretely:
cd_cdlzused 99014 bytes before the in-place CD spread (#183) and uses 57905 now. The whole 41% win could be handed back tomorrow and the job would stay green, because 99014 is still under the 200000 ceiling.Two thresholds, because there are two questions
REGRESSIONsits about 10% over what the code uses today and answers did we just lose ground.PLATFORMstays exactly where it was and answers does it still fit. Both have to hold, and the output names which one failed:Where the 10% comes from
All seven codecs measured on all three targets, today, on master:
Three different ISAs agree exactly on five codecs and differ by 8 bytes on the other two, so peak heap here is a property of the allocation sizes rather than of the ABI. 10% is therefore generous headroom for allocator and toolchain drift, not a fudge factor.
It is not headroom for design changes. Raising one should happen in the commit that spends the memory, with a reason.
Checked
Against the real qemu logs from all three targets (all pass), against a log edited to put
cd_cdlzback at 99014 (fails withREGRESSIONwhere it previously passed), against an over-platform value, and against a missing codec.