Skip to content

Conversation

gmarcosb
Copy link
Contributor

@gmarcosb gmarcosb commented Oct 7, 2025

Fixes #228

Size reporting code copied from matter C++ sdk: https://github.com/project-chip/connectedhomeip/tree/75286cab81f539d3b1072488b55020c968c438bb/scripts/tools/memory

NOTE: This PR, being the first, won't get a size comment; all PRs after this one should (the bloat_check workflow runs for all pending PRs)

size-check produces the artifacts; it actually runs bloaty & outputs the size info to artifacts, as an action on your PR
then

bloat-check runs as a cron, across the whole repo, every 5 minutes; it loads all PRs, checks their artifacts, and anything which has an artifact (produced by size-check) then gets a comment

Per @kpschoedel:

"I think it may have been the cost of reading all the artifacts that made us not do it at the end of every CI build."

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2025

CLA assistant check
All committers have signed the CLA.

@ivmarkov
Copy link
Contributor

ivmarkov commented Oct 8, 2025

I'm not sure I'm sold on having the whole C++ repo as a GIT submodule of our own repo.

It just doesn't sound right, given that - other than for running the YAML / Python tests (and even then - see #298) - we don't have a use for it.

What the cargo xtask itest task does is to git-checkout - on the fly - a shallow copy of the C++ repo (basically the "existenz" minim so that it could build chiptool for linux and then it only needs the scripts/ and tests/ dir).

I guess I need to examine in details what extras can the C++ repo provide us for the "bloat" check - if any - see the MCU topic treatment below, but I was rather thinking we can do something much more lightweight, which relies only on Rust (& LLVM) tooling.

For example:

  • cargo-binutils would give us the overview for the size of each .elf section
  • cargo-bloat would/can give us further insights into the .text section (i.e. code-size bloat not caused by .rodata)

... and then we can use any diff tool to figure out where, and how much % of growth we have in each section.

Also, ideally we should have something pure-Rust, like - say - a cargo xtask diff-bloat sub-task that can (a) output a bloat summary for an executable and (b) compare the bloat summaries of two executables against each other. Or something like that.

===

Regarding MCU support (where BSS and code bloat matters the most):

rs-matter is also - philosophy-wise - a very different animal from the C++ SDK with regards to "platform support".

In other words, it does not support each and every MCU under the world with MCU-specific code.
Rather, it can be compiled (in no_std mode, with most features disabled) into a platform-agnostic library, which - to be used - expects the user to inject implementations for a few traits, which - by necessity - require platform specific code.

The minimum traits that do require an implementation are the NetworkSend/NetworkReceive pair of traits and for these - and for the purposes of measuring bloat - we can just inject a "no op" / fake implementation of those, in an example executable (say - matter_bloat) which is specifically crafted to be used when measuring bloatness.

This example we could then initially compile for x86 STD and then later (and to get more "real" figures) - against a few no_std Rust targets - say - a Cortex one and a Riscv32 one and then measure its bloat.

====

Of course this way we cannot measure the "real" bloat, in that - by the NetworkSend/NetworkReceive traits being fake - we won't "bring-in" the concrete Wifi/Thread/Ethernet and BLE impl for the concrete MCU.

That we could solve downstream in rs-matter-embassy, by copy-pasting the bloat-checking code in there, and running it against the "real" NRF, rPI-Pico (W) and Espressif no_std APIs.

Yes, our "mini" bloat-revealing no_std binary would still tell us whether the core rs-matter code itself did regress or progress in terms of code-size and BSS.

@ivmarkov
Copy link
Contributor

ivmarkov commented Oct 8, 2025

Looks like there is an "easy" GH action for bloaty (which seems to be used by the C++ SDK as well). Here's a sample GH script I was pointed at in the Embassy room: https://github.com/tweedegolf/sequential-storage/blob/master/.github/workflows/ci.yaml#L49-L111

Is there more to it which is available in the C++ SDK or if not - perhaps we can just copy-paste this GH script and adjust to our needs?

@gmarcosb
Copy link
Contributor Author

gmarcosb commented Oct 8, 2025

I agree that just the size reports isn't enough to make the C++ repo a sub-module, but my thought is there are 3 reasons to do it, in order of importance:

  1. The TC tests, which are the same tests run for certification but are also run as CI - they are python and provide an immense amount of coverage for the C++ app which the rust app should also pass
  2. The existing use of chip-tool, which does a custom git dep instead of a standard one
  3. The size tests having a read-out consistent with the C++ sdk

The TC tests as CI are really the strongest reason for us to have the C++ sdk as a submodule; the other 2 simply would piggyback on that

Instead of doing a custom git sync hidden in a cargo, we use the standard git dep infra; (2) is currently using checkout_submodules anyway, so seems we could do that as a standard git pre-step instead of something custom

As for shallow, I hope that the 3 cases above would also mean that we can do a shallow copy of the C++ repo (I don't see a reason why that wouldn't be true, as their deps are no different than chip-tool)

Agreed on platform support, you'll notice that all of the github actions here target linux - in other words, should be exactly the same as xtask is already doing

@ivmarkov
Copy link
Contributor

ivmarkov commented Oct 8, 2025

What shall I do once I checkout this PR as a local branch, so that I can see the size reports?

@ivmarkov
Copy link
Contributor

ivmarkov commented Oct 8, 2025

The PR also fails CI.

@gmarcosb
Copy link
Contributor Author

gmarcosb commented Oct 8, 2025

@ivmarkov yes sorry still a work in progress 🤣 just wanted to be sure the general idea made sense & I'll finish/polish it up & make sure it's running properly

Sadly the only way to test CI that I know of is just to do pushes 🤣 I'll keep the PR in draft until it's actually giving results

@gmarcosb gmarcosb force-pushed the main branch 11 times, most recently from 6dd2d00 to eaee4c7 Compare October 14, 2025 22:56
@gmarcosb gmarcosb force-pushed the main branch 16 times, most recently from bc01cf5 to d0136b2 Compare October 15, 2025 18:22
@gmarcosb gmarcosb marked this pull request as ready for review October 15, 2025 18:32
@gmarcosb
Copy link
Contributor Author

gmarcosb commented Oct 15, 2025

See size report artifact attached to PR:
image

e.g.:

{
  "platform": "linux",
  "config": "rs-matter",
  "target": "onoff-light",
  "time": 1760553373,
  "input": "target/release/onoff_light",
  "by": "section",
  "event": "pull_request",
  "hash": "feb4b07addc0cb1312214830c9354fa5c823eac3",
  "parent": "5576071bdec184730824f889727372eab5bdea27",
  "pr": 321,
  "ref": "refs/pull/321/merge",
  "frames": {
    "section": [{"section":".bss","size":37568},{"section":".data","size":5192},{"section":".rodata","size":166248},{"section":".text","size":1971552}],
    "region": [{"region":"FLASH","size":2137800},{"region":"RAM","size":42760}]}}

Copy link
Contributor

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just one nit w.r.t. the naming of bloat_check.yaml (see below).

Other than that, I don't really understand the difference between bloat_check.yaml and size-check.yaml at all. Also, the Python scripts are quite... intrusive. I though it is bloaty-the-native-executable that does the heavy lifting of parsing an ELF and computing the sizes, but in the Python stuff I see "elftools", "readelf" and whatnot so it is almost as if the Python scripts also inspect the ELF by themselves. For whatever reason?

But: I hope I don't have to understand it in detail as long as it works. :)

@gmarcosb gmarcosb merged commit b4bfd38 into project-chip:main Oct 15, 2025
12 checks passed
@gmarcosb
Copy link
Contributor Author

gmarcosb commented Oct 15, 2025

Confirmed working @ #329
image

@kpschoedel
Copy link

@ivmarkov — The main reason we didn't just use bloaty is that it couldn't readily support all the platforms and C++ toolchains in use. Hence the ‘pluggable backends’ and the platform config files to deal with different segment naming conventions and such.

We also wanted to avoid having developers treat size reports as noise. You may want to change the highlight threshold over time; this is the --report-increases 0.2 in bloat_check.yaml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Implement size (code, rodata and BSS) checks in CI

4 participants