Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23934003:
Have all trace points emit to ETW. (Closed)
|
Created:
7 years, 3 months ago by fdoray Modified:
4 years, 1 month ago CC:
chromium-reviews, dsinclair+watch_chromium.org, erikwright+watch_chromium.org, etienneb Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionHave all trace points emit to ETW.
Have all activated trace points emit to ETW when a trace session is listening for events from Chrome.
As explained at crbug.com/81584, when no trace session is active, the cost of this instrumentation is to check a simple condition.
For now, trace point categories must be activated via chrome://tracing. In a forthcoming CL, it will be possible to enable categories with a command line flag. Categories enabled with this command line flag will emit to ETW but not to the Chrome tracing system.
TEST=
1. Execute these commands:
logman create trace log -mode globalsequence -o log.etl
logman update trace log -p {3DADA31D-19EF-4dc1-B345-037927193422} 0xffffffff 4
logman start log
2. Launch Chrome.
3. Go to chrome://tracing.
4. Click "Record".
5. Enable a few categories and click "Record".
6. Quit Chrome.
7. Execute this command:
logman stop log
Expected result: A file named "log.etl" has been created. It contains recorded events.
BUG=81584
R=chrisha
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : Add "extras" string. #Patch Set 4 : Convert event types to begin/end/instant #Patch Set 5 : Rename EXT -> OLD #Patch Set 6 : Output categories #Patch Set 7 : Reupload #Patch Set 8 : Reupload #Patch Set 9 : Rebase #
Total comments: 7
Patch Set 10 : Address Chris' comments, improved trace format #Patch Set 11 : Rebase #Patch Set 12 : Nits #
Total comments: 13
Patch Set 13 : Address Chris' comments #Patch Set 14 : Address Chris' comments #
Total comments: 21
Patch Set 15 : Address Siggi's comments #
Total comments: 1
Patch Set 16 : Nits #
Total comments: 5
Patch Set 17 : Address Etienne's comments #Patch Set 18 : Add missing condition. #Patch Set 19 : Rebase. #Patch Set 20 : Rebase #
Messages
Total messages: 15 (0 generated)
Some comments. https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_imp... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_imp... base/debug/trace_event_impl.cc:1205: TraceEventETWProvider::Trace(name, phase, id, NULL); Remove extra space. https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_win.cc File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_win... base/debug/trace_event_win.cc:100: unsigned long long id, Why are we changing the type of this? https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_win... base/debug/trace_event_win.cc:111: provider->TraceEvent(name, name_len, type, id, extra, extra_len); TraceEvent takes a const void * too, no? There's a type size mismatch here.
Chris, could you review the CL again? Thanks. https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_imp... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_imp... base/debug/trace_event_impl.cc:1205: TraceEventETWProvider::Trace(name, phase, id, NULL); On 2013/09/04 13:32:09, chrisha wrote: > Remove extra space. Done. https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_win.cc File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/3001/base/debug/trace_event_win... base/debug/trace_event_win.cc:100: unsigned long long id, I wanted to avoid an extra type conversion: |id| comes from TraceLog::AddTraceEventWithThreadIdAndTimestamp, which uses an "unsigned long long". |id| can also come from the TRACE_EVENT_*_ETW macros (via base::debug::TraceLog::AddTraceEventEtw), but I understand from crbug.com/81584 that these macros will disappear. Sawbuck had trouble displaying |extra| when I used an "unsigned long long" so I will keep a "const void*" for now. On 2013/09/04 13:32:09, chrisha wrote: > Why are we changing the type of this? https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event.h#... base/debug/trace_event.h:913: #define TRACE_EVENT_PHASE_INSTANT_OLD ('I') // Used by third party libraries. According to https://code.google.com/p/chromium/codesearch#chromium/src/third_party/trace-..., I is historic. It's still used by WebRTC, Webkit, Angle, etc.
Some more comments. https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event.h#... base/debug/trace_event.h:913: #define TRACE_EVENT_PHASE_INSTANT_OLD ('I') // Used by third party libraries. Two spaces before // https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:155: } This string manipulation is relatively expensive. How often does it occur? On every event? If this is a bottleneck, is it worth memoizing the results so we don't do the string manipulation each time? Maybe wrap the whole thing inside of provider->IsTracing like the above implementation of Trace? https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event_wi... base/debug/trace_event_win.h:63: // a string. This is a complicated call. Worth documenting the arguments?
Chris, could you look at the CL again? Thanks. https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event.h#... base/debug/trace_event.h:913: #define TRACE_EVENT_PHASE_INSTANT_OLD ('I') // Used by third party libraries. On 2013/09/05 20:21:30, chrisha wrote: > Two spaces before // Done. https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:155: } The method is called for every event. I wrapped the code inside provider->IsTracing as you suggested. Also, I changed the output format so that no expensive string manipulation is necessary even when a tracing session is active. This new format follows what is described at http://msdn.microsoft.com/en-us/library/windows/desktop/aa364148(v=vs.85).aspx to output many parameters. It's easier to parse the resulting .etl. On 2013/09/05 20:21:30, chrisha wrote: > This string manipulation is relatively expensive. How often does it occur? On > every event? > > If this is a bottleneck, is it worth memoizing the results so we don't do the > string manipulation each time? > > Maybe wrap the whole thing inside of provider->IsTracing like the above > implementation of Trace? https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/27001/base/debug/trace_event_wi... base/debug/trace_event_win.h:63: // a string. On 2013/09/05 20:21:30, chrisha wrote: > This is a complicated call. Worth documenting the arguments? Done. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:142: This method is only used by TRACE_EVENT_*_ETW macros, which are going to disappear according to crbug.com/81584. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:162: unsigned long long id, This method is called from TraceLog::AddTraceEventWithThreadIdAndTimestamp, where the type of |id| is unsigned long long. |id| cannot easily be truncated to void*: id of MessageLoop::PostTask events -> MSB=task id unique in the pool, LSB=pointer to the pool id of IPC events -> MSB=0x0, LSB=unique id I chose to output the 64 bits id to make sure that I wouldn't end up with duplicated ids (as I did with my previous patch). What do you think? Should I try to create a 32 bits id by adding the MSB and LSB? Or can I keep the 64 bits id?
This looks better than all of the string encoding logic used previously. A few more questions... https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:29: const int kTraceNumFields = 6 + 2 * kTraceMaxNumArgs; Where does this magic number come from? Link to documentation, or a more detailed comment? https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:111: } Do we want to have some DCHECK or log message if num_args > kTraceMaxNumArgs? https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:162: unsigned long long id, On 2013/09/08 18:35:50, fdoray wrote: > This method is called from TraceLog::AddTraceEventWithThreadIdAndTimestamp, > where the type of |id| is unsigned long long. > > |id| cannot easily be truncated to void*: > > id of MessageLoop::PostTask events -> MSB=task id unique in the pool, > LSB=pointer to the pool > id of IPC events -> MSB=0x0, LSB=unique id > > I chose to output the 64 bits id to make sure that I wouldn't end up with > duplicated ids (as I did with my previous patch). > > What do you think? Should I try to create a 32 bits id by adding the MSB and > LSB? Or can I keep the 64 bits id? The 64-bit ID seems fine, as long as we're consistent with it everywhere. Having a mix of different ID sizes is what was confusing me originally. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.h:62: // Emit a trace. The event is defined by |category_group|, |name| and |type|. Emit a trace event. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.h:101: // Emit a trace. Emit a trace event. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.h:120: // string. The length of |name| must be specified by |name_len|. Why do we need to specify a name_len and not a category_goup len? Or arg_names_lens? Curious about the inconsistency of the API.
Chris, could you review the CL again? Thanks. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:29: const int kTraceNumFields = 6 + 2 * kTraceMaxNumArgs; On 2013/09/09 16:41:12, chrisha wrote: > Where does this magic number come from? Link to documentation, or a more > detailed comment? Done. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:111: } On 2013/09/09 16:41:12, chrisha wrote: > Do we want to have some DCHECK or log message if num_args > kTraceMaxNumArgs? Done. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.h:62: // Emit a trace. The event is defined by |category_group|, |name| and |type|. On 2013/09/09 16:41:12, chrisha wrote: > Emit a trace event. Done. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.h:101: // Emit a trace. On 2013/09/09 16:41:12, chrisha wrote: > Emit a trace event. Done. https://codereview.chromium.org/23934003/diff/40001/base/debug/trace_event_wi... base/debug/trace_event_win.h:120: // string. The length of |name| must be specified by |name_len|. Done. I initially kept the |name_len| parameter from the existing code. It allowed to use the size() method instead of strlen() when a std::string was provided to the ETW macros. Since ETW macros are going to disappear and are not used a lot in the code, it's not necessary to keep this optimization. Macros from trace_event.h don't accept std::string for the event name. |thread_name| and |arg_values| are available as std::string in the caller, so I use this type for the parameters to avoid a call to strlen(). The drawback of this optimization is that the interface is not consistent (char* and std::string). This method is not used directly by users of the tracing macros, so I think it's OK. What do you think? On 2013/09/09 16:41:12, chrisha wrote: > Why do we need to specify a name_len and not a category_goup len? Or > arg_names_lens? Curious about the inconsistency of the API.
https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event.h#... base/debug/trace_event.h:913: #define TRACE_EVENT_PHASE_INSTANT_OLD ('I') // Used by third party libraries. What's the benefit of listing this here? The 'I' is historic and should not be used going forward. Consumers should always use the TRACE_EVENT_PHASE_INSTANT. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:173: convertable_values[i]->AppendAsTraceFormat(&arg_str_values[i]); If I'm reading this correctly this is a potential performance issue. Is this doing the conversion of the convertables as soon as the trace event is added? The convertables are designed to do the conversion as late as possible. The reason for this is that they can be quite large (i.e. full layer tree dumps, skp files, gpu pixel dumps, etc). We don't want to do this while we're tracing as it will add overhead that can skew the trace results.
+siggi, who knows about all things ETW Care to take a look? Maybe you can address Dan's comments, or suggest a way forward for Francois?
Nice. I think ETW should have runtime control of what's traced without requiring command line flags, or about:tracing, but I can (easily) be convinced otherwise. Since the format of these new messages is incompatible with existing use of the same facility/ID, you must change IDs to get a clean break. The message format you define doesn't seem workable. I'd argue for not encoding the thread name into every single event, as that's just wasteful. Better to trace the thread name once per thread. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:1568: TraceEventETWProvider::TraceWithArgs( As ETW provides runtime control of tracing, it would (IMHO) be more appropriate to do this around line 1505, more like on ANDROID? https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:22: 0xb967ae67, 0xbb22, 0x49d7, 0x94, 0x6, 0x55, 0xd9, 0x1e, 0xe1, 0xd5, 0x60 }; If you're changing the message format, you should change this GUID to avoid breaking existing tools that parse the old message format (Sawbuck). https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:31: const int kTraceNumFields = 6 + 2 * kTraceMaxNumArgs; The templated, fixed-size MofEvent is a performance hack for logging events with a fixed number of fields. It seems like it would be more appropriate here to use a vector of fields or some mode of dynamic allocation, as this code looks like it'll be allocating a fair amount of memory anyway. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:97: event.SetField(field_index++, strlen(name) + 1, name); I'd be very interested to know what Sawbuck does with these strung out, multi-field events :). Do you use Sawbuck, have you seen how it treats these? https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:100: event.SetField(field_index++, thread_name.size() + 1, thread_name.c_str()); Is it really necessary to stuff the thread name into every event? The event already contains the process & thread IDs, and if thread names are to be found in the trace (e.g. perhaps add thread name tracing), you'd be able to find the names by correlating the traces. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:156: void TraceEventETWProvider::TraceWithArgs( I'd argue for the following: - Leave the existing trace format as-is for the simpler events already traced, unless no one is tracing those anymore. In that case, I'd argue for removing the Trace() function and all the code that goes with it. - Allocate a new set of trace event class/IDs for these more complicated events. - Make plans to break the category_groups out to different ETW facilities. - Make plans to allow ETW-driven runtime control of per-facility/category tracing, and/or implement that off the bat. - Add tracing on thread creation to trace the thread ID->name association. You can also use ETW enable bits to control the level of logging - it might not be crazy, as a case in point, to elide tracing arguments, unless when explicitly directed to capture them by an enable bit. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:173: convertable_values[i]->AppendAsTraceFormat(&arg_str_values[i]); On 2013/09/09 19:57:46, dsinclair wrote: > If I'm reading this correctly this is a potential performance issue. Is this > doing the conversion of the convertables as soon as the trace event is added? > ETW provides a binary transport, so everything will need to be serialized for logging, unfortunately. This should only happen when ETW logging has been turned on for the facility, e.g. the conversion won't take place unless someone's listening on the facility. Lines 167-168 should take care of gating this. That being said, there's a size limit to what can be stashed in a single ETW message, on the order of 8K or so IIRC. I'm not familiar with the various event types passing through here, but perhaps there's reason to filter the handled events down to some subset initially, or to skip large messages, or to skip convertible values? > The convertables are designed to do the conversion as late as possible. The > reason for this is that they can be quite large (i.e. full layer tree dumps, > skp files, gpu pixel dumps, etc). We don't want to do this while we're tracing > as it will add overhead that can skew the trace results. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.h:88: // Emit a trace event. If you're defining a message format that's incompatible with what's previously been logged under a facility/ID pair, then you should allocate either a new facility GUID or a new ID (or both) for your message. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.h:101: // If stack capture is enabled for the trace session: It's not clear to me that this message format can be uniquely parsed, as the format is stated here. How do you disambiguate between an additional trailing string or a stack trace at the end of a message?
Siggi: could you take another look at this CL? I agree with you that it would be nice to use ETW facilities to control categories to trace instead of relying on a command line flag. It raises a lot of questions, so I would like to discuss the possible solutions before implementing them. Could you add your comments in this document? https://docs.google.com/document/d/1Q5J09pTlKL9Q2loXk5HaQVvHdGxuGi77ySYvuT-rI... Thanks! https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event.h#... base/debug/trace_event.h:913: #define TRACE_EVENT_PHASE_INSTANT_OLD ('I') // Used by third party libraries. I need the value to handle events from external libraries correctly. I moved the definition in EtwEventTypeFromInternalEventType() (trace_event_win.cc). On 2013/09/09 19:57:46, dsinclair wrote: > What's the benefit of listing this here? The 'I' is historic and should not be > used going forward. Consumers should always use the TRACE_EVENT_PHASE_INSTANT. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:1568: TraceEventETWProvider::TraceWithArgs( On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > As ETW provides runtime control of tracing, it would (IMHO) be more appropriate > to do this around line 1505, more like on ANDROID? Done. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:22: 0xb967ae67, 0xbb22, 0x49d7, 0x94, 0x6, 0x55, 0xd9, 0x1e, 0xe1, 0xd5, 0x60 }; On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > If you're changing the message format, you should change this GUID to avoid > breaking existing tools that parse the old message format (Sawbuck). Done. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:31: const int kTraceNumFields = 6 + 2 * kTraceMaxNumArgs; The difficulty is that the MOF fields must immediately follow the header in memory. A std::vector is not suitable for that. Did you have in mind something like below? I don't like to have this new class in addition to EtwMOFEvent (which will still be used by logging_win.cc and event_trace_provider.cc) to avoid 6 unused void* on the stack (2*(kTraceMaxNumArgs=2) + 2 pointers for stack trace). These extra pointers don't change the size of the resulting ETW event. What do you think? If you prefer dynamic memory, I can make it. class EtwDynamicMOFEvent { public: EtwDynamicMOFEvent(const EtwEventClass& event_class, EtwEventType type, EtwEventLevel level, size_t nb_fields) : current_field_index_(0), nb_fields_(nb_fields) { size_t event_size = sizeof(EVENT_TRACE_HEADER) + nb_fields_ * sizeof(MOF_FIELD); header_ = static_cast<EVENT_TRACE_HEADER*>(malloc(event_size)); memset(header_, 0, event_size); header_->Size = event_size; header_->Guid = event_class; header_->Class.Type = type; header_->Class.Level = level; header_->Flags = WNODE_FLAG_TRACED_GUID | WNODE_FLAG_USE_MOF_PTR; fields_ = nb_fields_ ? header_ + sizeof(EVENT_TRACE_HEADER) : NULL; } ~EtwDynamicMOFEvent() { free(header_); } void AddField(size_t size, const void *data) { DCHECK(current_field_index_ < nb_fields_); fields_[current_field_index_].DataPtr = reinterpret_cast<ULONG64>(data); fields_[current_field_index_].Length = static_cast<ULONG>(size); ++current_field_index; } ETW_TRACE_HEADER* get() { return header_; } private: EVENT_TRACE_HEADER* header_; MOF_FIELD_* fields_; size_t current_field_index_; size_t nb_fields_; DISALLOW_COPY_AND_ASSIGN(EtwDynamicMOFEvent); }; On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > The templated, fixed-size MofEvent is a performance hack for logging events with > a fixed number of fields. > It seems like it would be more appropriate here to use a vector of fields or > some mode of dynamic allocation, as this code looks like it'll be allocating a > fair amount of memory anyway. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:97: event.SetField(field_index++, strlen(name) + 1, name); Sawbuck doesn't parse these messages correctly. With the old GUID, the "category" was used as the "extra" string and other fields were ignored (no crash). With the new GUIDs, Sawbuck doesn't show the new events. On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > I'd be very interested to know what Sawbuck does with these strung out, > multi-field events :). Do you use Sawbuck, have you seen how it treats these? https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:100: event.SetField(field_index++, thread_name.size() + 1, thread_name.c_str()); On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > Is it really necessary to stuff the thread name into every event? > The event already contains the process & thread IDs, and if thread names are to > be found in the trace (e.g. perhaps add thread name tracing), you'd be able to > find the names by correlating the traces. Done. I've started a distinct CL for this: https://codereview.chromium.org/23510008/ https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:156: void TraceEventETWProvider::TraceWithArgs( - OK, I kept the existing trace format as-is for events that use the TRACE_EVENT_*_ETW macros. These macros are not used a lot, so I plan to make distinct CLs to replace them with the more generic macros from trace_event.h and then to remove everything related to the old format. - New GUIDs: done. - Categories: It would be nice, but it's complex to achieve. See the message associated with the publication of these comments. - Thread name event: OK, will be done in a distinct CL (https://codereview.chromium.org/23510008/) On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > I'd argue for the following: > - Leave the existing trace format as-is for the simpler events already traced, > unless no one is tracing those anymore. > In that case, I'd argue for removing the Trace() function and all the code > that goes with it. > - Allocate a new set of trace event class/IDs for these more complicated events. > - Make plans to break the category_groups out to different ETW facilities. > - Make plans to allow ETW-driven runtime control of per-facility/category > tracing, and/or implement that off the bat. > - Add tracing on thread creation to trace the thread ID->name association. > > You can also use ETW enable bits to control the level of logging - it might not > be crazy, as a case in point, to elide tracing arguments, unless when explicitly > directed to capture them by an enable bit. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:173: convertable_values[i]->AppendAsTraceFormat(&arg_str_values[i]); According to http://msdn.microsoft.com/en-us/library/windows/desktop/aa363668(v=vs.85).aspx, I think the size limit is 64K. I've added a way to skip convertible values. On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > On 2013/09/09 19:57:46, dsinclair wrote: > > If I'm reading this correctly this is a potential performance issue. Is this > > doing the conversion of the convertables as soon as the trace event is added? > > > > ETW provides a binary transport, so everything will need to be serialized for > logging, unfortunately. > This should only happen when ETW logging has been turned on for the facility, > e.g. the conversion won't take place unless someone's listening on the facility. > Lines 167-168 should take care of gating this. > > That being said, there's a size limit to what can be stashed in a single ETW > message, on the order of 8K or so IIRC. > I'm not familiar with the various event types passing through here, but perhaps > there's reason to filter the handled events down to some subset initially, or to > skip large messages, or to skip convertible values? > > > The convertables are designed to do the conversion as late as possible. The > > reason for this is that they can be quite large (i.e. full layer tree dumps, > > skp files, gpu pixel dumps, etc). We don't want to do this while we're > tracing > > as it will add overhead that can skew the trace results. > https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.h:88: // Emit a trace event. On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > If you're defining a message format that's incompatible with what's previously > been logged under a facility/ID pair, then you should allocate either a new > facility GUID or a new ID (or both) for your message. Done. https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_wi... base/debug/trace_event_win.h:101: // If stack capture is enabled for the trace session: The new format can be parsed using this schema: https://docs.google.com/file/d/0B3wWyRavE9yeVEcyV1B2SUpJT1k/edit?usp=sharing mofcomp -N:root\wmi chrome_event_declaration.mof tracerpt [trace file].etl Tested for all combination of number of arguments / capture stack trace enabled or disabled. Gives an output like that: <Event xmlns="http://schemas.microsoft.com/win/2004/08/events/event"> <System> <Provider Guid="{3dada31d-19ef-4dc1-b345-037927193422}" /> <EventID>0</EventID> <Version>0</Version> <Level>4</Level> <Task>0</Task> <Opcode>16</Opcode> <Keywords>0x0</Keywords> <TimeCreated SystemTime="2013-09-15T11:27:39.016608600Z" /> <Correlation ActivityID="{00000000-0000-0000-0000-000000000000}" /> <Execution ProcessID="8180" ThreadID="4516" ProcessorID="5" KernelTime="0" UserTime="30" /> <Channel /> <Computer /> </System> <EventData> <Data Name="name">MessageLoop::RunTask</Data> <Data Name="id">0x0</Data> <Data Name="category">task</Data> <Data Name="arg1_label">src_file</Data> <Data Name="arg1_value">d:\\chrome\\src\\ipc\\ipc_channel_proxy.cc</Data> <Data Name="arg2_label">src_func</Data> <Data Name="arg2_value">IPC::ChannelProxy::Send</Data> <Data Name="stack_size"> 21</Data> <Data Name="stack_pointers">0x100CF4DA</Data> <Data Name="stack_pointers">0x100CF9C9</Data> ... </EventData> <RenderingInfo Culture="en-US"> <Opcode>ChromeBegin</Opcode> <Provider>ChromeProvider</Provider> </RenderingInfo> <ExtendedTracingInfo xmlns="http://schemas.microsoft.com/win/2004/08/events/trace"> <EventGuid>{4b82d4e7-f6d5-4e64-8855-f55ed5f8f457}</EventGuid> </ExtendedTracingInfo> </Event> On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > It's not clear to me that this message format can be uniquely parsed, as the > format is stated here. > How do you disambiguate between an additional trailing string or a stack trace > at the end of a message? https://codereview.chromium.org/23934003/diff/62001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/62001/base/debug/trace_event_wi... base/debug/trace_event_win.h:148: BASE_EXPORT extern const GUID kTraceEventClass[]; Having multiple GUIDs makes it easy to parse the trace file using a schema (https://docs.google.com/file/d/0B3wWyRavE9yeVEcyV1B2SUpJT1k/edit?usp=sharing). I've not been able to achieve the same result by including the "num_args" in the trace. I would need to find a way to create an array of (label+value) -> not an array of labels followed by an array of values. Defining a new class and using it as a type for the array didn't work. See documentation http://msdn.microsoft.com/en-us/library/aa364100(v=vs.85).aspx
Siggi: could you take another look at this CL? I agree with you that it would be nice to use ETW facilities to control categories to trace instead of relying on a command line flag. It raises a lot of questions, so I would like to discuss the possible solutions before implementing them. Could you add your comments in this document?https://docs.google.com/document/d/1Q5J09pTlKL9Q2loXk5HaQVvHdGxuGi77ySYvuT-rI... Thanks! https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_wi... base/debug/trace_event_win.h:146: BASE_EXPORT extern const GUID kTraceEventClass[]; Having multiple GUIDs makes it easy to parse the trace file using a schema (https://docs.google.com/file/d/0B3wWyRavE9yeVEcyV1B2SUpJT1k/edit?usp=sharing). I've not been able to achieve the same result by including the "num_args" in the trace. I would need to find a way to create an array of (label+value) -> not an array of labels followed by an array of values. Defining a new class and using it as a type for the array didn't work. See documentation http://msdn.microsoft.com/en-us/library/aa364100(v=vs.85).aspx
On Sun, Sep 15, 2013 at 8:51 PM, <fdoray@chromium.org> wrote: > Siggi: could you take another look at this CL? > > Hey, I'm a little swamped right now - it'll be a couple of days until I can do this justice... > I agree with you that it would be nice to use ETW facilities to control > categories to trace instead of relying on a command line flag. It raises a > lot > of questions, so I would like to discuss the possible solutions before > implementing them. Could you add your comments in this document? > https://docs.google.com/**document/d/**1Q5J09pTlKL9Q2loXk5HaQVvHdGxuG** > i77ySYvuT-rIpM/edit?usp=**sharing<https://docs.google.com/document/d/1Q5J09pTlKL9Q2loXk5HaQVvHdGxuGi77ySYvuT-rIpM/edit?usp=sharing> > > Thanks! > > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event.h<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event.h> > File base/debug/trace_event.h (right): > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event.h#newcode913<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event.h#newcode913> > base/debug/trace_event.h:913: #define TRACE_EVENT_PHASE_INSTANT_OLD > ('I') // Used by third party libraries. > I need the value to handle events from external libraries correctly. I > moved the definition in EtwEventTypeFromInternalEventT**ype() > (trace_event_win.cc). > > > On 2013/09/09 19:57:46, dsinclair wrote: > >> What's the benefit of listing this here? The 'I' is historic and >> > should not be > >> used going forward. Consumers should always use the >> > TRACE_EVENT_PHASE_INSTANT. > > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_impl.cc<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_impl.cc> > File base/debug/trace_event_impl.cc (right): > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_impl.cc#**newcode1568<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_impl.cc#newcode1568> > base/debug/trace_event_impl.**cc:1568: > TraceEventETWProvider::**TraceWithArgs( > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> As ETW provides runtime control of tracing, it would (IMHO) be more >> > appropriate > >> to do this around line 1505, more like on ANDROID? >> > > Done. > > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.cc<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.cc> > File base/debug/trace_event_win.cc (right): > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.cc#**newcode22<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.cc#newcode22> > base/debug/trace_event_win.cc:**22: 0xb967ae67, 0xbb22, 0x49d7, 0x94, 0x6, > 0x55, 0xd9, 0x1e, 0xe1, 0xd5, 0x60 }; > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> If you're changing the message format, you should change this GUID to >> > avoid > >> breaking existing tools that parse the old message format (Sawbuck). >> > > Done. > > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.cc#**newcode31<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.cc#newcode31> > base/debug/trace_event_win.cc:**31: const int kTraceNumFields = 6 + 2 * > kTraceMaxNumArgs; > The difficulty is that the MOF fields must immediately follow the header > in memory. A std::vector is not suitable for that. Did you have in mind > something like below? I don't like to have this new class in addition to > EtwMOFEvent (which will still be used by logging_win.cc and > event_trace_provider.cc) to avoid 6 unused void* on the stack > (2*(kTraceMaxNumArgs=2) + 2 pointers for stack trace). These extra > pointers don't change the size of the resulting ETW event. What do you > think? If you prefer dynamic memory, I can make it. > > class EtwDynamicMOFEvent { > public: > EtwDynamicMOFEvent(const EtwEventClass& event_class, EtwEventType > type, > EtwEventLevel level, size_t nb_fields) > : current_field_index_(0), nb_fields_(nb_fields) { > size_t event_size = > sizeof(EVENT_TRACE_HEADER) + nb_fields_ * sizeof(MOF_FIELD); > header_ = static_cast<EVENT_TRACE_**HEADER*>(malloc(event_size)); > memset(header_, 0, event_size); > header_->Size = event_size; > header_->Guid = event_class; > header_->Class.Type = type; > header_->Class.Level = level; > header_->Flags = WNODE_FLAG_TRACED_GUID | WNODE_FLAG_USE_MOF_PTR; > > fields_ = nb_fields_ ? header_ + sizeof(EVENT_TRACE_HEADER) : NULL; > } > > ~EtwDynamicMOFEvent() { > free(header_); > } > > void AddField(size_t size, const void *data) { > DCHECK(current_field_index_ < nb_fields_); > fields_[current_field_index_].**DataPtr = > reinterpret_cast<ULONG64>(**data); > fields_[current_field_index_].**Length = static_cast<ULONG>(size); > ++current_field_index; > } > > ETW_TRACE_HEADER* get() { return header_; } > > private: > EVENT_TRACE_HEADER* header_; > MOF_FIELD_* fields_; > size_t current_field_index_; > size_t nb_fields_; > > DISALLOW_COPY_AND_ASSIGN(**EtwDynamicMOFEvent); > }; > > > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> The templated, fixed-size MofEvent is a performance hack for logging >> > events with > >> a fixed number of fields. >> It seems like it would be more appropriate here to use a vector of >> > fields or > >> some mode of dynamic allocation, as this code looks like it'll be >> > allocating a > >> fair amount of memory anyway. >> > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.cc#**newcode97<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.cc#newcode97> > base/debug/trace_event_win.cc:**97: event.SetField(field_index++, > strlen(name) + 1, name); > Sawbuck doesn't parse these messages correctly. With the old GUID, the > "category" was used as the "extra" string and other fields were ignored > (no crash). With the new GUIDs, Sawbuck doesn't show the new events. > > > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> I'd be very interested to know what Sawbuck does with these strung >> > out, > >> multi-field events :). Do you use Sawbuck, have you seen how it treats >> > these? > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.cc#**newcode100<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.cc#newcode100> > base/debug/trace_event_win.cc:**100: event.SetField(field_index++, > thread_name.size() + 1, thread_name.c_str()); > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> Is it really necessary to stuff the thread name into every event? >> The event already contains the process & thread IDs, and if thread >> > names are to > >> be found in the trace (e.g. perhaps add thread name tracing), you'd be >> > able to > >> find the names by correlating the traces. >> > > Done. I've started a distinct CL for this: > https://codereview.chromium.**org/23510008/<https://codereview.chromium.org/2... > > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.cc#**newcode156<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.cc#newcode156> > base/debug/trace_event_win.cc:**156: void > TraceEventETWProvider::**TraceWithArgs( > - OK, I kept the existing trace format as-is for events that use the > TRACE_EVENT_*_ETW macros. These macros are not used a lot, so I plan to > make distinct CLs to replace them with the more generic macros from > trace_event.h and then to remove everything related to the old format. > - New GUIDs: done. > - Categories: It would be nice, but it's complex to achieve. See the > message associated with the publication of these comments. > - Thread name event: OK, will be done in a distinct CL > (https://codereview.chromium.**org/23510008/<https://codereview.chromium.org/2... > ) > > > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> I'd argue for the following: >> - Leave the existing trace format as-is for the simpler events already >> > traced, > >> unless no one is tracing those anymore. >> In that case, I'd argue for removing the Trace() function and all >> > the code > >> that goes with it. >> - Allocate a new set of trace event class/IDs for these more >> > complicated events. > >> - Make plans to break the category_groups out to different ETW >> > facilities. > >> - Make plans to allow ETW-driven runtime control of >> > per-facility/category > >> tracing, and/or implement that off the bat. >> - Add tracing on thread creation to trace the thread ID->name >> > association. > > You can also use ETW enable bits to control the level of logging - it >> > might not > >> be crazy, as a case in point, to elide tracing arguments, unless when >> > explicitly > >> directed to capture them by an enable bit. >> > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.cc#**newcode173<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.cc#newcode173> > base/debug/trace_event_win.cc:**173: > convertable_values[i]->**AppendAsTraceFormat(&arg_str_**values[i]); > According to > http://msdn.microsoft.com/en-**us/library/windows/desktop/** > aa363668(v=vs.85).aspx<http://msdn.microsoft.com/en-us/library/windows/desktop/aa363668(v=vs.85).aspx> > , > I think the size limit is 64K. I've added a way to skip convertible > values. > > > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> On 2013/09/09 19:57:46, dsinclair wrote: >> > If I'm reading this correctly this is a potential performance issue. >> > Is this > >> > doing the conversion of the convertables as soon as the trace event >> > is added? > >> > >> > > ETW provides a binary transport, so everything will need to be >> > serialized for > >> logging, unfortunately. >> This should only happen when ETW logging has been turned on for the >> > facility, > >> e.g. the conversion won't take place unless someone's listening on the >> > facility. > >> Lines 167-168 should take care of gating this. >> > > That being said, there's a size limit to what can be stashed in a >> > single ETW > >> message, on the order of 8K or so IIRC. >> I'm not familiar with the various event types passing through here, >> > but perhaps > >> there's reason to filter the handled events down to some subset >> > initially, or to > >> skip large messages, or to skip convertible values? >> > > > The convertables are designed to do the conversion as late as >> > possible. The > >> > reason for this is that they can be quite large (i.e. full layer >> > tree dumps, > >> > skp files, gpu pixel dumps, etc). We don't want to do this while >> > we're > >> tracing >> > as it will add overhead that can skew the trace results. >> > > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.h<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.h> > File base/debug/trace_event_win.h (right): > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.h#**newcode88<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.h#newcode88> > base/debug/trace_event_win.h:**88: // Emit a trace event. > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> If you're defining a message format that's incompatible with what's >> > previously > >> been logged under a facility/ID pair, then you should allocate either >> > a new > >> facility GUID or a new ID (or both) for your message. >> > > Done. > > > https://codereview.chromium.**org/23934003/diff/51001/base/** > debug/trace_event_win.h#**newcode101<https://codereview.chromium.org/23934003/diff/51001/base/debug/trace_event_win.h#newcode101> > base/debug/trace_event_win.h:**101: // If stack capture is enabled for the > trace session: > The new format can be parsed using this schema: > https://docs.google.com/file/**d/**0B3wWyRavE9yeVEcyV1B2SUpJT1k/** > edit?usp=sharing<https://docs.google.com/file/d/0B3wWyRavE9yeVEcyV1B2SUpJT1k/edit?usp=sharing> > > mofcomp -N:root\wmi chrome_event_declaration.mof > tracerpt [trace file].etl > > Tested for all combination of number of arguments / capture stack trace > enabled or disabled. Gives an output like that: > <Event xmlns="http://schemas.**microsoft.com/win/2004/08/**events/event<http://schemas.microsoft.com/win/2004/08/events/event> > "> > <System> > <Provider Guid="{3dada31d-19ef-4dc1-**b345-037927193422}" > /> > <EventID>0</EventID> > <Version>0</Version> > <Level>4</Level> > <Task>0</Task> > <Opcode>16</Opcode> > <Keywords>0x0</Keywords> > <TimeCreated SystemTime="2013-09-15T11:27:**39.016608600Z" > /> > <Correlation ActivityID="{00000000-0000-**0000-0000-000000000000}" > /> > <Execution ProcessID="8180" ThreadID="4516" ProcessorID="5" > KernelTime="0" UserTime="30" /> > <Channel /> > <Computer /> > </System> > <EventData> > <Data Name="name">MessageLoop::**RunTask</Data> > <Data Name="id">0x0</Data> > <Data Name="category">task</Data> > <Data Name="arg1_label">src_file</**Data> > <Data > Name="arg1_value">d:\\chrome\\**src\\ipc\\ipc_channel_proxy.**cc</Data> > <Data Name="arg2_label">src_func</**Data> > <Data Name="arg2_value">IPC::**ChannelProxy::Send</Data> > <Data Name="stack_size"> 21</Data> > <Data Name="stack_pointers">**0x100CF4DA</Data> > <Data Name="stack_pointers">**0x100CF9C9</Data> > ... > </EventData> > <RenderingInfo Culture="en-US"> > <Opcode>ChromeBegin</Opcode> > <Provider>ChromeProvider</**Provider> > </RenderingInfo> > <ExtendedTracingInfo > xmlns="http://schemas.**microsoft.com/win/2004/08/**events/trace<http://schemas.microsoft.com/win/2004/08/events/trace> > "> > <EventGuid>{4b82d4e7-f6d5-**4e64-8855-f55ed5f8f457}</** > EventGuid> > </ExtendedTracingInfo> > </Event> > > > > On 2013/09/10 21:15:54, Sigurður Ásgeirsson wrote: > >> It's not clear to me that this message format can be uniquely parsed, >> > as the > >> format is stated here. >> How do you disambiguate between an additional trailing string or a >> > stack trace > >> at the end of a message? >> > > https://codereview.chromium.**org/23934003/diff/62001/base/** > debug/trace_event_win.h<https://codereview.chromium.org/23934003/diff/62001/base/debug/trace_event_win.h> > File base/debug/trace_event_win.h (right): > > https://codereview.chromium.**org/23934003/diff/62001/base/** > debug/trace_event_win.h#**newcode148<https://codereview.chromium.org/23934003/diff/62001/base/debug/trace_event_win.h#newcode148> > base/debug/trace_event_win.h:**148: BASE_EXPORT extern const GUID > kTraceEventClass[]; > Having multiple GUIDs makes it easy to parse the trace file using a > schema > (https://docs.google.com/file/**d/**0B3wWyRavE9yeVEcyV1B2SUpJT1k/** > edit?usp=sharing<https://docs.google.com/file/d/0B3wWyRavE9yeVEcyV1B2SUpJT1k/edit?usp=sharing> > ). > I've not been able to achieve the same result by including the > "num_args" in the trace. I would need to find a way to create an array > of (label+value) -> not an array of labels followed by an array of > values. Defining a new class and using it as a type for the array didn't > work. See documentation > http://msdn.microsoft.com/en-**us/library/aa364100(v=vs.85).**aspx<http://msd... > > https://codereview.chromium.**org/23934003/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
drive-by. https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:208: void* backtrace[32]; This declaration could be move in the scope below. https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_wi... base/debug/trace_event_win.h:146: BASE_EXPORT extern const GUID kTraceEventClass[]; I think you could use one GUID for the provider, and use multiple 'EventType' in the Mof file (see 'Event type MOF class qualifiers' in your link). On 2013/09/16 00:59:08, fdoray wrote: > Having multiple GUIDs makes it easy to parse the trace file using a schema > (https://docs.google.com/file/d/0B3wWyRavE9yeVEcyV1B2SUpJT1k/edit?usp=sharing). > I've not been able to achieve the same result by including the "num_args" in the > trace. I would need to find a way to create an array of (label+value) -> not an > array of labels followed by an array of values. Defining a new class and using > it as a type for the array didn't work. See documentation > http://msdn.microsoft.com/en-us/library/aa364100%28v=vs.85%29.aspx
https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_wi... File base/debug/trace_event_win.cc (right): https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_wi... base/debug/trace_event_win.cc:208: void* backtrace[32]; No, the array must still exist when the event is logged (line 222). On 2013/09/23 06:46:12, etienneb wrote: > This declaration could be move in the scope below. https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_win.h File base/debug/trace_event_win.h (right): https://codereview.chromium.org/23934003/diff/70001/base/debug/trace_event_wi... base/debug/trace_event_win.h:146: BASE_EXPORT extern const GUID kTraceEventClass[]; On 2013/09/23 06:46:12, etienneb wrote: > I think you could use one GUID for the provider, and use multiple 'EventType' in > the Mof file (see 'Event type MOF class qualifiers' in your link). > > On 2013/09/16 00:59:08, fdoray wrote: > > Having multiple GUIDs makes it easy to parse the trace file using a schema > > > (https://docs.google.com/file/d/0B3wWyRavE9yeVEcyV1B2SUpJT1k/edit?usp=sharing). > > I've not been able to achieve the same result by including the "num_args" in > the > > trace. I would need to find a way to create an array of (label+value) -> not > an > > array of labels followed by an array of values. Defining a new class and using > > it as a type for the array didn't work. See documentation > > http://msdn.microsoft.com/en-us/library/aa364100%2528v=vs.85%2529.aspx > Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
