Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15418002:
Record Chrome trace events in tcmalloc heap profiles (Closed)
|
Created:
7 years, 7 months ago by James Cook Modified:
7 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, dmikurube+memory_chromium.org, sail+watch_chromium.org, Jorge Lucangeli Obes, darin (slow to review), brettw Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRecord Chrome trace events in tcmalloc heap profiles
This allows about:tracing to show tcmalloc heap memory allocation over time.
The implementation:
* Adds a "memory" checkbox to about:tracing
* Uses thread-local-storage to store the "stack" of trace events per thread while about:tracing is running.
* Introduces a StackGeneratorFunction callback into tcmalloc, allowing it to call back into Chrome to get a "stack" of trace events.
* Parses the heap profiler output of tcmalloc and converts it to JSON to be displayed by a visualizer in the about:tracing trace viewer.
BUG=243895
TEST=base_unittests TraceMemoryTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213473
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : chrome works #Patch Set 4 : json output, but no green dots on trace #
Total comments: 6
Patch Set 5 : still no dots #
Total comments: 1
Patch Set 6 : dots appear in about:tracing #Patch Set 7 : don't trace the traces #Patch Set 8 : cleanup, run in all renderers #Patch Set 9 : only start heap profiling after tracing starts #Patch Set 10 : rebase #Patch Set 11 : fix crash in GetPseudoStack, rebase #Patch Set 12 : add simple ScopedTraceMemory unit test #Patch Set 13 : increase stack depth, tests need it #Patch Set 14 : fix deep scope nesting #Patch Set 15 : rebase #Patch Set 16 : dump via RepeatingTimer, more tests #Patch Set 17 : rebase, cleanup #
Total comments: 43
Patch Set 18 : rebase #Patch Set 19 : review comments part 1, rename to trace_event_memory #Patch Set 20 : review comments 2 #
Total comments: 7
Patch Set 21 : review comments 3 #
Total comments: 3
Patch Set 22 : review comments 4 #
Total comments: 23
Patch Set 23 : review comments 5 #Patch Set 24 : rebase, sorry #Patch Set 25 : New TLS implementation #Patch Set 26 : . #
Total comments: 16
Patch Set 27 : rebase #Patch Set 28 : jar review comments #Patch Set 29 : move to ChildThread #
Total comments: 1
Patch Set 30 : cleanup forward declarations #
Total comments: 3
Patch Set 31 : rebase #Patch Set 32 : fix comments #Patch Set 33 : fix tests on unsupported platforms, signed/unsigned conversions #Patch Set 34 : rebase, message_loop.h moved #Patch Set 35 : eliminate base to tcmalloc dependency #Patch Set 36 : fix memory leak, don't enable when tracing record dialog first opens #Patch Set 37 : Add system stats tracing to Chrome #Messages
Total messages: 52 (0 generated)
+dsinclair, any idea why the append method isn't being called? James says he verified that the calls before the TRACE_EVENT are being called, but the actual Append method isn't. https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_linux.cc (right): https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_linux.cc:143: bool enabled = TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED("tcmalloc2"); static bool* enabled = ... if (*enabled) { } https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_linux.cc:151: "tcmalloc2", "tcmalloc2::Heap", dump_holder.get(), dump_holder); 3rd arg should be 1
lgtm ; https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_linux.cc (right): https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_linux.cc:148: scoped_ptr<MemoryDumpHolder> dump_holder(new MemoryDumpHolder(dump)); maybe this needs to be scoped_ptr<base::debug::ConvertableToTraceFormat> dump_holder()
oops
https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main_linux.cc (right): https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_linux.cc:143: bool enabled = TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED("tcmalloc2"); On 2013/05/20 23:03:41, nduca wrote: > static bool* enabled = ... > if (*enabled) { > > } bool enabled; TRACE_EVENT_CATEGORY_GROUP_ENABLED("tcmalloc2", &enabled); if (enabled) { } would also work. https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_linux.cc:148: scoped_ptr<MemoryDumpHolder> dump_holder(new MemoryDumpHolder(dump)); On 2013/05/20 23:04:44, nduca wrote: > maybe this needs to be scoped_ptr<base::debug::ConvertableToTraceFormat> > dump_holder() Doesn't have to be, you can use PassAs below. https://codereview.chromium.org/15418002/diff/8001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main_linux.cc:151: "tcmalloc2", "tcmalloc2::Heap", dump_holder.get(), dump_holder); You don't want to do the .get() on dump_holder. That will return the MemoryDumpHolder* which will get interpreted by trace event as the raw pointer type and print the address. You want to do: dump_holder.PassAs<base::debug::ConvertableToTraceFormat>() which will provide a scoped_ptr<ConvertableToTraceFormat> to trace event and call the AppendAsTraceFormat correctly. Is there some way we could detect this in trace event? Figure out that the pointer type you passed inherits from Convertable and print a warning?
AppendAsTraceFormat is getting called now using PassAs. But still no green dots. https://codereview.chromium.org/15418002/diff/21001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main_linux.cc (right): https://codereview.chromium.org/15418002/diff/21001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main_linux.cc:115: LOG(ERROR) << "JAMESDEBUG AppendAsTraceFormat dump len " << strlen(dump_); I'm now seeing this string printed after taking a trace, with a length of about 48000, but still no green dots in the trace viewer. Any ideas?
Pop open devtools once about:tracing has finished loading. Are there any javascript console warnings? Or, is there an import errors button? Maybe throw me the trace as a file
On 2013/05/21 02:13:54, nduca wrote: > Pop open devtools once about:tracing has finished loading. Are there any > javascript console warnings? Or, is there an import errors button? Maybe throw > me the trace as a file Trace sent. I didn't see any JS errors or input errors and my data appears in the trace dump. Weird.
Above problem looks like it was due to a trace viewer issue, it works now.
Nat, PTAL
https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.cc File base/debug/trace_memory.cc (right): https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:16: #if !defined(NO_TCMALLOC) && !defined(OS_NACL) && (defined(OS_LINUX) || defined(OS_ANDROID)) Could this be done through gyp? Have a trace_memory_stub.cc which is the NOTREACHED versions and a trace_memory_tcmalloc.cc which is the real version. Have gyp pick which one we want at build time? (I have no idea if this is possible/sane, heh) https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:62: LazyInstance<ThreadLocalPointer<TraceMemoryStack> >::Leaky trace_memory_stack = Not sure if it's in the styleguide, but in trace_event_impl.cc we named the globals as g_trace_memory_stack. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:77: int GetPseudoStack(void** stack_out) { category_stack_ has const char*, why use void* instead of const char*? https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:118: "memory", TRACE_DISABLED_BY_DEFAULT("memory") ? https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:129: ScopedTraceMemory initialize(TRACE_MEMORY_IGNORE); Wouldn't it be easier to get just get the stack and see if it exists and explicitly initialize here? Seems weird to create the fake event just to init the stack. (Or, change InitTraceMemoryStack() to just return if the stack exists then you can just call it here without doing the get first.) https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:181: TraceMemoryStart(); Should this do a: if (dump_timer_.IsRunning()) return; Just in case the trace-event framework sends enabled twice? https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:203: stack = InitTraceMemoryStack(); If InitTraceMemoryStack() always returns this can just become: TraceMemoryStack* stack = InitTraceMemoryStack(); (Although, maybe then it should be called GetTraceMemoryStack?) https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.h File base/debug/trace_memory.h (right): https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.h... base/debug/trace_memory.h:42: bool IsTimerRunningForTest() const; If this is only for test, can we make it private and friend the test class? https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.h... base/debug/trace_memory.h:62: ScopedTraceMemory(const char* category); The memory for category, does that need to be static or, who owns it? https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.h... base/debug/trace_memory.h:80: // TODO(jamescook): Make it record both category and name. You'll need to be careful when you add category in here as they can be a comma separated list. TRACE_MEMORY("foo,bar,baz", "boop"); https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... base/message_loop/message_loop.cc:466: INTERNAL_TRACE_EVENT_ADD_SCOPED("task", "MessageLoop::RunTask", This makes me sad. If we're going to be using it outside of the trace_event files we should rename it to no longer be INTERNAL_. If this seems like something we'll do often, what about doing a: TRACE_EVENT_NO_MEMORY0,1,2? Then TRACE_EVENT0 just becomes TRACE_MEMORY() \ TRACE_EVENT_NO_MEMORY0 etc. https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... content/browser/browser_main_loop.cc:489: #endif Does this also need to be done in places like the GPU process? https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:341: int printed = snprintf(buffer + written, Would it be possible, instead of converting to JSON format here, returning a simple struct with the data and doing the JSON conversion in the ConvertableToTraceFormat? Doing it here will add a bit of CPU overhead which may skew the tracing profile a bit. (Hm, doesn't look like it without creating a new GetHeapProfile, bummer)
Some comments in third_party/tcmalloc. https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h (right): https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h:72: * applications that exclusively use GetHeapProfile(). This comment looks confusing. The existing profiler assumes that the profiler is turned off if filename_prefix == NULL. The comment should explicitly describe which function is allowed, and which function is disallowed in case of |prefix| == NULL. E.g. "No writing to disk while profiling is started if |prefix| == NULL. GetHeapProfile() and ???() are available, but ???() and ???() cannot be used." (My English statement might be improved, though. ;)) https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:341: int printed = snprintf(buffer + written, On 2013/06/18 15:30:15, dsinclair wrote: > Would it be possible, instead of converting to JSON format here, returning a > simple struct with the data and doing the JSON conversion in the > ConvertableToTraceFormat? Doing it here will add a bit of CPU overhead which > may skew the tracing profile a bit. > > (Hm, doesn't look like it without creating a new GetHeapProfile, bummer) Agreed with dsinclair. Would it be possible to keep a similar format with the original tcmalloc's heap-profiler? It may allow to use some existing tools. If I understand correctly, your pseudo-stacks are recorded in the same way with the existing profiler. We can use the same "unparser" and "filler", can't we? Dumping it in a JSON format is a different issue from your pseudo-stacking. At least, this function should have a different name while I don't believe we need it. Changes in these tcmalloc files should be requisite minimum since it is third_party. Changes make it harder to roll up. https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profiler.cc:389: int skip_count) { It's almost the same with RecordAlloc(). Why do we need this additional function? Also, don't we need RecordFree? https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profiler.cc:425: void PseudoStackNewHook(const void* ptr, size_t size) { ditto. https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profiler.cc:573: filename_prefix = reinterpret_cast<char*>(ProfilerMalloc(prefix_length + 1)); Over 80? https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profiler.cc:675: extern "C" void SetPseudoStackGenerator(PseudoStackGenerator callback) { The function name should contain HeapProfiler or something like that since the function is in the global namespace. Also, what happens when this function is called after HeapProfilerStart()? What happens when this function is called twice? I think we should hide it, and have HeapProfilerStartWithPseudoStack() or something like that. Wdyt?
Thanks for the feedback, guys! I'm busy with some last minute R29 patches but should have a new version up in a day or two.
<3 this patch https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... base/debug/trace_event.h:231: TRACE_MEMORY(category_group, name) \ should we prefix these defines with INTERNAL_TRACE_MEMORy? The intent, right, is that people wont do these calls themselves... i think? https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... base/debug/trace_event.h:316: #define TRACE_EVENT_BEGIN0(category_group, name) \ these need to go into memory too... https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... base/debug/trace_event.h:360: #define TRACE_EVENT_END0(category_group, name) \ so do these https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... base/debug/trace_event.h:390: name, id, thread_id, timestamp) \ and these https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... base/message_loop/message_loop.cc:462: // was posted is much more useful than "MessageLoop::RunTask" so we invoke lets pull this out to another followup patch.. its a good one that merits some focused thinking, hence the proposal to pull out. https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... content/browser/browser_main_loop.cc:485: // TODO(jamescook): Support all tcmalloc platforms, including Windows. How about always creating it, but have the observe do nothing if its not supported? Then the goup about supported only goes inisde the observer...
On 2013/06/20 20:09:29, nduca wrote: > <3 this patch > > https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h > File base/debug/trace_event.h (right): > > https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... > base/debug/trace_event.h:231: TRACE_MEMORY(category_group, name) \ > should we prefix these defines with INTERNAL_TRACE_MEMORy? The intent, right, is > that people wont do these calls themselves... i think? > > https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... > base/debug/trace_event.h:316: #define TRACE_EVENT_BEGIN0(category_group, name) \ > these need to go into memory too... > > https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... > base/debug/trace_event.h:360: #define TRACE_EVENT_END0(category_group, name) \ > so do these > > https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... > base/debug/trace_event.h:390: name, id, thread_id, timestamp) \ > and these > > https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... > File base/message_loop/message_loop.cc (right): > > https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... > base/message_loop/message_loop.cc:462: // was posted is much more useful than > "MessageLoop::RunTask" so we invoke > lets pull this out to another followup patch.. its a good one that merits some > focused thinking, hence the proposal to pull out. > > https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... > File content/browser/browser_main_loop.cc (right): > > https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... > content/browser/browser_main_loop.cc:485: // TODO(jamescook): Support all > tcmalloc platforms, including Windows. > How about always creating it, but have the observe do nothing if its not > supported? Then the goup about supported only goes inisde the observer... Not sure if it makes sense, but, nduca and I have base/debug/OWNERS for trace_event* files, so, what about renaming trace_memory.{cc|h} to trace_event_memory.{cc|h}? The file is part of the trace event framework, so I think it makes sense?
Guys, please take another look. Apologies for the delay - R29 kept me occupied recently. This version uses tcmalloc's native heap dump format, then parses that in Chrome and converts it to JSON for the tracing system. This decreases the size of the patch against tcmalloc. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... base/debug/trace_event.h:231: TRACE_MEMORY(category_group, name) \ On 2013/06/20 20:09:29, nduca wrote: > should we prefix these defines with INTERNAL_TRACE_MEMORy? The intent, right, is > that people wont do these calls themselves... i think? Done. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_event.h#... base/debug/trace_event.h:316: #define TRACE_EVENT_BEGIN0(category_group, name) \ On 2013/06/20 20:09:29, nduca wrote: > these need to go into memory too... Are we guaranteed that each begin will get an end? If not, the stack accounting code needs to get a lot more clever. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.cc File base/debug/trace_memory.cc (right): https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:16: #if !defined(NO_TCMALLOC) && !defined(OS_NACL) && (defined(OS_LINUX) || defined(OS_ANDROID)) On 2013/06/18 15:30:15, dsinclair wrote: > Could this be done through gyp? Have a trace_memory_stub.cc which is the > NOTREACHED versions and a trace_memory_tcmalloc.cc which is the real version. > > Have gyp pick which one we want at build time? (I have no idea if this is > possible/sane, heh) I think we follow this pattern of using ifdefs other places we interact with the heap profiler, for example in content/renderer/devtools/devtools_agent.h and in content/renderer/memory_benchmarking_extension.h https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:62: LazyInstance<ThreadLocalPointer<TraceMemoryStack> >::Leaky trace_memory_stack = On 2013/06/18 15:30:15, dsinclair wrote: > Not sure if it's in the styleguide, but in trace_event_impl.cc we named the > globals as g_trace_memory_stack. Style guide says it is optional to use g_, but I like it, so done. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:118: "memory", On 2013/06/18 15:30:15, dsinclair wrote: > TRACE_DISABLED_BY_DEFAULT("memory") ? Yes, good catch. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:129: ScopedTraceMemory initialize(TRACE_MEMORY_IGNORE); On 2013/06/18 15:30:15, dsinclair wrote: > Wouldn't it be easier to get just get the stack and see if it exists and > explicitly initialize here? > > Seems weird to create the fake event just to init the stack. > > (Or, change InitTraceMemoryStack() to just return if the stack exists then you > can just call it here without doing the get first.) Changed InitTraceMemoryStack() to GetTraceMemoryStack() which does initialize if needed, and used it here. That simplified later code too. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:181: TraceMemoryStart(); On 2013/06/18 15:30:15, dsinclair wrote: > Should this do a: > > if (dump_timer_.IsRunning()) > return; > > Just in case the trace-event framework sends enabled twice? Good idea. Done. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.c... base/debug/trace_memory.cc:203: stack = InitTraceMemoryStack(); On 2013/06/18 15:30:15, dsinclair wrote: > If InitTraceMemoryStack() always returns this can just become: > > TraceMemoryStack* stack = InitTraceMemoryStack(); > > (Although, maybe then it should be called GetTraceMemoryStack?) Done. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.h File base/debug/trace_memory.h (right): https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.h... base/debug/trace_memory.h:42: bool IsTimerRunningForTest() const; On 2013/06/18 15:30:15, dsinclair wrote: > If this is only for test, can we make it private and friend the test class? Done. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.h... base/debug/trace_memory.h:62: ScopedTraceMemory(const char* category); On 2013/06/18 15:30:15, dsinclair wrote: > The memory for category, does that need to be static or, who owns it? Added clarifying comment and changed to |name|, since that's what it actually records right now. https://codereview.chromium.org/15418002/diff/84001/base/debug/trace_memory.h... base/debug/trace_memory.h:80: // TODO(jamescook): Make it record both category and name. On 2013/06/18 15:30:15, dsinclair wrote: > You'll need to be careful when you add category in here as they can be a comma > separated list. > > TRACE_MEMORY("foo,bar,baz", "boop"); OK. For now I'm only recording the name, but we'll have to decide how category works (should it be recorded as-is, or split, and if split which fragment wins). https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... File base/message_loop/message_loop.cc (right): https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... base/message_loop/message_loop.cc:462: // was posted is much more useful than "MessageLoop::RunTask" so we invoke On 2013/06/20 20:09:29, nduca wrote: > lets pull this out to another followup patch.. its a good one that merits some > focused thinking, hence the proposal to pull out. OK. I pulled this section out. https://codereview.chromium.org/15418002/diff/84001/base/message_loop/message... base/message_loop/message_loop.cc:466: INTERNAL_TRACE_EVENT_ADD_SCOPED("task", "MessageLoop::RunTask", On 2013/06/18 15:30:15, dsinclair wrote: > This makes me sad. If we're going to be using it outside of the trace_event > files we should rename it to no longer be INTERNAL_. > > If this seems like something we'll do often, what about doing a: > > TRACE_EVENT_NO_MEMORY0,1,2? > > Then TRACE_EVENT0 just becomes > TRACE_MEMORY() \ > TRACE_EVENT_NO_MEMORY0 > > etc. Yeah. The only reason this was here was to support the hack above to get the file_name() of the posted task. We probably won't need access to INTERNAL_TRACE_EVENT if I can build TRACE_MEMORY1,2 etc. and record multiple items. https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... content/browser/browser_main_loop.cc:485: // TODO(jamescook): Support all tcmalloc platforms, including Windows. On 2013/06/20 20:09:29, nduca wrote: > How about always creating it, but have the observe do nothing if its not > supported? Then the goup about supported only goes inisde the observer... Done. Also consolidated all the macro craziness into a single TRACE_MEMORY_SUPPORTED macro. https://codereview.chromium.org/15418002/diff/84001/content/browser/browser_m... content/browser/browser_main_loop.cc:489: #endif On 2013/06/18 15:30:15, dsinclair wrote: > Does this also need to be done in places like the GPU process? Probably. I think I can shim this in around GpuMain, but could use some advice on whether that's the right place or not. https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h (right): https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/gperftools/heap-profiler.h:72: * applications that exclusively use GetHeapProfile(). On 2013/06/19 04:34:03, Dai Mikurube wrote: > This comment looks confusing. The existing profiler assumes that the profiler > is turned off if filename_prefix == NULL. > > The comment should explicitly describe which function is allowed, and which > function is disallowed in case of |prefix| == NULL. E.g. "No writing to disk > while profiling is started if |prefix| == NULL. GetHeapProfile() and ???() are > available, but ???() and ???() cannot be used." (My English statement might be > improved, though. ;)) Updated the comment. I think the system considers profiling off when |is_on| is false? Also, this should be a little clearer now that I'm using HeapProfilerWithPseudoStackStart(). https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profiler.cc:389: int skip_count) { On 2013/06/19 04:34:03, Dai Mikurube wrote: > It's almost the same with RecordAlloc(). Why do we need this additional > function? > > Also, don't we need RecordFree? I elected to copy the function because this is performance critical code inside the allocator - I thought it was better to do this than to combine the common parts of RecordAlloc and PseudoStackRecordAlloc and introduce an extra function call or branch. RecordFree is identical in both cases, the pseudo-stack push/pops are handled in the Chrome trace-event level and don't need any information about frees. https://codereview.chromium.org/15418002/diff/84001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profiler.cc:675: extern "C" void SetPseudoStackGenerator(PseudoStackGenerator callback) { On 2013/06/19 04:34:03, Dai Mikurube wrote: > The function name should contain HeapProfiler or something like that since the > function is in the global namespace. > > Also, what happens when this function is called after HeapProfilerStart()? What > happens when this function is called twice? I think we should hide it, and have > HeapProfilerStartWithPseudoStack() or something like that. Wdyt? Good idea. Done.
Thanks for working on it, James! Added some more comments. https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profile-table.cc:407: // and increases the size of the profile dumps significantly. Is maps so large? Is large output so serious? I don't think it's so reasonable to add this option |profile_self_maps| to drop maps... https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:389: // so it does not attempt to share code with RecordAlloc() above. Is it really performance critical? I don't think it affects performance even if an if-clause is added while I don't see the actual numbers. Also, in my understanding, it's not always on (only when tracing is on). I don't think it's worth to add almost duplicated new functions for it. Do you have numbers that show performance impact? The HeapProfiler is almost always on (|is_on|) when (PseudoStack)RecordAlloc is called. heap_profile->RecordAlloc is enough heavy to hide the performance impact since it always computes a hash value from a (maybe pseudo-)stack trace in GetBucket() and operates a hash table in AddressMap. In addition, we may be able to do it without an if-clause.
https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profile-table.cc:407: // and increases the size of the profile dumps significantly. On 2013/07/01 05:47:42, Dai Mikurube wrote: > Is maps so large? Is large output so serious? I don't think it's so reasonable > to add this option |profile_self_maps| to drop maps... There's also a problem where attempting to read /proc/self/maps from the GPU process causes it to hang. I haven't investigated that in detail because it seemed easier to skip collection of the data (since I don't use it anyway).
https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profile-table.cc:407: // and increases the size of the profile dumps significantly. On 2013/07/01 16:59:16, James Cook (Chromium) wrote: > On 2013/07/01 05:47:42, Dai Mikurube wrote: > > Is maps so large? Is large output so serious? I don't think it's so > reasonable > > to add this option |profile_self_maps| to drop maps... > > There's also a problem where attempting to read /proc/self/maps from the GPU > process causes it to hang. I haven't investigated that in detail because it > seemed easier to skip collection of the data (since I don't use it anyway). The GPU process hang is due to the security sandbox attempting to allocate memory inside of a signal handler when attempting to open the file /proc/self/maps. jorgelo@ on the security team is going to look into it. I'm going to disable my code in the GPU process until this is sorted out. You might also have problems with normal tcmalloc heap profiles of the GPU process, unless you are disabling the sandbox somehow.
Guys, please take another look. https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:389: // so it does not attempt to share code with RecordAlloc() above. On 2013/07/01 05:47:42, Dai Mikurube wrote: > Is it really performance critical? I don't think it affects performance even if > an if-clause is added while I don't see the actual numbers. Also, in my > understanding, it's not always on (only when tracing is on). I don't think it's > worth to add almost duplicated new functions for it. Do you have numbers that > show performance impact? > > The HeapProfiler is almost always on (|is_on|) when (PseudoStack)RecordAlloc is > called. heap_profile->RecordAlloc is enough heavy to hide the performance > impact since it always computes a hash value from a (maybe pseudo-)stack trace > in GetBucket() and operates a hash table in AddressMap. > > In addition, we may be able to do it without an if-clause. I benchmarked it both with and without a branch (introduced with ?:) -- there is no measurable difference. Thanks for suggesting I benchmark it. I consolidated the functions.
I'll say l g t m for the heap-profiler things in tcmalloc/chromium if the /proc/self/maps issue is finally fixed (or is determined to go as is). https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profile-table.cc:407: // and increases the size of the profile dumps significantly. On 2013/07/01 22:37:16, James Cook (Chromium) wrote: > On 2013/07/01 16:59:16, James Cook (Chromium) wrote: > > On 2013/07/01 05:47:42, Dai Mikurube wrote: > > > Is maps so large? Is large output so serious? I don't think it's so > > reasonable > > > to add this option |profile_self_maps| to drop maps... > > > > There's also a problem where attempting to read /proc/self/maps from the GPU > > process causes it to hang. I haven't investigated that in detail because it > > seemed easier to skip collection of the data (since I don't use it anyway). > > The GPU process hang is due to the security sandbox attempting to allocate > memory inside of a signal handler when attempting to open the file > /proc/self/maps. jorgelo@ on the security team is going to look into it. I'm > going to disable my code in the GPU process until this is sorted out. You might > also have problems with normal tcmalloc heap profiles of the GPU process, unless > you are disabling the sandbox somehow. Ah, makes sense. Sorry that I forgot it. In case of DMP, it requires --no-sandbox. Does the sandbox failure happen in the open() system call? If not, how about fixing ProcMapsIterator to "fail cleanly" when it cannot open /proc/self/maps? It's a reasonable fix also in upstream tcmalloc. If it doesn't fix, DisableProfileSelfMaps() might be the second best reasonable way. https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/107001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:389: // so it does not attempt to share code with RecordAlloc() above. On 2013/07/01 23:51:20, James Cook (Chromium) wrote: > On 2013/07/01 05:47:42, Dai Mikurube wrote: > > Is it really performance critical? I don't think it affects performance even > if > > an if-clause is added while I don't see the actual numbers. Also, in my > > understanding, it's not always on (only when tracing is on). I don't think > it's > > worth to add almost duplicated new functions for it. Do you have numbers that > > show performance impact? > > > > The HeapProfiler is almost always on (|is_on|) when (PseudoStack)RecordAlloc > is > > called. heap_profile->RecordAlloc is enough heavy to hide the performance > > impact since it always computes a hash value from a (maybe pseudo-)stack trace > > in GetBucket() and operates a hash table in AddressMap. > > > > In addition, we may be able to do it without an if-clause. > > I benchmarked it both with and without a branch (introduced with ?:) -- there is > no measurable difference. Thanks for suggesting I benchmark it. I consolidated > the functions. > Thank you for working on it! JFYI, I'm not confident, but I think we may not need any branch if you initialize |pseudo_stack_generator| with HeapProfileTable::GetCallerStackTrace with using the same function signature.
It seems to me that tcmalloc reading /proc isn't required when we're using pseudo stacks. Thats only needed when the dump itself has real pointers in it. Can we disable this somehow?
On 2013/07/02 08:01:02, nduca wrote: > It seems to me that tcmalloc reading /proc isn't required when we're using > pseudo stacks. Thats only needed when the dump itself has real pointers in it. > > Can we disable this somehow? I suggest the following fix instead of adding an option to disable it. > about fixing ProcMapsIterator to "fail cleanly" when it cannot open > /proc/self/maps? It's a reasonable fix also in upstream tcmalloc. The reason is because we should minimize un-upstreamable changes in third_party/tcmalloc. Such changes make it harder to roll up the third_party library. I think DisableProfileSelfMaps() is too specific to upstream.
> > about fixing ProcMapsIterator to "fail cleanly" when it cannot open > > /proc/self/maps? It's a reasonable fix also in upstream tcmalloc. Thats fine. Im sorry I missed that note in your previous review.
lgtm with a couple nits. https://codereview.chromium.org/15418002/diff/121001/base/debug/trace_event_m... File base/debug/trace_event_memory.h (right): https://codereview.chromium.org/15418002/diff/121001/base/debug/trace_event_m... base/debug/trace_event_memory.h:78: // to trace event compatible JSON and append to |output|. Visible for testing. nit: Extra to https://codereview.chromium.org/15418002/diff/121001/base/debug/trace_event_m... base/debug/trace_event_memory.h:82: // Converts a the first |line| of heap profiler data into trace event compatible nit: Extra a https://codereview.chromium.org/15418002/diff/121001/base/debug/trace_event_m... base/debug/trace_event_memory.h:82: // Converts a the first |line| of heap profiler data into trace event compatible Is this the first |line| it's converting, or the totals?
Dai, Nat, please take another look. +jorgelo FYI I replaced the optional pseudo_stack_generator callback with a stack_generator_function that either points to HeapProfileTable::GetCallerStackTrace() or the pseudo-stack generator, which avoids a branch in RecordAlloc. For the GPU process hang when opening /proc/self/maps, I can't try to open it then recover from a failure. The hang is inside the open() call. I talked to jorgelo@ on the security team -- the root cause of the problem is a bug in the sandboxing code. It is allocating memory inside a signal handler when it shouldn't. Jorge planning to fix this, see crbug.com/256452. We could try to detect if tcmalloc is running inside an application inside a sandbox by making an unnecessary system call to a function that we know will be blocked by the sandbox, but will not trigger the bug above. That seems hacky and I would prefer not to do it. I suggest we use this patch, which does not attempt to profile the GPU process. When Jorge fixes the bug above, I will add back pseudo-stack profiling to the GPU process. Does that sound OK? Or would you prefer that I add back the code to skip trying to read /proc/self/maps?
https://codereview.chromium.org/15418002/diff/129001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/129001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:382: int depth = (*stack_generator_function)(skip_count + 1, stack); FYI - I manually tested both pseudo-stack profiles and normal stack unwind profiles. Both work OK with this patch.
I like this patch. lgtm!
jar, can I get OWNERS approval for third_party/tcmalloc/*? This is the first patch towards implementing tcmalloc heap profiling with "pseudo-stacks" from the trace event system -- Nat mentioned it yesterday. The output looks like this: http://dev.chromium.org/developers/abouttracing-memory-snapshots
A bunch of nits, and a few real questions. https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... File base/debug/trace_event_memory.cc (right): https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:18: #if !defined(NO_TCMALLOC) && !defined(OS_NACL) && (defined(OS_LINUX) || defined(OS_ANDROID)) nit: I think you can backslash break this line. https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:59: memset(category_stack_, 0, kMaxStackSize * sizeof(const char*)); nit: Always try to use variable, rather than a raw type, in a sizeof. Better is sizeof(category_stack_[0]) https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:76: // Intentionally leak one stack per thread. How does this play with worker threads? Is this really an unbounded leak, as threads are recycled? https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:94: stack->index_ < kMaxStackSize ? stack->index_ : kMaxStackSize; nit: std::max() https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:96: memcpy(stack_out, stack->category_stack_, count * sizeof(void*)); nit: sizeof(stack->category_[0]) https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:113: DVLOG(1) << "DumpMemoryProfile"; Do you care about allocations done by these logs? Should this be inside your the block starting after 116? https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:121: if (enabled) { nit: (personal?): early returns often help readability, and avoid the need to indent blocks. https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:181: DCHECK(message_loop_proxy_->PostTask( DCHECKs should never have side effects. If you meant this, much better is: if !defined(NDEBUG) Same thing on line 189. https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... File base/debug/trace_event_memory.h (right): https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.h:29: TraceMemoryController( nit: explicit? https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.h:67: ScopedTraceMemory(const char* name); nit: explicit https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m... base/debug/trace_event_memory.h:73: }; nit: DISALLOW_COPY_AND_ASSIGN (despite the fact that we don't manipulate 'em.... it is just a style rule). https://codereview.chromium.org/15418002/diff/129001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/129001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:551: if (prefix) { nit: (personal?): Early return?
jar, please take another look. I had to do a rebase in the middle to get it to
build, apologies.
It turns out we have two different thread-local storage wrappers. I
re-implemented my TLS with the one from thread_local_storage.{h|cc}, which
allows a finalization/cleanup function to run when the thread is destroyed,
allowing me to deallocate my memory.
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
File base/debug/trace_event_memory.cc (right):
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.cc:18: #if !defined(NO_TCMALLOC) &&
!defined(OS_NACL) && (defined(OS_LINUX) || defined(OS_ANDROID))
On 2013/07/04 02:18:30, jar wrote:
> nit: I think you can backslash break this line.
Done.
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.cc:59: memset(category_stack_, 0, kMaxStackSize *
sizeof(const char*));
On 2013/07/04 02:18:30, jar wrote:
> nit: Always try to use variable, rather than a raw type, in a sizeof. Better
is
> sizeof(category_stack_[0])
Ah, I had forgotten that part of the style guide. Done, thanks!
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.cc:94: stack->index_ < kMaxStackSize ?
stack->index_ : kMaxStackSize;
On 2013/07/04 02:18:30, jar wrote:
> nit: std::max()
Done (I actually need std::min here).
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.cc:96: memcpy(stack_out, stack->category_stack_,
count * sizeof(void*));
On 2013/07/04 02:18:30, jar wrote:
> nit: sizeof(stack->category_[0])
Done.
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.cc:113: DVLOG(1) << "DumpMemoryProfile";
On 2013/07/04 02:18:30, jar wrote:
> Do you care about allocations done by these logs? Should this be inside your
the
> block starting after 116?
Good point. Moved below the macro that disables tracing allocations.
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.cc:121: if (enabled) {
On 2013/07/04 02:18:30, jar wrote:
> nit: (personal?): early returns often help readability, and avoid the need to
> indent blocks.
Done.
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
File base/debug/trace_event_memory.h (right):
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.h:29: TraceMemoryController(
On 2013/07/04 02:18:30, jar wrote:
> nit: explicit?
Done.
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.h:67: ScopedTraceMemory(const char* name);
On 2013/07/04 02:18:30, jar wrote:
> nit: explicit
Done.
https://codereview.chromium.org/15418002/diff/129001/base/debug/trace_event_m...
base/debug/trace_event_memory.h:73: };
On 2013/07/04 02:18:30, jar wrote:
> nit: DISALLOW_COPY_AND_ASSIGN (despite the fact that we don't manipulate
> 'em.... it is just a style rule).
Done.
https://codereview.chromium.org/15418002/diff/129001/third_party/tcmalloc/chr...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):
https://codereview.chromium.org/15418002/diff/129001/third_party/tcmalloc/chr...
third_party/tcmalloc/chromium/src/heap-profiler.cc:551: if (prefix) {
On 2013/07/04 02:18:30, jar wrote:
> nit: (personal?): Early return?
Done.
Mostly nits... https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... File base/debug/trace_event_memory.cc (right): https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:64: int index_; nit: better is size_t https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:253: const int index = trace_memory_stack->index_; nit: size_t https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:341: output->resize(output->size() - 2); I think this works... but the approach of backing-up scares me a bit (and certainly made me read this more carefully). Better pattern might be to have AppendHeapProfileLineAsTraceFormat() (if it decides to add anything) pre-insert a ",\n" at line 378. That would also get rid of line 331, as well as line 337 and 338. https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:367: // 68: 4195 [ 1087: 98009] @ 0x7fa7fa9b9ba0 0x7fa7f4b3be13 nit: Please add comments telling what these numbers mean. https://codereview.chromium.org/15418002/diff/159001/content/browser/browser_... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/15418002/diff/159001/content/browser/browser_... content/browser/browser_main_loop.h:21: } nit: Namespace termination curlies should have comment like: // namespace debug Also applies to namespaces terminators below. https://codereview.chromium.org/15418002/diff/159001/content/renderer/render_... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/15418002/diff/159001/content/renderer/render_... content/renderer/render_thread_impl.h:51: } nit: // namespace debug Same above and below. https://codereview.chromium.org/15418002/diff/159001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/159001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:228: &HeapProfileTable::GetCallerStackTrace; nit: no need for the ampersand. https://codereview.chromium.org/15418002/diff/159001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:382: int depth = (*stack_generator_function)(skip_count + 1, stack); nit: no need for the indirection or parens. stack_generator_function(skip_count + 1, stack); Should be fine.
jar, please take another look https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... File base/debug/trace_event_memory.cc (right): https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:64: int index_; On 2013/07/12 01:24:26, jar wrote: > nit: better is size_t Done. https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:253: const int index = trace_memory_stack->index_; On 2013/07/12 01:24:26, jar wrote: > nit: size_t Done. https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:341: output->resize(output->size() - 2); On 2013/07/12 01:24:26, jar wrote: > I think this works... but the approach of backing-up scares me a bit (and > certainly made me read this more carefully). > > Better pattern might be to have AppendHeapProfileLineAsTraceFormat() (if it > decides to add anything) pre-insert a ",\n" at line 378. That would also get > rid of line 331, as well as line 337 and 338. Yes, that's cleaner, thanks. Used your suggestion. (FWIW, I learned the backing-up trick at my last job. It's cheap on std::string and can avoid lots of conditionals around when to insert delimiters. But it's not needed here.) https://codereview.chromium.org/15418002/diff/159001/base/debug/trace_event_m... base/debug/trace_event_memory.cc:367: // 68: 4195 [ 1087: 98009] @ 0x7fa7fa9b9ba0 0x7fa7f4b3be13 On 2013/07/12 01:24:26, jar wrote: > nit: Please add comments telling what these numbers mean. Done. https://codereview.chromium.org/15418002/diff/159001/content/browser/browser_... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/15418002/diff/159001/content/browser/browser_... content/browser/browser_main_loop.h:21: } On 2013/07/12 01:24:26, jar wrote: > nit: Namespace termination curlies should have comment like: > > // namespace debug > > Also applies to namespaces terminators below. Done, but are you sure you want me to do this? Across wide swaths of Chrome brief forward-declarations of classes do not use namespace terminator comments. This seems inconsistent with the rest of the code base, though I cannot find a Google or Chromium C++ style guide reference specifically about namespace comments for forward declarations. https://codereview.chromium.org/15418002/diff/159001/content/renderer/render_... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/15418002/diff/159001/content/renderer/render_... content/renderer/render_thread_impl.h:51: } On 2013/07/12 01:24:26, jar wrote: > nit: // namespace debug > > Same above and below. Done, but again, this seems inconsistent with the rest of the code base. https://codereview.chromium.org/15418002/diff/159001/third_party/tcmalloc/chr... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/15418002/diff/159001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:228: &HeapProfileTable::GetCallerStackTrace; On 2013/07/12 01:24:26, jar wrote: > nit: no need for the ampersand. Done. https://codereview.chromium.org/15418002/diff/159001/third_party/tcmalloc/chr... third_party/tcmalloc/chromium/src/heap-profiler.cc:382: int depth = (*stack_generator_function)(skip_count + 1, stack); On 2013/07/12 01:24:26, jar wrote: > nit: no need for the indirection or parens. > > stack_generator_function(skip_count + 1, stack); > > Should be fine. Done. Apparently I have forgotten how to deal with C-style function pointers. :-)
piman, can I get OWNERS approval for content/* ? I'm adding an observer for the trace event system so I can turn on and off memory profiling information for about:tracing.
2 questions, otherwise the patch seems reasonable: 1- what does it mean to instantiate a TraceMemoryController? Since it's not passed anywhere, I assume it registers itself with some sort of singleton, but what is the side effect of that? In particular, does that mean that we're now collecting info before we start tracing? Does it have a memory/perf impact? 2- would it make sense to instantiate it in ChildThread instead of RenderThread, so that it benefits all child process types, not just the renderer?
Addressing piman's comments: > 1- what does it mean to instantiate a TraceMemoryController? Since it's not > passed anywhere, I assume it registers itself with some sort of singleton, but > what is the side effect of that? In particular, does that mean that we're now > collecting info before we start tracing? Does it have a memory/perf impact? Instantiating a TraceMemoryController just registers an observer with the TraceLog system, waiting for tracing to be turned on. It doesn't start doing any collection by itself. There should be no memory/perf impact until tracing is turned on (and then only if you check the off-by-default memory checkbox, and even then the impact is small). > 2- would it make sense to instantiate it in ChildThread instead of RenderThread, > so that it benefits all child process types, not just the renderer? Good idea - I didn't notice ChildThread before. Done. (There used to be deadlock in the heap-profiler's memory allocator caused by the GPU process security sandbox's signal handler allocating memory. jorgelo fixed it and I verified I can run my heap profiling on the GPU process again.)
lgtm https://codereview.chromium.org/15418002/diff/183001/content/renderer/render_... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/15418002/diff/183001/content/renderer/render_... content/renderer/render_thread_impl.h:51: class TraceMemoryController; nit: you don't need this any more, though you may need it in child_thread.h.
Brett/Darin: Do you know what the style guide advocates for the issue below? I suspect the resolution should "follow" from our OS tests like: #if defined(OS_POSIX) that we should always go the def vs undef route... but it is not explicit (for anything other than OS_* in our Chromium style guide. https://codereview.chromium.org/15418002/diff/188001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/15418002/diff/188001/base/debug/trace_event.h... base/debug/trace_event.h:242: #define TRACING_IS_OFFICIAL_BUILD 0 I wish I could find a standard style.... but I couldn't :-( IMO, this should read: #undef TRACING_IS_OFFICIAL_BUILD The big question is, should we test via: #if defined(TRACING_IS_OFFICIAL) or via #if TRACING_IS_OFFICIAL If you use the undef, then both tests will work "as expected." If you use the define of 0, then one will work, and will will be consistently wrong :-(. There should be a style standard, and I can <sadly> see this is confusing others (I went searching for conflicting code). Take for instance: https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=O... which usesd "defined(OFFICIAL_BUILD)". Then look at: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/remoting.... which sadly defines (in a gyp) OFFICIAL_BUILD to be 0 :-(. IMO, it is better to use a definition which is safe with either test. I think the gyp is wrong in the above example... but I probably need a standard... and I couldn't find one in the style guide.
https://codereview.chromium.org/15418002/diff/188001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/15418002/diff/188001/base/debug/trace_event.h... base/debug/trace_event.h:242: #define TRACING_IS_OFFICIAL_BUILD 0 On 2013/07/13 00:51:08, jar wrote: > I wish I could find a standard style.... but I couldn't :-( > > IMO, this should read: > #undef TRACING_IS_OFFICIAL_BUILD > > The big question is, should we test via: > #if defined(TRACING_IS_OFFICIAL) > or via > #if TRACING_IS_OFFICIAL > > If you use the undef, then both tests will work "as expected." If you use the > define of 0, then one will work, and will will be consistently wrong :-(. > > There should be a style standard, and I can <sadly> see this is confusing others > (I went searching for conflicting code). > > Take for instance: > https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=O... > which usesd "defined(OFFICIAL_BUILD)". > > Then look at: > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/remoting.... > > which sadly defines (in a gyp) OFFICIAL_BUILD to be 0 :-(. > > IMO, it is better to use a definition which is safe with either test. I think > the gyp is wrong in the above example... but I probably need a standard... and I > couldn't find one in the style guide. Could we resolve the issue about official build macros outside my review? That piece of code pre-dates my changes and I'm not an owner of this module -- maybe nduca or dsinclair could help you sort it out?
I think it is fine to proceed using the style of the module. We can file a bug for cleanup if you wish, @jar.
LGTM with the one nit below. Thanks. https://codereview.chromium.org/15418002/diff/188001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/15418002/diff/188001/base/debug/trace_event.h... base/debug/trace_event.h:242: #define TRACING_IS_OFFICIAL_BUILD 0 On 2013/07/15 16:58:27, James Cook (Chromium) wrote: > On 2013/07/13 00:51:08, jar wrote: > > I wish I could find a standard style.... but I couldn't :-( > > > > IMO, this should read: > > #undef TRACING_IS_OFFICIAL_BUILD > > > > The big question is, should we test via: > > #if defined(TRACING_IS_OFFICIAL) > > or via > > #if TRACING_IS_OFFICIAL > > > > If you use the undef, then both tests will work "as expected." If you use the > > define of 0, then one will work, and will will be consistently wrong :-(. > > > > There should be a style standard, and I can <sadly> see this is confusing > others > > (I went searching for conflicting code). > > > > Take for instance: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=O... > > which usesd "defined(OFFICIAL_BUILD)". > > > > Then look at: > > > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/remoting.... > > > > which sadly defines (in a gyp) OFFICIAL_BUILD to be 0 :-(. > > > > IMO, it is better to use a definition which is safe with either test. I think > > the gyp is wrong in the above example... but I probably need a standard... and > I > > couldn't find one in the style guide. > > Could we resolve the issue about official build macros outside my review? That > piece of code pre-dates my changes and I'm not an owner of this module -- maybe > nduca or dsinclair could help you sort it out? Please switch to one of these alternatives in all cases: #define TRACING_IS_OFFICIAL_BUILD 1 vs #undef TRACING_IS_OFFICIAL_BUILD The latter is what you should use on line 242.
lgtm
Yeah, I don't think we have a style rule here. In my experience, the #if defined(FOO) approach is actually somewhat flawed. It is too easy to be burned by mistyping FOO. The #if FOO approach on the other hand will complain if you mistype FOO, which can really be helpful. I have more thoughts on this topic. The Blink / WebKit approach has some other nice advantages. Anyways, this is probably best as a topic on chromium-dev. -Darin On Fri, Jul 12, 2013 at 5:51 PM, <jar@chromium.org> wrote: > Brett/Darin: Do you know what the style guide advocates for the issue > below? > > I suspect the resolution should "follow" from our OS tests like: > #if defined(OS_POSIX) > that we should always go the def vs undef route... but it is not explicit > (for > anything other than OS_* in our Chromium style guide. > > > > > https://codereview.chromium.**org/15418002/diff/188001/base/** > debug/trace_event.h<https://codereview.chromium.org/15418002/diff/188001/base/debug/trace_event.h> > File base/debug/trace_event.h (right): > > https://codereview.chromium.**org/15418002/diff/188001/base/** > debug/trace_event.h#newcode242<https://codereview.chromium.org/15418002/diff/188001/base/debug/trace_event.h#newcode242> > base/debug/trace_event.h:242: #define TRACING_IS_OFFICIAL_BUILD 0 > I wish I could find a standard style.... but I couldn't :-( > > IMO, this should read: > #undef TRACING_IS_OFFICIAL_BUILD > > The big question is, should we test via: > #if defined(TRACING_IS_OFFICIAL) > or via > #if TRACING_IS_OFFICIAL > > If you use the undef, then both tests will work "as expected." If you > use the define of 0, then one will work, and will will be consistently > wrong :-(. > > There should be a style standard, and I can <sadly> see this is > confusing others (I went searching for conflicting code). > > Take for instance: > https://code.google.com/p/**chromium/codesearch#chromium/** > src/base/logging.h&q=OFFICIAL_**BUILD%20defined%20&sq=package:** > chromium&type=cs&l=476<https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=OFFICIAL_BUILD%20defined%20&sq=package:chromium&type=cs&l=476> > which usesd "defined(OFFICIAL_BUILD)". > > Then look at: > https://code.google.com/p/**chromium/codesearch#chromium/** > src/remoting/remoting.gyp&q=**OFFICIAL_BUILD%200&sq=package:** > chromium&type=cs&l=1895<https://code.google.com/p/chromium/codesearch#chromium/src/remoting/remoting.gyp&q=OFFICIAL_BUILD%200&sq=package:chromium&type=cs&l=1895> > > which sadly defines (in a gyp) OFFICIAL_BUILD to be 0 :-(. > > IMO, it is better to use a definition which is safe with either test. I > think the gyp is wrong in the above example... but I probably need a > standard... and I couldn't find one in the style guide. > > https://codereview.chromium.**org/15418002/<https://codereview.chromium.org/1... >
Filed https://code.google.com/p/chromium/issues/detail?id=261382 to track the OFFICIAL_BUILD macro cleanup.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamescook@chromium.org/15418002/212001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamescook@chromium.org/15418002/229001
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_dbg_simulator. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamescook@chromium.org/15418002/248002
Retried try job too often on linux_chromeos for step(s) cacheinvalidation_unittests, dbus_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamescook@chromium.org/15418002/286001
Message was sent while issue was closed.
Change committed as 213473 |
