Investigate RAM size increases in example apps

[mlepage-google]

PRs #14680 and #14657 report .bss size increases of a few KB. (Could they be the same?)

Investigate this to see what's causing it and if it can be reduced.

[bzbarsky-apple]

PR #14680 is on top of PR #14657, which is already merged.

PR #14657 having a size increase makes sense: it adds an AccessControl instance that is now hanging around.

PR #14680 having a size increase does NOT make sense: it's just moving the AccessControl instance from one place to another. Why does it have a size increase?

[bzbarsky-apple]

So in particular, #14680 shows a size increase only on some configurations. On esp32 all-clusters-app it does not, as expected.

But some configurations show increases on both. I am looking at (efr32, lighting-app, BRD4161A) for example.

[bzbarsky-apple]

Or another example: (nrfconnect, lighting-app, nrf52840dk_nrf52840).

[bzbarsky-apple]

I just tried doung the nrfconnect lighting-app build locally, like so:

(cd ~/connectedhomeip/examples/lighting-app/nrfconnect && source ~/tools/nrfconnect/zephyr/zephyr-env.sh && west build -b nrf52840dk_nrf52840 -- -DPYTHON_PREFER=python3.9)

and it does in fact show a size increase for me from #14680 as measured with bloaty for examples/lighting-app/nrfconnect/build/zephyr/zephyr.elf (but not for examples/lighting-app/nrfconnect/build/app/libapp.a ?).

The build output also claims this size increase: before #14680 it shows:

            SRAM:      191099 B       256 KB     72.90%

and after it shows:

            SRAM:      192155 B       256 KB     73.30%

[bzbarsky-apple]

zephyr.map file before #14680 and
zephyr.map file after #14680

@Damian-Nordic @kkasperczyk-no any suggestions on pinning down the size increase for specific symbols? Are there tools for comparing the two binaries (bloaty does not do a great job with pinning down which symbols contribute to .bss) or the .map files?

[harimau-qirex]

There's scripts/tools/memory/diffsyms.py in the repository. When I checked efr32's lighting app, I found that the size increases were from the ACL symbols. My guess is that they were being optimised out in previous builds because they weren't referenced.

[bzbarsky-apple]

But #14680 does not change which symbols are referenced....

[harimau-qirex]

It adds a reference from Server to the ACL system and calls AccessControl::Init after setting an instance of AccessControlExampleDelegate. The symbols I see being added with that change are the ones in the anonymous namespace in src/access/examples/ExampleAccessControlDelegate.cpp, so presumably they were being removed since the default AccessControl implementation doesn't use that delegate implementation and nothing else references it.

[bzbarsky-apple]

and calls AccessControl::Init after setting an instance of AccessControlExampleDelegate.

Yes, but that all used to happen already under MatterAccessControlPluginServerInitCallback. That init just got moved....

[bzbarsky-apple]

And just to be clear, we are talking about .bss here, not .text, right?

[harimau-qirex]

This is both .bss and .text. Here are the outputs I obtained with diffsyms.py, restricted to those two sections in the EFR32 lighting example:
diffsyms-bss.txt
diffsyms-text.txt

[bzbarsky-apple]

Hmm. So why is the AccessControl gAccessControl(Examples::GetAccessControlDelegate()); in access-control-server.cpp not pulling that in before PR 14680?

[bzbarsky-apple]

So specifically, what is the breakdown of the .bss increase on the EFR32 lighting app from PR #14657? @harimau-qirex

[harimau-qirex]

Here's the .bss diff from #14657 for the EFR32 lighting app. The majority of the increase is from the example ACL delegate's implementation. I'm still investigating why the symbols weren't included in previous builds.
diffsyms-bss-14657.txt

[mlepage-google]

All examples should have the access control module (AccessControl.cpp) as part of the core, and called by IM, it should be linked in. But without the "real" implementation, it's just a relatively thin API and scaffolding.

The cluster implementation adds more code (cluster server, plus whatever generated serialization code it pulls in) and also configures the example "real" access control implementation, which has more code, plus the storage space for storing ACLs.

I can't say for sure, but I expect that access-control-server.cpp was not built by all the example apps. I only added it to all-clusters-app (in late Nov or early Dec), and I think maybe at some point it might have got added to one or two more examples, but I did not add it to all of them (hence issue #14461).

[bzbarsky-apple]

but I expect that access-control-server.cpp was not built by all the example apps

It was built by the ones we are looking at here....

[harimau-qirex]

Re-reading through the comments here, I'm a bit confused.

At the time PR #14680 was written, it was NOT based on PR #14657. The size comparison bot showed that both PRs increased the size of various apps because they each added changes that resulted in references to the example ACL delegate.

For example, the efr32 lighting app showed an increase in both PRs. There is almost no size difference in this app when built with PR #14657 and when built with both PRs #14657 and #14680. (Tested by syncing to 5db4015 and 62e37a2 and comparing the builds at each point)

[bzbarsky-apple]

Interesting. So is the point that the table in #14680 (comment) is just using the wrong base for the PR, which had been rebased to a different base? @kpschoedel

[kpschoedel]

The base has been a problem before. #11841 —

For a GitHub PR, the actual parent may be different from the commit given by github.event.pull_request.base.sha (see actions/checkout#27). In this case, size reports incorrectly include changes from commit(s) between the purported and actual parent.

Normally when CI runs on a PR the checkout HEAD has a commit message of the form “Merge SHA into BASE”, and we scrape that to get the base commit. If anyone understands GitHub enough to suggest a better way — or a way to detect that the base might be misleading — we can try it.

[harimau-qirex]

So overall this is a non-issue: the size increase in #14680 is because it didn't include #14657 in its base, because they were written independently and uploaded on the same date. #14657 was merged six days later, and #14680 was never rebased before it was merged one day after that. As a result, the size comparison table in #14680 doesn't take into account any increases from #14657.

#14680's size increases are because, without #14657, some examples didn't include access-control-server.cpp in their builds at all. Moving the AccessControl initialisation from access-control-server.cpp to Server.cpp meant adding a new reference to the example ACL delegate for those examples, since it was previously only referenced in access-control-server.cpp.

If #14657 had been merged before #14680 was uploaded, then #14680 wouldn't have shown any size changes (apart from a few bytes from moving the reference around). The general size increase from ACL is already tracked by #14450, so this issue is no longer needed.

[mlepage-google]

Thanks for looking into this.

[bzbarsky-apple]

Yes, @harimau-qirex thank you for figuring out what the story was! It's very much appreciated.