Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 397593007: Handle shared memory symbols better in the binarysize tool. (Closed)

Created:
6 years, 5 months ago by Daniel Bratell
Modified:
5 years, 10 months ago
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Handle shared memory symbols better in the binarysize tool. The linker can let two symbols share the same memory space and then it is wrong to count that memory space twice. Better to let each symbol contribute with a proportional part of that symbol. This only affects the explain_binary_size_delta program. The graphical treeview will still use the full symbol size since it's valuable information in that context. BUG= Committed: https://crrev.com/5ec91b1d40fb6174d43d5f5cc84209b9a4505b6b Cr-Commit-Position: refs/heads/master@{#316844}

Patch Set 1 #

Patch Set 2 : Shared symbols: Rebased to newer master. #

Total comments: 10

Patch Set 3 : Refactored to show "shared" in the end results. #

Total comments: 1

Patch Set 4 : Removed extra output in unittest. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+526 lines, -137 lines) Patch
M tools/binary_size/binary_size_utils.py View 2 chunks +4 lines, -4 lines 0 comments Download
M tools/binary_size/explain_binary_size_delta.py View 1 2 12 chunks +178 lines, -56 lines 0 comments Download
M tools/binary_size/explain_binary_size_delta_unittest.py View 1 2 3 4 chunks +342 lines, -75 lines 0 comments Download
M tools/binary_size/run_binary_size_analysis.py View 1 2 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 12 (2 generated)
Daniel Bratell
I was a bit worried that I had made some bad decisions because of this ...
6 years, 5 months ago (2014-07-21 15:01:12 UTC) #1
Daniel Bratell
andrewhayden, can you take a look at this? Without it there is a risk of ...
6 years, 3 months ago (2014-09-23 15:35:31 UTC) #2
Daniel Bratell
ping
5 years, 10 months ago (2015-02-16 12:17:49 UTC) #3
Andrew Hayden (chromium.org)
Sorry for the delay. This looks good overall, my only potential issue is with the ...
5 years, 10 months ago (2015-02-16 12:47:38 UTC) #4
Daniel Bratell
A bit tricky to get the information that sizes were adjusted to compensate for sharing ...
5 years, 10 months ago (2015-02-18 13:31:53 UTC) #5
Andrew Hayden (chromium.org)
LGTM (looks great to me). Thanks very much for this contribution! https://codereview.chromium.org/397593007/diff/40001/tools/binary_size/explain_binary_size_delta_unittest.py File tools/binary_size/explain_binary_size_delta_unittest.py (right): ...
5 years, 10 months ago (2015-02-18 13:48:24 UTC) #6
Daniel Bratell
> I have no idea why the first "All tests passed" got propagated into the ...
5 years, 10 months ago (2015-02-18 13:57:03 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/397593007/60001
5 years, 10 months ago (2015-02-18 16:40:34 UTC) #10
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 10 months ago (2015-02-18 16:46:27 UTC) #11
commit-bot: I haz the power
5 years, 10 months ago (2015-02-18 16:47:32 UTC) #12
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/5ec91b1d40fb6174d43d5f5cc84209b9a4505b6b
Cr-Commit-Position: refs/heads/master@{#316844}

Powered by Google App Engine
This is Rietveld 408576698