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

Tools: Differential Memap #7590

Merged
merged 6 commits into from Aug 15, 2018

Conversation

Projects
None yet
6 participants
@theotherjimmy
Contributor

theotherjimmy commented Jul 24, 2018

Description

This PR adds a new feature to our memory analysis package: Differental
Analysis. With this new feature, every build of an Mbed OS project will
print out a comparison between the previous build, or a fixed build, and
the current build. Use the environment vairable MBED_COMPARE_FIXED
to use the prior build as the fixed comparison point.

Example

In this example, I added a printf to blinky as follows:

diff --git a/main.cpp b/main.cpp
index 2df9a05..e292ab4 100644
--- a/main.cpp
+++ b/main.cpp
@@ -5,6 +5,7 @@ DigitalOut led1(LED1);
 // main() runs in its own thread in the OS
 int main() {
     while (true) {
+        printf("foo");
         led1 = !led1;
         wait(0.5);
     }

Which results in the following table (stats depth at the defualt of 2):

Building project blinky (K64F, GCC_ARM)
Scan: blinky
Scan: env
Compile [100.0%]: main.cpp
Link: blinky
Elf2Bin: blinky
+------------------+--------------+----------+----------+
| Module           |        .text |    .data |     .bss |
+------------------+--------------+----------+----------+
| [fill]           |      103(-8) |    4(+0) | 2464(+0) |
| [lib]/c.a        | 31623(+7074) | 2472(+0) |   89(+0) |
| [lib]/gcc.a      |     3112(+0) |    0(+0) |    0(+0) |
| [lib]/misc       |      204(+0) |    4(+0) |   28(+0) |
| [lib]/nosys.a    |       32(+0) |    0(+0) |    0(+0) |
| main.o           |      72(+16) |    0(+0) |    4(+0) |
| mbed-os/drivers  |       56(+0) |    0(+0) |    0(+0) |
| mbed-os/features |       42(+0) |    0(+0) |  184(+0) |
| mbed-os/hal      |     1725(+0) |    4(+0) |   68(+0) |
| mbed-os/platform |     2673(+0) |  260(+0) |  133(+0) |
| mbed-os/rtos     |     9290(+0) |  168(+0) | 6073(+0) |
| mbed-os/targets  |     8479(+0) |   12(+0) |  381(+0) |
| Subtotals        | 57411(+7082) | 2924(+0) | 9424(+0) |
+------------------+--------------+----------+----------+
Total Static RAM memory (data + bss): 12348(+0) bytes
Total Flash memory (text + data): 60335(+7082) bytes

Printf costs an extra 7074 bytes! and the call costs 16 bytes.

The following is a link to the flame graph generated by the same build
and colorized based on the delta:
http://mbed-os.s3-eu-west-1.amazonaws.com/builds/diff-memap-demo.html

The colorization scheme is:

  • light yellow: No change
  • Dark red: +100% of the current code size
  • Vibrant blue: -99.99% of the current code size (-100% would not show
    up, as we don't show modules with size 0)

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[x] Feature
[ ] Breaking change
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 25, 2018

So if I understand correctly, is the following flow the correct way to enable/use this?

export MBED_COMPARE_FIXED=1
mbed compile. ...
unset MBED_COMPARE_FIXED
mbed compile ...
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 25, 2018

@screamerbg @MarceloSalazar This would change the default output. Thoughts?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 25, 2018

@cmonr No, It's always enabled. that's how you fix a particular mbed compile as the comparison point.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 25, 2018

@cmonr That's why it's 5.10, yeah?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 25, 2018

@theotherjimmy Correct, that's the only reason for marking it for 5.10.
Users would immediately see a change in output, without an option to disable the change.

The story would change if the output were unmodified unless compiling with some option/flag/indicator/config option of some sort that was explicitly enabled by the user.

Remove file before moving over it
Windows is dumb sometimes

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:differential-memap branch from f83b449 to fc97d77 Jul 27, 2018

@cmonr

cmonr approved these changes Jul 30, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 14, 2018

Considering that no objection has been brought up in two weeks, going to progress this PR.

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Aug 14, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Aug 14, 2018

Build : SUCCESS

Build number : 2790
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7590/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Aug 14, 2018

@cmonr cmonr merged commit 90163bb into ARMmbed:master Aug 15, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed code size change RAM(+0.049%) ROM(+0.00)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 793 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9573 cycles (-97 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Aug 16, 2018

Would it be possible to add a command line option or even a configuration flag how to permanently disable this feature? This diff feature may be really useful for some or even most users, but it also harms us who used to copy&paste the build output to spreadsheet for further analysis or did simple text file comparisons between various builds' output captures.

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 6, 2018

@TeroJaasko Sorry for initially missing your comment. For reference, once a PR goes in, our visibility on updates to the PR is decreased.

In general we recommend opening a GitHub issue since those are sync'd to Jira and are triaged just about daily.

As for the option, as I understand the feature, it's not used unless the user explicitly sets an environment variable.

@vidavidorra

This comment has been minimized.

Contributor

vidavidorra commented Sep 27, 2018

@theotherjimmy @cmonr When I run the tool standalone it renames the input file (see here). Is that correct behaviour? I'd expect it would copy the file so when I run it for a 2nd time it shows (+0) everywhere since both versions are the same.
When a new compile is made, the .map file is overwritten and when you run the tol it will again show the difference and update the file.
IMHO that would be more logic behaviour and allow other tools to use the .map file as well as they might not expect it to be called .map.old.
Depending on your thoughts I can create a new ticket (or PR) for it, but I thought to post this here to keep the conversation in this thread.

@vidavidorra

This comment has been minimized.

Contributor

vidavidorra commented Oct 2, 2018

Just tested, what I described above (copy rather than move) can be accomplished by removing the rename import and adding from shutil import copyfile and replacing rename(mapfile, old_mapfile) with copyfile(mapfile, old_mapfile)

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 8, 2018

Sorry for initially missing your comment. For reference, once a PR goes in, our visibility on updates to the PR is decreased.

@vidavidorra Please open an issue referring to this PR. Once a PR is merged, our visibility on subsequent conversations with it are significantly hampered.

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