-
Notifications
You must be signed in to change notification settings - Fork 55
Add size reports #321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add size reports #321
Conversation
a6b43bc
to
8aba70e
Compare
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 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:
... 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 === Regarding MCU support (where BSS and code bloat matters the most):
In other words, it does not support each and every MCU under the world with MCU-specific code. The minimum traits that do require an implementation are the This example we could then initially compile for x86 STD and then later (and to get more "real" figures) - against a few ==== Of course this way we cannot measure the "real" bloat, in that - by the That we could solve downstream in Yes, our "mini" bloat-revealing no_std binary would still tell us whether the core |
Looks like there is an "easy" GH action for 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? |
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:
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 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 |
What shall I do once I checkout this PR as a local branch, so that I can see the size reports? |
The PR also fails CI. |
@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 |
…ng to use them from a submodule
6dd2d00
to
eaee4c7
Compare
bc01cf5
to
d0136b2
Compare
There was a problem hiding this 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. :)
Confirmed working @ #329 |
@ivmarkov — The main reason we didn't just use 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 |
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 PRthen
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 commentPer @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."