Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11366109:
Adding raw tracing to trace framework. (Closed)
|
Created:
8 years, 1 month ago by dsinclair Modified:
7 years, 10 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding raw tracing to trace framework.
With some types of tracing you do not want to use the system time as the recorded event time. In those cases, we want to provide a timestamp that will be used to display the event.
In a similar vein, the thread which is outputting the event may not be the thread where the event happened so we also allow an explicit thread_id to be provided.
BUG=111509
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182768
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add unit tests for the new trace macros #Patch Set 3 : Switch to using _COPY methods. #Patch Set 4 : Rename from RAW to EXPLICIT; drop thread_name in favor of thread_id #Patch Set 5 : Rebased onto master. #Patch Set 6 : Set the thread_name now that we're providing the id in the _explicit calls #Patch Set 7 : Use ASYNC event type; drop thread_id as it isn't needed anymore #Patch Set 8 : #Patch Set 9 : #
Total comments: 4
Patch Set 10 : #Patch Set 11 : Use ThreadIdNameManager to retrieve correct thread names. #
Total comments: 4
Patch Set 12 : Rebasing with master #Patch Set 13 : Fixup WebKit Glue trace event call. #Patch Set 14 : Changing AddTraceEvent API to always accept a tid and timestamp. #
Total comments: 2
Patch Set 15 : Hide GetTimeOffset() #Patch Set 16 : #Patch Set 17 : Mergin header changes from master. #Patch Set 18 : Rebased, trying to fix apply issues. #Patch Set 19 : Fix typo. #
Total comments: 10
Patch Set 20 : #Patch Set 21 : #
Total comments: 4
Patch Set 22 : #Patch Set 23 : style fixes. #
Total comments: 1
Messages
Total messages: 44 (0 generated)
I've added the unit tests for the new macros. As to the thread_name. I've left it in at the moment. It's currently using a negative tid internal to the trace system but I'm passing the name through so that on about:tracing you see something like GPUInfo instead of -1. It will allocate a new negative tid for each new name you pass through. So, we could in theory have CompositorGPUInfo, RenderGPUInfo, SomeOtherRawEventInfo, etc. If we don't care about seeing a nice name on about:tracing I can change it to receive a tid instead of thread_name. Although, I'm making the assumption that we'll want to be able to see the GPU info broken out for different parts of the system, if we just want to see it all combined, then I could drop the tid from the API, use -1 internally, and just inject a fake name of GPUInfo into the metadata. I'm not sure I understand why exposing the AddRaw is such a big no-no. It's, essentially, the same as the other Add call. Is it just the fact that the timestamp, or the thread_name is visible with this call? (Or, is the other Add call a no-no as well that's been grandfathered in?) https://codereview.chromium.org/11366109/diff/1/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/1/base/debug/trace_event.h#newc... base/debug/trace_event.h:308: #define TRACE_RAW_EVENT_BETWEEN0(category, thread_name, name, start, end) \ This was added purely for convenience. At the point where I was using the API I had both begin and end values. This seemed clearer to me. Can remove if you think it's just clutter. https://codereview.chromium.org/11366109/diff/1/base/debug/trace_event.h#newc... base/debug/trace_event.h:625: // Add a trace event to the platform tracing system. Returns thresholdBeginId If this isn't part of the API then I'm not going to be able to add the trace events? Is it due to the timestamp and thread_name that you'd like this to not appear as part of the API? Other then those, this is almost the same as the non-raw event method.
+jbates, I'd like his feedback before I review this. My general guidance is to
be as minimally intrusive as needed. For instance, you could make a
base::Thread("Gpu"). Leave it running but off... that'll create a thread for
you. Then use that tid in your gpu traces. All you need then is a
TRACE_EVENT_BEGIN_EXPLICIT that accepts an explicit tid and timestamp.
As far as exposing raw, please remember, on some platforms, trace_event.h
doens't even go into trace_event_impl.h -- e.g. android. There,it redirects into
**android's** tracing system. You'll need this to work with those atrace
backends too (see atrace in the headers).
On 2012/11/07 02:45:08, nduca wrote:
> +jbates, I'd like his feedback before I review this. My general guidance is to
> be as minimally intrusive as needed. For instance, you could make a
> base::Thread("Gpu"). Leave it running but off... that'll create a thread for
> you. Then use that tid in your gpu traces. All you need then is a
> TRACE_EVENT_BEGIN_EXPLICIT that accepts an explicit tid and timestamp.
Is it possible for us to query the trace framework to determine when tracing has
started/stopped? i.e. would we be able to create/kill this fake thread so it
only exists when tracing is executing?
If this gets used by something other then GPU tracing, do we want to start
accumulating these fake threads in the system?
> As far as exposing raw, please remember, on some platforms, trace_event.h
> doens't even go into trace_event_impl.h -- e.g. android. There,it redirects
into
> **android's** tracing system. You'll need this to work with those atrace
> backends too (see atrace in the headers).
Ah, I didn't know there were different backends for the tracing system. So,
anything I add in trace_event.h would have to have multiple implementations.
Is it just android, or are there others as well?
> Is it possible for us to query the trace framework to determine when tracing has > started/stopped? You can determine if a category is active. I forget how but careful reading of the source can show you how. So just create the thread on demand, and never delete it. That should be good enough. > If this gets used by something other then GPU tracing, do we want to start > accumulating these fake threads in the system? Sure, why not? Think simple, okay? We can refine later. > Ah, I didn't know there were different backends for the tracing system. So, > anything I add in trace_event.h would have to have multiple implementations. Yeah. Just android.
On 2012/11/07 18:32:16, nduca wrote: > > Is it possible for us to query the trace framework to determine when tracing > has > > started/stopped? > > You can determine if a category is active. I forget how but careful reading of > the source can show you how. > > So just create the thread on demand, and never delete it. That should be good > enough. Ok, I've changed thread_name into a thread_id and, from your comments, changed from _RAW to _EXPLICIT. > > > Ah, I didn't know there were different backends for the tracing system. So, > > anything I add in trace_event.h would have to have multiple implementations. > Yeah. Just android. Looking at the atrace stuff, and if I'm understanding correctly, the atrace is triggered inside the trace_event_impl.cc inside a #if defined(OS_ANDROID) as part of the AddTraceEvent() call. So, this current code would work fine on android, it just means that anything that's added as _EXPLICIT will not show up in the atrace framework. I can dig into atrace to see if there is a way to provide the timestamp along with the other tracing information, but as it stands, this code should work I believe.
> I can dig into atrace to see if there is a way to provide the timestamp along > with the other tracing information, but as it stands, this code should work I > believe. Yeah, if you could, that'd be good. Effectively, all the atrace macros do is they write a string to /proc/kernel/debug/trace_marker. That the gets pushed verbatim into the trace file, which is parsed by about:tracing's UI by trace-viewer. Point being, you can write a custom record to the marker in this, with a new prefix. Then, modify http://code.google.com/p/trace-viewer/source/browse/trunk/src/linux_perf_pars... to recognize those new events, using the process in http://code.google.com/p/trace-viewer/wiki/Contributing. Figure out the format first by writing a patch to the viewer and getting it reviewed by @jgennis. Then go back and make the changes here.
On 2012/11/07 23:14:02, nduca wrote: > > I can dig into atrace to see if there is a way to provide the timestamp along > > with the other tracing information, but as it stands, this code should work I > > believe. > > Yeah, if you could, that'd be good. Effectively, all the atrace macros do is > they write a string to /proc/kernel/debug/trace_marker. That the gets pushed > verbatim into the trace file, which is parsed by about:tracing's UI by > trace-viewer. > > Point being, you can write a custom record to the marker in this, with a new > prefix. Then, modify > http://code.google.com/p/trace-viewer/source/browse/trunk/src/linux_perf_pars... > to recognize those new events, using the process in > http://code.google.com/p/trace-viewer/wiki/Contributing. Figure out the format > first by writing a patch to the viewer and getting it reviewed by @jgennis. Then > go back and make the changes here. Taking a look now. In the mean time, quick question. Since Android pushes to the file and parses that file in the viewer, why do we still do all the regular trace processing on Android? Why is there no return inside the #if defined(OS_ANDROID) block?
We support both on android
On 2012/11/07 23:14:02, nduca wrote: > Yeah, if you could, that'd be good. Effectively, all the atrace macros do is > they write a string to /proc/kernel/debug/trace_marker. That the gets pushed > verbatim into the trace file, which is parsed by about:tracing's UI by > trace-viewer. Since the Android code can be put into a CL at a later point, would it be possible to move forward and get this CL committed so we can unblock the other GPU trace work I'm doing? When the Android requirements are hammered out I'll then make a new CL which will output the appropriate data.
I think we should take some time to parse @jgennis' ideas and comments around tiling architectures, actually. I hate being the one to change my mind, but it may be when we consider the case of tiled gpus, it may be that the trace data we insert into the log is much more specialized. In such a case, it may be that we should instead expose some simplified way to add raw trace events --- just like you originally proposed. :$ The thing I'd like to see before we go landing more of this proposal is a common design doc or email on how we do gpu-side profiling in chrome & tracing, with a story for both forward and tiled renderers. Once we have a clear vision we're working toward, then we can rationalize patches against that vision.
On 2012/11/21 18:20:11, nduca wrote: > I think we should take some time to parse @jgennis' ideas and comments around > tiling architectures, actually. I hate being the one to change my mind, but it > may be when we consider the case of tiled gpus, it may be that the trace data we > insert into the log is much more specialized. In such a case, it may be that we > should instead expose some simplified way to add raw trace events --- just like > you originally proposed. :$ > > The thing I'd like to see before we go landing more of this proposal is a common > design doc or email on how we do gpu-side profiling in chrome & tracing, with a > story for both forward and tiled renderers. Once we have a clear vision we're > working toward, then we can rationalize patches against that vision. Document created: https://docs.google.com/a/chromium.org/document/d/1_nmAkOUezGFxiEgR-VZJfWEz9D...
PTAL. A few changes, dropped the thread_id as we're outputting from our own thread now on the GPU side. Cleaned up some other API things along the way.
https://codereview.chromium.org/11366109/diff/24002/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/24002/base/debug/trace_event.h#... base/debug/trace_event.h:270: #define TRACE_EVENT_COPY_BEGIN_EXPLICIT0(category, name, id, at) \ at->ts? we're not using a custom |thread_name| anymore? if you BEGIN_WITH_EXPLICIT_TIMESTAMP? Add the non-copying forms too. https://codereview.chromium.org/11366109/diff/24002/base/debug/trace_event.h#... base/debug/trace_event.h:627: #define TRACE_EVENT_API_ADD_TRACE_EVENT_EXPLICIT \ what requires this one?
and, at -> ts
https://codereview.chromium.org/11366109/diff/24002/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/24002/base/debug/trace_event.h#... base/debug/trace_event.h:270: #define TRACE_EVENT_COPY_BEGIN_EXPLICIT0(category, name, id, at) \ On 2012/12/04 05:09:38, nduca wrote: > at->ts? Done > we're not using a custom |thread_name| anymore? > if you BEGIN_WITH_EXPLICIT_TIMESTAMP? No, we ended up using a thread to handle the outputting for us. I couldn't figure out any way to go from a thread id to a thread name without being on the actual thread. Using PostTask it was actually pretty easy to get the output working on the thread. > > Add the non-copying forms too. Done https://codereview.chromium.org/11366109/diff/24002/base/debug/trace_event.h#... base/debug/trace_event.h:627: #define TRACE_EVENT_API_ADD_TRACE_EVENT_EXPLICIT \ On 2012/12/04 05:09:38, nduca wrote: > what requires this one? It's used on line 992 in AddTraceEventExplicit().
Ok, I get it now. Posting tasks to a thread seems a bit performance-meh. Do you mind giving an explicit_tid_explicit_timestamp variant a try? We still need the thread, but it can be idle...
(Sorry to be churning you --- you're doing a great job, whereas I'm doing a spotty job at giving you good up-front review feedback. My sincere apologies)
On 2012/12/04 17:00:42, nduca wrote: > Ok, I get it now. > > Posting tasks to a thread seems a bit performance-meh. Do you mind giving an > explicit_tid_explicit_timestamp variant a try? We still need the thread, but it > can be idle... How do I, given a thread_id, get the thread_name in the tracing framework? I couldn't get this part figured out when I was passing the thread_id which is why we switched to the outputter thread.
Also, the GPU code that is using this API is only outputting when GPU tracing is enabled, so the performance hit should be minimal. Similarly the GL_ARB_timer_query calls are only made when tracing is enabled.
> How do I, given a thread_id, get the thread_name in the tracing framework?
Why do you need it?
This is what I want:
base:::Thread thread("gpu hardware");
TRACE_EVENT_BEGIN_WITH_EXPLICIT_TS("foo", "bar", thread->thread_id(), 100);
TRACE_EVENT_END_WITH_EXPLICIT_TS("foo", "bar", thread->thread_id(), 120);
That would work, no?
And, just because its only slow when its on is no excuse. You're causing a
kernel context switch just to push performance data. That disrupts the
performance measurement that we're trying to do. Thats not good.
On 2012/12/04 20:40:33, nduca wrote:
> > How do I, given a thread_id, get the thread_name in the tracing framework?
> Why do you need it?
>
> This is what I want:
>
> base:::Thread thread("gpu hardware");
> TRACE_EVENT_BEGIN_WITH_EXPLICIT_TS("foo", "bar", thread->thread_id(), 100);
> TRACE_EVENT_END_WITH_EXPLICIT_TS("foo", "bar", thread->thread_id(), 120);
>
> That would work, no?
>
> And, just because its only slow when its on is no excuse. You're causing a
> kernel context switch just to push performance data. That disrupts the
> performance measurement that we're trying to do. Thats not good.
If we only have the thread_id then the output in chrome://tracing just shows
1234: 3456 which isn't very useful. With the thread_name we can get 1234:
GL_ARB_timer_query which is a lot nicer to deal with.
base::Thread thread("gpu hardware") names the thread automatically. Look at
thread.cc --- thats supposed to somehow plumb to trace event.
On 2012/12/04 20:51:11, nduca wrote:
> base::Thread thread("gpu hardware") names the thread automatically. Look at
> thread.cc --- thats supposed to somehow plumb to trace event.
The trace_event code does a PlatformThread::GetName(); which gets the name from
the currently executing thread. The posix platform thread uses a thread local
variable to store the name.
I'm not sure how, given a thread_id to access the actual thread object to query
the name. In looking around I haven't seen any code that lets me do the Thread
lookup.
Then modify platform thread for get_name_for_tid maybe.
On 2012/12/04 21:33:56, nduca wrote: > Then modify platform thread for get_name_for_tid maybe. Since we'll end up making the same-ish modification, would it be worthwhile going back to look at the original API I used, instead of passing thread_id, pass the thread_name and let the tracing framework deal with making a fake thread_id. We'll have the same map structure, just hidden inside the tracing framework. I'm not sure how worthwhile the change to base would be to add the tid-> thread_name mapping as there doesn't seem to be a lot of demand for the structure in the code base.
Its the wrong architecture, thats all. You give threads names. trace_event picks those names up. The way it picks up the names has a bug. We fix the bug, not hack the architecture.
Drive by question (since I was reviewing an underlying CL) https://codereview.chromium.org/11366109/diff/30001/base/debug/trace_event_im... File base/debug/trace_event_impl.h (right): https://codereview.chromium.org/11366109/diff/30001/base/debug/trace_event_im... base/debug/trace_event_impl.h:288: unsigned long long id, nit: Why do we use long long rather than int64 (or in this case, uint64)?
https://codereview.chromium.org/11366109/diff/30001/base/debug/trace_event_im... File base/debug/trace_event_impl.h (right): https://codereview.chromium.org/11366109/diff/30001/base/debug/trace_event_im... base/debug/trace_event_impl.h:288: unsigned long long id, On 2012/12/06 17:28:59, jar wrote: > nit: Why do we use long long rather than int64 (or in this case, uint64)? I used it here to keep consistent with the existing AddTraceEvent method. I don't know why it was originally chosen, nduca@ maybe able to fill that in.
_WITH_EXPLICIT_xxx_and_YY --- explicit is too narrow. We're providing timestamp and tid explicitly right/
https://codereview.chromium.org/11366109/diff/30001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/30001/base/debug/trace_event.h#... base/debug/trace_event.h:642: #define TRACE_EVENT_API_ADD_TRACE_EVENT_EXPLICIT \ How about always providing the tid? And always providing the ts? It'll be more edits to the header but it keeps us to one entry point which was a design goal believe it or not.
I dropped the EXPLICIT as the name was getting pretty long and it seemed redundant. So, TRACE_EVENT_BEGIN_WITH_ID_TID_AND_TIMESTAMP0 is what it currently looks like. https://codereview.chromium.org/11366109/diff/30001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/30001/base/debug/trace_event.h#... base/debug/trace_event.h:642: #define TRACE_EVENT_API_ADD_TRACE_EVENT_EXPLICIT \ On 2012/12/11 19:40:58, nduca wrote: > How about always providing the tid? And always providing the ts? It'll be more > edits to the header but it keeps us to one entry point which was a design goal > believe it or not. Done.
Really close. https://codereview.chromium.org/11366109/diff/45001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/45001/base/debug/trace_event.h#... base/debug/trace_event.h:911: base::TimeTicks now = base::TimeTicks::NowFromSystemTraceTime() - Can we take off the TimeOffset inside the TraceLog implementation? I'd rather we didn't expose that as part of the public api.
https://codereview.chromium.org/11366109/diff/45001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/45001/base/debug/trace_event.h#... base/debug/trace_event.h:911: base::TimeTicks now = base::TimeTicks::NowFromSystemTraceTime() - On 2012/12/20 20:19:27, nduca wrote: > Can we take off the TimeOffset inside the TraceLog implementation? I'd rather we > didn't expose that as part of the public api. Done.
lgtm well done.
jar@, can you please take a look for base/ OWNERS? Thanks, dan
https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event.h#... base/debug/trace_event.h:40: // int64 timestamp, nit: This arg list as waaaay long, and it is saddening that there are a bunch of ints in a row (easy to mix them up, and hard to read). Have you considered using a base::Time or a base::TimeTicks here? Each would (under the covers) be an int64 in a class, and would avoid any chance of confusing the argements. https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:636: base::TimeTicks now = base::TimeTicks::NowFromSystemTraceTime(); nit: This surely shouts for passing in a TimeTicks instead of an int to the call on line 638. https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_im... File base/debug/trace_event_impl.h (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_im... base/debug/trace_event_impl.h:268: void AddTraceEvent(char phase, nit: style guide suggests not using function overloading, but instead giving different names to variants, such as was done on line 279. [Note: Overload on line 283 vs 279 is permitted, as the two take the same values, in just slightly different forms.] http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl... https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_in... File base/debug/trace_event_internal.h (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_in... base/debug/trace_event_internal.h:271: name, id, tid, ts) \ nit: please use larger names, like thread_id, and time_stamp. https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_un... File base/debug/trace_event_unittest.cc (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_un... base/debug/trace_event_unittest.cc:377: 1, 42, 12345); nit: please use named (constants?) rather than embedded magic numbers.
https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event.h#... base/debug/trace_event.h:40: // int64 timestamp, On 2013/02/14 02:17:54, jar wrote: > nit: This arg list as waaaay long, and it is saddening that there are a bunch of > ints in a row (easy to mix them up, and hard to read). Have you considered > using a base::Time or a base::TimeTicks here? Each would (under the covers) be > an int64 in a class, and would avoid any chance of confusing the argements. Done. https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_im... base/debug/trace_event_impl.cc:636: base::TimeTicks now = base::TimeTicks::NowFromSystemTraceTime(); On 2013/02/14 02:17:54, jar wrote: > nit: This surely shouts for passing in a TimeTicks instead of an int to the call > on line 638. Done. https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_im... File base/debug/trace_event_impl.h (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_im... base/debug/trace_event_impl.h:268: void AddTraceEvent(char phase, On 2013/02/14 02:17:54, jar wrote: > nit: style guide suggests not using function overloading, but instead giving > different names to variants, such as was done on line 279. > > [Note: Overload on line 283 vs 279 is permitted, as the two take the same > values, in just slightly different forms.] > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl... Done. https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_in... File base/debug/trace_event_internal.h (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_in... base/debug/trace_event_internal.h:271: name, id, tid, ts) \ On 2013/02/14 02:17:54, jar wrote: > nit: please use larger names, like thread_id, and time_stamp. Done. https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_un... File base/debug/trace_event_unittest.cc (right): https://codereview.chromium.org/11366109/diff/79001/base/debug/trace_event_un... base/debug/trace_event_unittest.cc:377: 1, 42, 12345); On 2013/02/14 02:17:54, jar wrote: > nit: please use named (constants?) rather than embedded magic numbers. Done.
LGTM after nits below. https://codereview.chromium.org/11366109/diff/80006/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/11366109/diff/80006/base/debug/trace_event_im... base/debug/trace_event_impl.cc:642: void TraceLog::AddTraceEventWithThreadIdAndTimestamp(char phase, style nit: in declaration and definitions, when you can't fit all args aligned with the opening paren, wrap them all (including this first one) so that the are all aligned at an indent of 4 (as you did below). https://codereview.chromium.org/11366109/diff/80006/base/debug/trace_event_im... File base/debug/trace_event_impl.h (right): https://codereview.chromium.org/11366109/diff/80006/base/debug/trace_event_im... base/debug/trace_event_impl.h:268: void AddTraceEventWithThreadIdAndTimestamp(char phase, nit: move "char phase," to next line, so that all parameters are aligned.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dsinclair@chromium.org/11366109/76006
https://codereview.chromium.org/11366109/diff/80006/base/debug/trace_event_im... File base/debug/trace_event_impl.cc (right): https://codereview.chromium.org/11366109/diff/80006/base/debug/trace_event_im... base/debug/trace_event_impl.cc:642: void TraceLog::AddTraceEventWithThreadIdAndTimestamp(char phase, On 2013/02/15 02:01:39, jar wrote: > style nit: in declaration and definitions, when you can't fit all args aligned > with the opening paren, wrap them all (including this first one) so that the are > all aligned at an indent of 4 (as you did below). Done. https://codereview.chromium.org/11366109/diff/80006/base/debug/trace_event_im... File base/debug/trace_event_impl.h (right): https://codereview.chromium.org/11366109/diff/80006/base/debug/trace_event_im... base/debug/trace_event_impl.h:268: void AddTraceEventWithThreadIdAndTimestamp(char phase, On 2013/02/15 02:01:39, jar wrote: > nit: move "char phase," to next line, so that all parameters are aligned. Done.
Message was sent while issue was closed.
Change committed as 182768
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11366109/diff/76006/base/debug/trace_e... File base/debug/trace_event.h (right): https://chromiumcodereview.appspot.com/11366109/diff/76006/base/debug/trace_e... base/debug/trace_event.h:6: // to remain platform and context neutral. To use tracing in another context, I would like to point out that this CL bakes in a dependence on 'base' in both the API and trace_event_internal.h, making this comment not very useful, and the division of trace_event and trace_event_internal pretty useless too. I've exposed this API to Pepper in a previous CL, which has no base/, and was in the process of implementing a version that would be self-contained in libppapi_cpp, when I noticed I could no longer include trace_event_internal.h hermetically: https://codereview.chromium.org/12212198/ I'm looking at what I can do to fix for my stuff as well as better future-proof this situation now.
Message was sent while issue was closed.
I'm pretty puzzled about the whole current stage of trace event right now. I didn't see the change go by earlier, and so I dont get what you're trying to do. The original intent was that people that dont use base, like webkit, would **copy** trace_event.h, then implement the TRACE_EVENT_API* calls their own way so that it routed into real tracing eventually. I dont understand why that wasn't done in the last place, and i'm pretty confused.
Message was sent while issue was closed.
On 2013/02/20 01:18:15, nduca wrote: > I'm pretty puzzled about the whole current stage of trace event right now. I > didn't see the change go by earlier, and so I dont get what you're trying to do. > The original intent was that people that dont use base, like webkit, would > **copy** trace_event.h, then implement the TRACE_EVENT_API* calls their own way > so that it routed into real tracing eventually. I dont understand why that > wasn't done in the last place, and i'm pretty confused. I see. Well, it looks like I did something not in keeping with tradition. I was trying to factor things out to make it easier for consumers of the trace event API to do their own implementation of the API functions while being able to use the stock trace_event_internal.h file. I guess that might not have been the right way to go about it. I still think the division is good, and it's being moved into its own trace_event_api_definitions.h file in a different CL by someone else. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
