Support running memory watch under vista, plus other tweaks...

Support running memory watch under vista, plus other tweaks

This version of memory_watcher can run under Vista, even though the
recursive calls that it handles are appearing often enough that there
is a performance penalty.  With this landed, it may be possible for other
folks to run the tool, and I can work on improving its performance.

This CL also resolves the problem with hanging processes.  Although
memory reporting can only be done once, and it leaves a pile of memory
"hanging around," the browser can be cleanly exited.

Tweaks include outputing the aggregate stacks such that the largest
stacks appear at the start of the output file.

This version avoids ongoing aggregation of stats in favor of only
doing the aggregation at dump-time.  This probably enhances performance
at run-time, although it is hidden (on Vista) by the recursive calling.
This also simplifies the tracking code a fair amount.

There is some evidence that a small number of duplicate calls are being
made to "track" the same memory region, without an intervening free (i.e.,
call to "untrack").  The code to better diagnose this is currently in
place, but ifdef'ed, as it is only useful under a debugger.  Exercise
of this code (turning a stack-frame list into a human readable stack
trace string) currently causes some corruption shortly after it triggers,
so I can't leave it on full time.


r=mbelshe

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31299

[Mike Belshe, 2009-11-06 18:23:01]

thanks - simplifications look good

lgtm

http://codereview.chromium.org/366031/diff/6001/6002
File tools/memory_watcher/call_stack.cc (right):

http://codereview.chromium.org/366031/diff/6001/6002#newcode169
Line 169: if (!dbghelp_loaded_) {
As per discussion, I understand why you're not calling DebugBreak(), but a
comment and maybe a function break-out is better?

http://codereview.chromium.org/366031/diff/6001/6002#newcode250
Line 250: active_thread_id_ = GetCurrentThreadId();
Since this goes hand in hand with the Dbghelp lock (e.g. you should never grab
the dbghelp lock without also setting/unsetting the active_thread_id_), should
there be an explicit LockDbgHelp and UnlockDbgHelp routine?

http://codereview.chromium.org/366031/diff/6001/6003
File tools/memory_watcher/call_stack.h (right):

http://codereview.chromium.org/366031/diff/6001/6003#newcode55
Line 55: PrivateHookAllocator<char> > String;
nit: Should we name this "CustomAllocString"?  String is pretty generic?

http://codereview.chromium.org/366031/diff/6001/6003#newcode95
Line 95: // Record the fact that dbghlep has been loaded.
nit: dbghlep

http://codereview.chromium.org/366031/diff/6001/6003#newcode98
Line 98: // static
nit: remove line 98?

http://codereview.chromium.org/366031/diff/6001/6004
File tools/memory_watcher/dllmain.cc (right):

http://codereview.chromium.org/366031/diff/6001/6004#newcode63
Line 63: ResetEvent(g_dump_event);
Instead of calling ResetEvent, to avoid the loop, just set stopping = true.

http://codereview.chromium.org/366031/diff/6001/6005
File tools/memory_watcher/memory_watcher.cc (right):

http://codereview.chromium.org/366031/diff/6001/6005#newcode114
Line 114: // much.  Uncomment this line if you think you need it.
The "uncomment" part of this comment no longer applies.

http://codereview.chromium.org/366031/diff/6001/6006
File tools/memory_watcher/memory_watcher.h (right):

http://codereview.chromium.org/366031/diff/6001/6006#newcode36
Line 36: typedef std::basic_string<char, std::char_traits<char>,
nit:  Naming should indicate PrivateAllocator?

[jar (doing other things), 2009-11-06 21:16:36]

All changes made per your suggestions.

Jim

http://codereview.chromium.org/366031/diff/6001/6002
File tools/memory_watcher/call_stack.cc (right):

http://codereview.chromium.org/366031/diff/6001/6002#newcode169
Line 169: if (!dbghelp_loaded_) {
On 2009/11/06 18:23:01, Mike Belshe wrote:
> As per discussion, I understand why you're not calling DebugBreak(), but a
> comment and maybe a function break-out is better?

Done.

http://codereview.chromium.org/366031/diff/6001/6002#newcode250
Line 250: active_thread_id_ = GetCurrentThreadId();
On 2009/11/06 18:23:01, Mike Belshe wrote:
> Since this goes hand in hand with the Dbghelp lock (e.g. you should never grab
> the dbghelp lock without also setting/unsetting the active_thread_id_), should
> there be an explicit LockDbgHelp and UnlockDbgHelp routine?

Done.

http://codereview.chromium.org/366031/diff/6001/6003
File tools/memory_watcher/call_stack.h (right):

http://codereview.chromium.org/366031/diff/6001/6003#newcode55
Line 55: PrivateHookAllocator<char> > String;
On 2009/11/06 18:23:01, Mike Belshe wrote:
> nit: Should we name this "CustomAllocString"?  String is pretty generic?

Done.

http://codereview.chromium.org/366031/diff/6001/6003#newcode95
Line 95: // Record the fact that dbghlep has been loaded.
On 2009/11/06 18:23:01, Mike Belshe wrote:
> nit: dbghlep

Done.

http://codereview.chromium.org/366031/diff/6001/6003#newcode98
Line 98: // static
On 2009/11/06 18:23:01, Mike Belshe wrote:
> nit: remove line 98?

Done.

http://codereview.chromium.org/366031/diff/6001/6004
File tools/memory_watcher/dllmain.cc (right):

http://codereview.chromium.org/366031/diff/6001/6004#newcode63
Line 63: ResetEvent(g_dump_event);
On 2009/11/06 18:23:01, Mike Belshe wrote:
> Instead of calling ResetEvent, to avoid the loop, just set stopping = true.

Done.

http://codereview.chromium.org/366031/diff/6001/6005
File tools/memory_watcher/memory_watcher.cc (right):

http://codereview.chromium.org/366031/diff/6001/6005#newcode114
Line 114: // much.  Uncomment this line if you think you need it.
On 2009/11/06 18:23:01, Mike Belshe wrote:
> The "uncomment" part of this comment no longer applies.

Done.

http://codereview.chromium.org/366031/diff/6001/6006
File tools/memory_watcher/memory_watcher.h (right):

http://codereview.chromium.org/366031/diff/6001/6006#newcode36
Line 36: typedef std::basic_string<char, std::char_traits<char>,
On 2009/11/06 18:23:01, Mike Belshe wrote:
> nit:  Naming should indicate PrivateAllocator?

Done.