Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
956413002:
[tracing] Enable actual generation of memory dump points. (Closed)
|
Created:
5 years, 10 months ago by Primiano Tucci (use gerrit) Modified:
5 years, 9 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, Sami, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@no_sandbox_fix Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Enable actual generation of memory dump points.
This CL introduces a new TRACE_EVENT_PHASE for memory dump
and enables the MemoryDumpManager to generate actual trace
events when CreateLocalDumpPoint is invoked.
BUG=458295
Committed: https://crrev.com/4a7ba126270ca1c2339aa91a18794041db562804
Cr-Commit-Position: refs/heads/master@{#318727}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Address review comments #
Messages
Total messages: 16 (4 generated)
primiano@chromium.org changed reviewers: + dsinclair@chromium.org, nduca@chromium.org
PTAL
lgtm with nit https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:140: const uint64 event_guid = ++g_next_guid; i think you need TRACE_ID_DONT_MANGLE -- see trace_event.h macro explanation
https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:142: TRACE_EVENT_API_ADD_TRACE_EVENT( I'm not sure if want to leak this out of trace_event.h or not, it's an implementation detail. Would it be better to add an macro? TRACE_EVENT_MEMORY_DUMP(kTraceCategory, event_name, event_guid, pmd) Or something? https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event... base/trace_event/trace_event.h:1042: #define TRACE_EVENT_PHASE_MEMORY_DUMP ('v') Can you make sure this gets added back into the tracing doc which lists out the various event flags?
picksi@google.com changed reviewers: + picksi@google.com
Some bike-sheddy comments :) https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:121: if (!dump_provider->DumpInto(pmd.get())) { Not part of this CL, but it would read easier if you removed the '!' and swapped the if/else bodies around. https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:136: [Without thinking too deeply about this...] Does the remainder of this function need to be inside the lock? (if g_next_guid was atomically incremented, I suppose?) Are TRACE_EVENTS thread safe? https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:145: kTraceEventNumArgs, kTraceEventArgNames, kTraceEventArgTypes, NULL, It would make this code clearer if the NULL was a named const saying what is was. https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.h:68: void CreateLocalDumpPoint(DumpPointType type); IMO It would aid readability throughout if 'type' was renamed dumpPointType (i.e. match the class). https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event... base/trace_event/trace_event.h:1042: #define TRACE_EVENT_PHASE_MEMORY_DUMP ('v') Is there any rationale for choosing lowercase or uppercase for the phase IDs? Is lowercase here 'correct'?
https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event... base/trace_event/trace_event.h:1042: #define TRACE_EVENT_PHASE_MEMORY_DUMP ('v') On 2015/02/26 at 17:58:04, picksi wrote: > Is there any rationale for choosing lowercase or uppercase for the phase IDs? Is lowercase here 'correct'? We don't have a proscribed system for dolling these out, it's kinda random. We try to pick things that make sense, but that broke down a while ago. I think 'v' is as good as anything else....
https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event.h File base/trace_event/trace_event.h (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event... base/trace_event/trace_event.h:1042: #define TRACE_EVENT_PHASE_MEMORY_DUMP ('v') On 2015/02/26 18:02:06, dsinclair wrote: > On 2015/02/26 at 17:58:04, picksi wrote: > > Is there any rationale for choosing lowercase or uppercase for the phase IDs? > Is lowercase here 'correct'? > > We don't have a proscribed system for dolling these out, it's kinda random. We > try to pick things that make sense, but that broke down a while ago. I think 'v' > is as good as anything else.... I used 'v' because I saw that in the Trace Event format doc [1]: "Memory Dump Events V (global), v (process)" [1] https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... https://codereview.chromium.org/956413002/diff/1/base/trace_event/trace_event... base/trace_event/trace_event.h:1042: #define TRACE_EVENT_PHASE_MEMORY_DUMP ('v') On 2015/02/26 17:54:54, dsinclair wrote: > Can you make sure this gets added back into the tracing doc which lists out the > various event flags? It's already there, I took it from https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy...
https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:121: if (!dump_provider->DumpInto(pmd.get())) { On 2015/02/26 17:58:04, picksi wrote: > Not part of this CL, but it would read easier if you removed the '!' and swapped > the if/else bodies around. Makes sense. Done https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:136: On 2015/02/26 17:58:04, picksi wrote: > [Without thinking too deeply about this...] Does the remainder of this function > need to be inside the lock? (if g_next_guid was atomically incremented, I > suppose?) Done. Turns out I needed to change the g_next_guid logic anyways. > Are TRACE_EVENTS thread safe? My understanding is that TRACE_EVENTs use thread-local buffers so should be thread safe. https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:140: const uint64 event_guid = ++g_next_guid; On 2015/02/26 17:26:29, nduca wrote: > i think you need TRACE_ID_DONT_MANGLE -- see trace_event.h macro explanation Right. FTR: the rationale here is that we don't want tracing to mangle the id, because we need to read it back to pass it to the other processes via IPC. Removing the mangling logic and adding a TODO + bug (crbug.com/462931) to track this (a longer description of the problem is avilable in the bug). Will do in a separate CL. My understanding is that TRACE_ID_DONT_MANGLE is not needed in this case (that is for the higher level TRACE_EVENT_ macros not when using TRACE_EVENT_API_ADD_TRACE_EVENT). I just need to remove the TRACE_EVENT_FLAG_MANGLE_ID flag I added. At the end all boils down to having or not the TRACE_EVENT_FLAG_MANGLE_ID flag in the event). In other words: TRACE_ID_DONT_MANGLE -> TraceID::DontMangle -> Noop TRACE_ID_MANGLe -> TraceID::ForceMangle -> Sets the TRACE_EVENT_FLAG_MANGLE_ID flag. https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:142: TRACE_EVENT_API_ADD_TRACE_EVENT( On 2015/02/26 17:54:54, dsinclair wrote: > I'm not sure if want to leak this out of trace_event.h or not, it's an > implementation detail. Oh, I though that was meant to be part of the public API as it was in trace_event.h (vs _impl), anyways memory_dump_manager is part of trace_event (read: not sure if it should be considered a tracing "client" or an implementation detail of tracing). (Note, there is one precedent for TRACE_EVENT_API_ADD_TRACE_EVENT in ui/gl/gl_implementation_win.cc) > Would it be better to add an macro? I had this discussion with skyostil@ when writing this. The line of reasoning of our discussion was: This should be the only point in the memory tracing code which creates actual trace events. In other words, I don't expect to see any other TRACE_EVENT_API_ADD_TRACE_EVENT(..'v'..) anywhere else in the memory tracing code. All the future code should ultimately reach into the memory dump manager. In this optic, it felt more maintainable using an existing API rather than creating a macro which is going to be used only once in the codebase. In summary: My personal preference would be macroizing this only the day that we'd need a 2nd call to this, and keep as it is until that point. WDYT? https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.cc:145: kTraceEventNumArgs, kTraceEventArgNames, kTraceEventArgTypes, NULL, On 2015/02/26 17:58:04, picksi wrote: > It would make this code clearer if the NULL was a named const saying what is > was. Makes sense making it explicit. IIRC in chromium we tend to avoid dummy consts in favor of this pattern: NULL /* arg_values */ Done https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/956413002/diff/1/base/trace_event/memory_dump... base/trace_event/memory_dump_manager.h:68: void CreateLocalDumpPoint(DumpPointType type); On 2015/02/26 17:58:04, picksi wrote: > IMO It would aid readability throughout if 'type' was renamed dumpPointType > (i.e. match the class). Done.
Looks good!
lgtm
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org Link to the patchset: https://codereview.chromium.org/956413002/#ps20001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/956413002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4a7ba126270ca1c2339aa91a18794041db562804 Cr-Commit-Position: refs/heads/master@{#318727} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
