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

Implement zoomable html-flamegraph memap output #6582

Merged
merged 5 commits into from Jun 4, 2018

Conversation

Projects
None yet
9 participants
@theotherjimmy
Contributor

theotherjimmy commented Apr 9, 2018

Description

Sample: http://mbed-os.s3-eu-west-1.amazonaws.com/builds/fat-fs_map.html

Pull request type

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 9, 2018

@sg-

This comment has been minimized.

Member

sg- commented Apr 9, 2018

Would be great if there were percentages attached to each level of zoom. For instance:
feature, rtos, hal, etc. in the context of text or data/bss and when you zoom in on an object and it explodes, the percentage resolves for the new scope.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 10, 2018

@sg- Yeah, I don't know how to do that.

@loverdeg-ep

This comment has been minimized.

loverdeg-ep commented Apr 10, 2018

Would also be nice if it used the linker script to map to the target's address space.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 10, 2018

@sg- I added a bit of text that does the percent thing at the bottom. I updated the example.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 10, 2018

@loverdeg-ep I'm not entirely sure how that would work. If you mean that you want symbols sorted by where they're located in ROM/RAM, I think that has the potential to break up the bars into multiple parts.

This is meant as a visualization of relative allocation sizes, so that you could compare the size of the rtos to printf, for example. This allows you to make decisions about where to spend your time optimizing. Removing the location information is an important step to easily visualize the relative size of the allocations.

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Apr 11, 2018

@theotherjimmy is looking good - thanks!

Can you please elaborate on the meaning of "Non-zero initialized data (ROM + RAM)" in this context?
Is it about bss (not explicitly initialized to any value) or data (initialized to a given value)?
It's not clear how this would benefit users/developers as it's now.

I understand (data) is first stored in ROM, and during run/init time it's copied/de-compressed to RAM. Therefore this is how I'd expect the data to be split:

ROM: text (code+const) + data (compressed)
RAM: bss + data (de-compressed) + ZI

Does it make sense? Please share your thoughts. Thanks!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 11, 2018

Can you please elaborate on the meaning of "Non-zero initialized data (ROM + RAM)" in this context?

That's data. If it were zero, it would be in the bss. From my research:

The .bss section in an ELF file is used for static data which is not initialized programmatically but guaranteed to be set to zero at runtime.

There is a difference between not programatically initialized, and guaranteed to be zero. https://en.wikipedia.org/wiki/.bss

It's not clear how this would benefit users/developers as it's now.

I'm not sure what this statement is referring to. If you're talking about the way that it's split, I go into detail about that below.

ROM: text (code+const) + data (compressed)
RAM: bss + data (de-compressed) + ZI

I tried that data split first, and found that representation to be lacking. When I had the memory split this way my first question was: "what parts are data?" If the intent of this graphic is to help find the source of space in ROM/RAM, then having the data section separate from the text section could help identify where moving the initial state of a structure to be all 0's would help with ROM savings for example.

Tangent: We don't have either the compressed or the de-compressed numbers. Memap seems to assume that the data section is the same size in RAM and ROM.

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Apr 13, 2018

Thanks for the clarifications @theotherjimmy

I see at the bottom of each graph, there is an indication of bss, data and text, which is great.

But I still feel we should improve the wording on the "Non-zero initialized data (ROM + RAM)", as it's a bit ambiguous; maybe use (ROM = RAM) to indicate this data goes to two memories.

Btw in case you haven't seen this, could be useful to get some ideas: https://armmbed.github.io/mbed-os-linker-report

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 16, 2018

@MarceloSalazar I could move them back to .text, .data and .bss. Also, I find the ROM = RAM wording confusing, maybe we should just use english: stored in ROM and RAM? or copied from ROM to RAM on boot?

tree_data = {"name": ".data"}
for name, dct in self.modules.items():
if ".text" not in dct:
continue

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 16, 2018

Contributor

This is straight up wrong. I need to remove this.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 16, 2018

If the intent of this graphic is to help find the source of space in ROM/RAM, then having the data section separate from the text section could help identify where moving the initial state of a structure to be all 0's would help with ROM savings for example.

Well, if that's really the intent, then the current graph provides no benefit: you cannot visually compare the two (the entire point of having a graph). I'm pushing another change that uses a combined graph, but makes ".text" and ".data" children of "ROM" and ".bss" and ".data" children on "RAM". This should allow you to compare these allocations visually.

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Apr 18, 2018

I'm pushing another change that uses a combined graph

Thanks, let me know once that's done and we have a link to a demo.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 18, 2018

@MarceloSalazar It was done when I commented that. You should be able to see it in the demo. Edit: I updated the demo in the link in the top post.

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Apr 18, 2018

Thanks - I quite like the combined image with corresponding sections for each RAM and ROM.
Perhaps add a title for each graph: RAM, ROM

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 18, 2018

Perhaps add a title for each graph: RAM, ROM

Personally I find that redundant: there must be a bottom bar, and it's already labeled ROM or RAM.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Apr 30, 2018

@theotherjimmy what is the current state of this PR now?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 30, 2018

Approval from @sg- @thegecko and @screamerbg is next.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 8, 2018

Approval from @sg- @thegecko and @screamerbg is next.

Ping.

@thegecko

This comment has been minimized.

Member

thegecko commented May 8, 2018

I'm not sure what I'm signing off on here.

From a product PoV, I personally don't believe data formatting and templates (html, etc.) is inline with the remit of an embedded OS codebase. I believe any data being output should be in a raw and consumable format (json, etc.) and users can then create visual representations of it. Without knowledge nor context of this area of functionality I'm unaware whether a raw output may already exist. Inclusion of this feature would be down the to the Mbed OS product manager.

From a testing PoV, what happens if the data format changes? Are there tests which will highlight the html rendering may break?

From a code PoV I'm not a pythonista, so can't really comment, sorry.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 8, 2018

@thegecko

Without knowledge nor context of this area of functionality I'm unaware whether a raw output may already exist.

We already have the data as CSV and JSON for every compile. the JSON format is not particularly usefull for D3. I could instead add a D3 JSON output. That could be an alternative approach.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 31, 2018

Since no one has brought up any major reasons why this shouldn't make it in, I'm progressing the PR.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 31, 2018

Build : SUCCESS

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

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 Jun 1, 2018

@cmonr cmonr merged commit 78d9c4f into ARMmbed:master Jun 4, 2018

12 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
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10231 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Jun 7, 2018

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