Skip to content
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 Delta Reporting to the Build #541

Closed
gerickson opened this issue Apr 29, 2020 · 19 comments · Fixed by #986
Closed

Add Size Delta Reporting to the Build #541

gerickson opened this issue Apr 29, 2020 · 19 comments · Fixed by #986
Assignees
Labels
build issue enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@gerickson
Copy link
Contributor

Problem

Per the project architectural guidelines and operating principles, fitting on small, resource-constrained devices and scaling up is critical to project success. Without a size-first approach to development, the project will quickly spill over the sides of the resource bucket.

Regular reporting of R/O data, R/W data, and text size deltas needs to be incorporated. In addition, linker checks against one or more supported device SoC/MCU parts needs to be performed as part of CI.

Proposed Solution

The OpenThread project has some inspiration that might be worth borrowing / leveraging:

@gerickson gerickson added enhancement New feature or request help wanted Extra attention is needed build issue labels Apr 29, 2020
@rwalker-apple rwalker-apple added this to the June 12, 2020 milestone Apr 30, 2020
@andy31415
Copy link
Contributor

https://pigweed.googlesource.com/pigweed/pigweed/ project is trying to add piecemeal embedded development things. We could inspire ourselves from some things that are there and specifically for size reporting, they have a pw_bloat bit.

https://pigweed.googlesource.com/pigweed/pigweed/+/refs/heads/master/pw_bloat/docs.rst

I believe their build system is incompatible with CHIP, however the concept and the approach may be used since pigweed tries hard to have each module reasonably independent.

@andy31415
Copy link
Contributor

https://github.com/google/bloaty is the underlying tool that pigweed is using. it seems to need some reference binaries (maybe we could make our CI system build before and after the patch and compare?) and needs python + compilation (cmake).

@bukepo
Copy link
Contributor

bukepo commented May 18, 2020

I can try adding the the size diff tool from OpenThread. It requires installing the size-report app to this project. @andy31415 @gerickson

@bukepo
Copy link
Contributor

bukepo commented May 19, 2020

Could you help install the app? @gerickson

@gerickson
Copy link
Contributor Author

Could you help install the app? @gerickson

@bukepo, which app?

@jwhui
Copy link
Contributor

jwhui commented May 19, 2020

Could you help install the app? @gerickson

@bukepo, which app?

I just requested to install the openthread/size-report app. An admin of the project-chip organization will need to approve the request.

@gerickson
Copy link
Contributor Author

@robszewczyk, can you help @bukepo with permissions for the OT size-report app?

@woody-apple
Copy link
Contributor

woody-apple commented Jun 2, 2020

We'll be meeting at the Software Development Tiger team this Thursday, we should bring this up for discussion there. Agree we need this ASAP, want to understand how things work, and what the processes are for this, how this fits in CI, etc.

@woody-apple
Copy link
Contributor

Looking at this more deeply, we definitely need to drop this in to our larger CI, so we don't duplicate the build matrices for this workflow as well.

@woody-apple woody-apple self-assigned this Jun 2, 2020
@andy31415
Copy link
Contributor

I was going to look at what it takes to have differential bloat reports on a list of apps (e.g. sample apps) to be reported as part of CLs. that way we hopefully see the impact of any code refactors.

@woody-apple
Copy link
Contributor

@andy31415 That sounds right to me, and if we drop it nicely into the build matrices, we can get that for free for all platforms.

@andy31415
Copy link
Contributor

A github integrated tool also seems like very beneficial. Can we have an example output for it to understand how it would look like in chip?

@jwhui
Copy link
Contributor

jwhui commented Jun 2, 2020

For more information on the workings of OpenThread's size-report:

As for OpenThread's review process, there are no hard thresholds. Reviewers are responsible for reviewing the size report and providing their opinions when reviewing the PR.

The size-report app has been a huge value to the OpenThread project. There have been many cases of compilers doing unexpected things.

@andy31415
Copy link
Contributor

It looks to me as if the size report would be faster to integrate than figuring out bloaty and CI.

I would still try the CI bits, but until those are finalized, should we add the size-report tool?

@woody-apple
Copy link
Contributor

woody-apple commented Jun 2, 2020

This is why we need to align on strategy here. You could integrate it, but it wouldn't even run - note that GitHub workflows do not run on PRs from forked repos, for private repositories. (https://help.github.com/en/actions/reference/events-that-trigger-workflows)

@andy31415
Copy link
Contributor

That is unfortunate. So this one would depend on our Repo becoming public? I believe we want to have that at some point, but not sure when that would happen.

@jwhui
Copy link
Contributor

jwhui commented Jun 2, 2020

This is why we need to align on strategy here. You could integrate it, but it wouldn't even run - note that GitHub workflows do not run on PRs from forked repos, for private repositories. (https://help.github.com/en/actions/reference/events-that-trigger-workflows)

The core logic for size reporting is isolated to a separate script.

Should be fairly easy to adapt to other CIs (we had it running in both Travis and GitHub Actions).

@woody-apple
Copy link
Contributor

@jwhui Yup, I dug into the core script, it looks great. Hence let's figure out how to get it into our CI.

@gerickson
Copy link
Contributor Author

@robszewczyk, @mrjerryjohns: is it worth contributing memory metrics to this project?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build issue enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants