ShowGlobals tool to dump large/repeated Win32 globals

ShowGlobals tool to dump large/repeated Win32 globals

When investigating binary size regressions or just looking for size
reduction opportunities it can be helpful to examine global variables.
The most interesting ones are those that are repeated (due to abuse of
const or static in header files) or those that are very large. This tool
uses DIA2 to analyze a PDB and print the interesting global variables.

The configuration options for what is large enough to count as
interesting are in the code - making them command-line arguments is left
as an exercise for some future developer.

This is not part of the regular Chromium build. Project files are
included for building with Visual Studio.

BUG=630755
Committed: https://crrev.com/06d14dce6d5cb0b4ba2df631c5247c5c85047dd8
Cr-Commit-Position: refs/heads/master@{#439981}

[brucedawson, 2016-12-15 22:47:01]

brucedawson@chromium.org changed reviewers:
+ chengx@chromium.org, stanisc@chromium.org

[brucedawson, 2016-12-15 22:47:02]

PTAL at the source for this new tool. This is only intended for use when
investigating binary sizes, but should still follow the Chromium coding
standard.

Stanisc for the official review, chengx for practice reviewing code.

[brucedawson, 2016-12-19 18:31:05]

Ping - PTAL.

[chengx, 2016-12-19 21:48:03]

LGTM with a few comments.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
File tools/win/ShowGlobals/ShowGlobals.cc (right):

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:106: IDiaSymbol* type_symbol;
I think type_symbol needs to be freed to avoid memory leak?

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:119:
symbols.push_back(SymbolData((DWORD)size, section, name));
Not very familiar with BSTR. But no conversion needed between BSTR and
std::wstring which is the type of name in SymbolData?

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:155: std::reverse(repeats.begin(),
repeats.end());
Another way is to define a comparison function using the operator ("<")
overloading you defined for RepeatData, and use it in std::sort(). In this way,
std::reverse is no longer needed.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:156: for (auto& repeat : repeats) {
Is "auto const &" better?

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:166: std::reverse(symbols.begin(),
symbols.end());
Same as line 155, reverse operation can be avoided.

[stanisc, 2016-12-19 22:11:40]

lgtm with a few nits / questions

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
File tools/win/ShowGlobals/ShowGlobals.cc (right):

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:64: }
Maybe skip braces to make this similar to NameCompare

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:115: section = -2;
Should this need (DWORD) cast as well?

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:155: std::reverse(repeats.begin(),
repeats.end());
nit: You could avoid reversing by changing the comparison operator.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:159: wprintf(L"%d\t%u\t0\t0\t%s\t%s\n",
repeat.repeat_count, repeat.bytes_wasted,
nit: I think this might not align with the header. It depends on whether symbol
names take between 8 and 16 characters.
"#Dups\tDupSize\tSize\tSection\tSymbol name\tPDB name\n"

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:166: std::reverse(symbols.begin(),
symbols.end());
The same as above.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:167: for (auto p = symbols.begin(); p !=
symbols.end(); ++p) {
Could use for (auto& p : symbols) cycle

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:248: Cleanup(session, global);
What about releasing the source?

[brucedawson, 2016-12-19 23:00:51]

Thanks for all the feedback.

I changed the code to get rid of all of the raw pointers, which fixes a bunch of
resource leaks.

Aside: DIA2, and COM in general, works very clumsily with smart pointers. That
is disappointing.

I also cleaned up some types to avoid casts and made some other tweaks. I tested
a /analyze build and an x64 build and they both build without warnings.

PTAL

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
File tools/win/ShowGlobals/ShowGlobals.cc (right):

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:64: }
On 2016/12/19 22:11:40, stanisc wrote:
> Maybe skip braces to make this similar to NameCompare

Done.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:106: IDiaSymbol* type_symbol;
On 2016/12/19 21:48:02, chengx wrote:
> I think type_symbol needs to be freed to avoid memory leak?

Yep. The original sample code used manual calls to Release. I've updated my code
to use CComPtr and CComBSTR, although paradoxically the DIA2 APIs handle this
poorly.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:115: section = -2;
On 2016/12/19 22:11:40, stanisc wrote:
> Should this need (DWORD) cast as well?

Done. And changed to static_cast.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:119:
symbols.push_back(SymbolData((DWORD)size, section, name));
On 2016/12/19 21:48:02, chengx wrote:
> Not very familiar with BSTR. But no conversion needed between BSTR and
> std::wstring which is the type of name in SymbolData?

BSTR is basically a null-terminated string with weird memory management
semantics, so std::wstring sees it as wchar_t*.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:155: std::reverse(repeats.begin(),
repeats.end());
On 2016/12/19 21:48:02, chengx wrote:
> Another way is to define a comparison function using the operator ("<")
> overloading you defined for RepeatData, and use it in std::sort(). In this
way,
> std::reverse is no longer needed. 

I had that originally for RepeatData and decided I didn't like having an
operator< that was, technically, backwards.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:156: for (auto& repeat : repeats) {
On 2016/12/19 21:48:02, chengx wrote:
> Is "auto const &" better?  

Yes. const all the things.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:159: wprintf(L"%d\t%u\t0\t0\t%s\t%s\n",
repeat.repeat_count, repeat.bytes_wasted,
On 2016/12/19 22:11:40, stanisc wrote:
> nit: I think this might not align with the header. It depends on whether
symbol
> names take between 8 and 16 characters.
> "#Dups\tDupSize\tSize\tSection\tSymbol name\tPDB name\n"

Yeah, PDB name doesn't align. But it generally doesn't matter.

I did tweak the numeric alignment slightly.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:167: for (auto p = symbols.begin(); p !=
symbols.end(); ++p) {
On 2016/12/19 22:11:40, stanisc wrote:
> Could use for (auto& p : symbols) cycle

Done.

https://codereview.chromium.org/2580833003/diff/20001/tools/win/ShowGlobals/S...
tools/win/ShowGlobals/ShowGlobals.cc:248: Cleanup(session, global);
On 2016/12/19 22:11:40, stanisc wrote:
> What about releasing the source?

Sample code didn't, but I do now.

[stanisc, 2016-12-21 01:26:02]

lgtm

lgtm

[brucedawson, 2016-12-21 02:08:01]

The CQ bit was checked by brucedawson@chromium.org

[brucedawson, 2016-12-21 02:08:02]

The patchset sent to the CQ was uploaded after l-g-t-m from chengx@chromium.org
Link to the patchset: https://codereview.chromium.org/2580833003/#ps60001
(title: "More code review tweaks")

[commit-bot: I haz the power, 2016-12-21 02:08:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 02:19:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482286081929330,
"parent_rev": "4d4e7b833dd0e1b0c1414a45c8e95001d1e19f00", "commit_rev":
"2100497f610b8f549ded88fc521bb77867029b97"}

[commit-bot: I haz the power, 2016-12-21 02:19:40]

Description was changed from

==========
ShowGlobals tool to dump large/repeated Win32 globals

When investigating binary size regressions or just looking for size
reduction opportunities it can be helpful to examine global variables.
The most interesting ones are those that are repeated (due to abuse of
const or static in header files) or those that are very large. This tool
uses DIA2 to analyze a PDB and print the interesting global variables.

The configuration options for what is large enough to count as
interesting are in the code - making them command-line arguments is left
as an exercise for some future developer.

This is not part of the regular Chromium build. Project files are
included for building with Visual Studio.

BUG=630755
==========

to

==========
ShowGlobals tool to dump large/repeated Win32 globals

When investigating binary size regressions or just looking for size
reduction opportunities it can be helpful to examine global variables.
The most interesting ones are those that are repeated (due to abuse of
const or static in header files) or those that are very large. This tool
uses DIA2 to analyze a PDB and print the interesting global variables.

The configuration options for what is large enough to count as
interesting are in the code - making them command-line arguments is left
as an exercise for some future developer.

This is not part of the regular Chromium build. Project files are
included for building with Visual Studio.

BUG=630755

Review-Url: https://codereview.chromium.org/2580833003
==========

[commit-bot: I haz the power, 2016-12-21 02:19:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-21 02:21:01]

Description was changed from

==========
ShowGlobals tool to dump large/repeated Win32 globals

When investigating binary size regressions or just looking for size
reduction opportunities it can be helpful to examine global variables.
The most interesting ones are those that are repeated (due to abuse of
const or static in header files) or those that are very large. This tool
uses DIA2 to analyze a PDB and print the interesting global variables.

The configuration options for what is large enough to count as
interesting are in the code - making them command-line arguments is left
as an exercise for some future developer.

This is not part of the regular Chromium build. Project files are
included for building with Visual Studio.

BUG=630755

Review-Url: https://codereview.chromium.org/2580833003
==========

to

==========
ShowGlobals tool to dump large/repeated Win32 globals

When investigating binary size regressions or just looking for size
reduction opportunities it can be helpful to examine global variables.
The most interesting ones are those that are repeated (due to abuse of
const or static in header files) or those that are very large. This tool
uses DIA2 to analyze a PDB and print the interesting global variables.

The configuration options for what is large enough to count as
interesting are in the code - making them command-line arguments is left
as an exercise for some future developer.

This is not part of the regular Chromium build. Project files are
included for building with Visual Studio.

BUG=630755

Committed: https://crrev.com/06d14dce6d5cb0b4ba2df631c5247c5c85047dd8
Cr-Commit-Position: refs/heads/master@{#439981}
==========

[commit-bot: I haz the power, 2016-12-21 02:21:02]

Patchset 4 (id:??) landed as
https://crrev.com/06d14dce6d5cb0b4ba2df631c5247c5c85047dd8
Cr-Commit-Position: refs/heads/master@{#439981}