Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
149563004:
initial import of Chrome's trace_event into skia framework (Closed)
Descriptioninitial import of Chrome's trace_event into skia framework
This patch includes a modified version of Chrome's trace_event.h, which provides
tracing macros that can easily integrate into the about://tracing framework.
Currently the macros link to a default implementation of the (narrow) tracing
class SkDefaultEventTracer which does nothing; next step will be to have Chrome
subclass the SkEventTracer with a shim that bolts Skia's trace events to its own,
allowing Skia's trace events to show up in about://tracing.
I've verified that this file builds properly, and when I added a simple scoped
TRACE_EVENT0 to SkCanvas::drawRect, along with some debug prints in the NOP
implementation of tracing, I saw what I expected printed to the screen.
BUG=skia:
Committed: http://code.google.com/p/skia/source/detail?r=13256
Patch Set 1 #Patch Set 2 : use SkOnce to sensibly initialize the singleton #
Total comments: 13
Patch Set 3 : Some cleanups from Mike #
Total comments: 35
Patch Set 4 : address comments from mike and mtklein #
Total comments: 6
Patch Set 5 : SkEventTraceHandle --> SkEventTracer::Handle #
Total comments: 1
Patch Set 6 : Attempt to fix GCC 4.7 build #Patch Set 7 : rebase #Patch Set 8 : remove duplicate test entries (error from rebase) #
Messages
Total messages: 29 (0 generated)
PTAL
Wow. SkTraceEvent.h is gigantic. Are we sure making a skia/generic version is worth it? https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... File include/utils/SkEventTracer.h (right): https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:19: * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT This copyright header seems a lot bigger than the one we've been using (now that we're in chrome). Look at include/gpu/GrTypes.h for a sample. https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:32: #define EventTracer_h skia's pattern would be to qualify this guard thusly -- #ifndef SkEventTracer_DEFINED https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:40: namespace skia { skia namespace not needed for types like SkEventTracer. https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:41: namespace tracing { why this other namespace? Is this explicitly to access types from Chrome? https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:48: class SK_API SkEventTracer { This name is very generic looking (and somewhat a near-collision with SkEvent and SkEventHandler. Can you document more specifically its scope and use? That may help us find the best name. https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h File src/core/SkTraceEvent.h (right): https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:190: #ifndef SkTRACEEVENT_DEFINED Skia pattern is to keep camel casing (until the suffix) SkTraceEvent_DEFINED https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:193: #include <string> Do we really need this? I don't see it in any of our other headers.
On 2014/01/29 18:38:40, reed1 wrote: > Wow. SkTraceEvent.h is gigantic. Are we sure making a skia/generic version is > worth it? > > https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... > File include/utils/SkEventTracer.h (right): > > https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:19: * "AS IS" AND ANY EXPRESS OR IMPLIED > WARRANTIES, INCLUDING, BUT NOT > This copyright header seems a lot bigger than the one we've been using (now that > we're in chrome). Look at include/gpu/GrTypes.h for a sample. > > https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:32: #define EventTracer_h > skia's pattern would be to qualify this guard thusly -- > > #ifndef SkEventTracer_DEFINED > > https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:40: namespace skia { > skia namespace not needed for types like SkEventTracer. > > https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:41: namespace tracing { > why this other namespace? Is this explicitly to access types from Chrome? > > https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:48: class SK_API SkEventTracer { > This name is very generic looking (and somewhat a near-collision with SkEvent > and SkEventHandler. Can you document more specifically its scope and use? That > may help us find the best name. > > https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h > File src/core/SkTraceEvent.h (right): > > https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h#... > src/core/SkTraceEvent.h:190: #ifndef SkTRACEEVENT_DEFINED > Skia pattern is to keep camel casing (until the suffix) > > SkTraceEvent_DEFINED > > https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h#... > src/core/SkTraceEvent.h:193: #include <string> > Do we really need this? I don't see it in any of our other headers.
https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... File include/utils/SkEventTracer.h (right): https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:19: * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT On 2014/01/29 18:38:40, reed1 wrote: > This copyright header seems a lot bigger than the one we've been using (now that > we're in chrome). Look at include/gpu/GrTypes.h for a sample. Whoops, you're right. Shortened. https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:32: #define EventTracer_h On 2014/01/29 18:38:40, reed1 wrote: > skia's pattern would be to qualify this guard thusly -- > > #ifndef SkEventTracer_DEFINED Yeah, this is another copy/paste bug; fixed. https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:41: namespace tracing { On 2014/01/29 18:38:40, reed1 wrote: > why this other namespace? Is this explicitly to access types from Chrome? It's probably unnecessary; it just mirrors the Chrome structure. I can make more of it top-level and make sure that the types (e.g., TraceEventHandle) are properly prefixed with Sk https://codereview.chromium.org/149563004/diff/40001/include/utils/SkEventTra... include/utils/SkEventTracer.h:48: class SK_API SkEventTracer { On 2014/01/29 18:38:40, reed1 wrote: > This name is very generic looking (and somewhat a near-collision with SkEvent > and SkEventHandler. Can you document more specifically its scope and use? That > may help us find the best name. Sure, I'm open to any name here. SkEventTracer implementations provide a narrow interface that the tracing macros bolt on to. There's a single instance of this class created; either the default one (which currently does nothing), or the one that Chrome will provide, which will hook the events to its own tracing mechanism. Maybe something like SkTraceEventProcessor or something? https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h File src/core/SkTraceEvent.h (right): https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:190: #ifndef SkTRACEEVENT_DEFINED On 2014/01/29 18:38:40, reed1 wrote: > Skia pattern is to keep camel casing (until the suffix) > > SkTraceEvent_DEFINED Done. https://codereview.chromium.org/149563004/diff/40001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:193: #include <string> On 2014/01/29 18:38:40, reed1 wrote: > Do we really need this? I don't see it in any of our other headers. Nope; removed.
Woah didn't mean to mega-reply. More concisely...
On 2014/01/29 18:45:22, nduca wrote:
> > Wow. SkTraceEvent.h is gigantic. Are we sure making a skia/generic version
is
> > worth it?
This header looks big and scary, but what it really is a bunch of macros
designed to sugar a ton of perf tracing use cases down to a set of 4 lowlevel
tracing APIs. The idea here is that of the tons of use cases that people end up
wanting to trace, the vast bulk of it is sugar: TRACE_EVENT1("skia.gpu",
"Flush", "reason", flush_reason) looks pretty straightforward, for instance.
But, what if flush_reason is a std::string? What if you dont want arguments?
What if you want to turn on "skia.gpu" traces but not "skia.image_cache"? Or
maybe you want to explicitly say when the event begins and ends. If you code
this up so every trace macro variant that you find useful has to has its own
callback from skia to blink, then you're in a world of pain: every valid perf
use case suddenly becomes a plumbing nightmare. To address this, we have a low
level tracing api consisting of the 4 lowlevel calls: once that is plumbed
across the fence, you can implement the tons of varied tracing calls as sugar on
top:
- categories to limit what you trace
- begin/end events, scoped and implicit.
- scoped trace events that get represented as one event in a log, rather than 2
[harder to implement efficiently than you'd think!]
- events with explicit threads and timestamps (good for storing gpu timestamps)
- asynchronous and flow events (for arrows and things like post tasks,
gpu-deferred work, etc)
- events whose names require copying [happens when the event being traced is
data, e.g from a sk comment]
- events with arguments, both ones that are by-value and require copying
- events with complex arguments where argument serializatoin is deferred to when
the recording is stopped
... probably more.
Now, I totally get that it looks intimidatingly large. The idea with this though
is that the header is copyable into the child repo and can be tweaked and
tinkered over time to match skias preferences with without having to really
change the embedding form. We do this in blink:
http://src.chromium.org/viewvc/blink/trunk/Source/platform/TraceEvent.h?revis...
One possible route here is to home-grow skia flavored tracing apis from scratch
or semi scratch. Android went this route building "atrace:" they started with a
subset, but haven't really added to it and one they locked down their embedding
api to be that subset, they had a hard time growing it out over time to support
all the use cases described above. Thus, cases come up all the time of "you
could do that in chrome, but can't do it in android apps because atrace isn't as
powerful." The result being people hack around it and move on with their day
jobs.
One other possibility is to trim the mega-macros down a bit. I believe greg has
actually already removed some stuff in there to keep things simpler. Maybe
there's more that could be deemed un-interesting through inspection though. I'd
leave that to you. The thing I would encourage is adopting the kernel notion of
tracing: that there's a very lowlevel embedding api that you can make the
embedder of skia understand, that lets you separate out the tracing "sugar" from
the plumbing parts.
Hope the context helps. :)
Must admit I skimmed the giant file. I am somewhat in favor of taking it all and figuring out what's useful to us as we go. Many of my comments were about style. It may be we want most of this to remain Chromium-style so we can pull it in verbatim (DEPS?), with another think Skia-style wrapper #including it with some examples in Skia language? https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... File include/utils/SkEventTracer.h (right): https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:8: #ifndef EventTracer_h SkEventTracer_DEFINED https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:12: I'm getting the feeling users aren't supposed to even look at this file. If that's so, can you add a big comment that points them over to the macros they want to be using? https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:15: #define TRACE_DISABLED_BY_DEFAULT(name) "disabled-by-default-" name Do we not double-define this? Also in SkTraceEvent. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:17: namespace skia { I don't mind namespace namespacing or Sk namespacing, but doing both seems redundant. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:23: SK_API extern long* traceSamplingState[3]; I don't see where we need this here? https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:30: static void SetInstance(SkEventTracer* tracer) { Can this be private? Who calls it? https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:35: virtual ~SkEventTracer() { SkDELETE(SkEventTracer::_instance); } Does this ever get called? Is it not an infinite deletion loop? You can pass another void(*)(void) to SkOnce as the fourth argument to register it to be called at process exit, if you want to clean up the global. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:41: enum CategoryGroupEnabledFlags { We'd normally call these kEnabledForRecording_CategoryGroupEnabledFlags, etc. Is there any particular reason to keep them LIKE_THIS? https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:50: virtual const unsigned char* GetCategoryGroupEnabled(const char* name) = 0; So there's one global SkEventTracer that's an interface to one of several concrete types? Can you make an empty protected constructor for SkEventTracer? https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:50: virtual const unsigned char* GetCategoryGroupEnabled(const char* name) = 0; These methods all want to be firstLetterLowercase. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:55: AddTraceEvent(char phase, I'm getting a little thrown by the types here some places we have const char*, some const unsigned char*, some long, some unsigned long long, etc. Where the type matters and it's not semantically an ASCII character or a string (char or const char*), can you use the types from stdint instead? (uint8_t, int64_t, uint64_t)? https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:70: static SkEventTracer *_instance; -> static SkEventTracer* fInstance; or static SkEventTracer* gInstance; ? https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h File src/core/SkTraceEvent.h (right): https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:116: // TRACE_EVENT_INSTANT0("SUBSYSTEM", str); // BAD! Just curious. Would it be incompatible with Chromium if we use the "" suffix trick to force compile-time constants? Is it common that you want to pass a non-static const char*? https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:145: // class MyData : public skia::tracing::ConvertableToTraceFormat { : SkNonCopyable, public skia::tracing::... https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:148: // virtual void AppendAsTraceFormat(std::string* out) const OVERRIDE { SK_OVERRIDE https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:157: // scoped_refptr<ConvertableToTraceFormat>(new MyData())); I think we're pretty unlikely to be using scoped_refptr. Can you translate to Skiaese? https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.cpp File src/utils/SkEventTracer.cpp (right): https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... src/utils/SkEventTracer.cpp:2: * Copyright 2013 Google Inc. I bet you can bump this to 2014 now. https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... src/utils/SkEventTracer.cpp:15: virtual TraceEventHandle Doesn't hurt to tack on SK_OVERRIDE to these. You don't really need virtual unless you're planning on subclassing SkDefaultEventTracer, but I guess they don't hurt. https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... src/utils/SkEventTracer.cpp:44: static void intiailize_default_tracer(void**) { If you're not going to use the argument, we usually make it int and pass 0. I guess you can call it void****** if you like. https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... src/utils/SkEventTracer.cpp:48: SkEventTracer *SkEventTracer::GetInstance() { We almost always put the * on the left. https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... src/utils/SkEventTracer.cpp:55: } If you keep the namespaces, can you add } // namespace tracing } // namespace skia ?
On 2014/01/29 20:57:54, mtklein wrote: > Must admit I skimmed the giant file. I am somewhat in favor of taking it all > and figuring out what's useful to us as we go. > > Many of my comments were about style. It may be we want most of this to remain > Chromium-style so we can pull it in verbatim (DEPS?), with another think > Skia-style wrapper #including it with some examples in Skia language? > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > File include/utils/SkEventTracer.h (right): > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:8: #ifndef EventTracer_h > SkEventTracer_DEFINED > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:12: > I'm getting the feeling users aren't supposed to even look at this file. If > that's so, can you add a big comment that points them over to the macros they > want to be using? > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:15: #define TRACE_DISABLED_BY_DEFAULT(name) > "disabled-by-default-" name > Do we not double-define this? Also in SkTraceEvent. > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:17: namespace skia { > I don't mind namespace namespacing or Sk namespacing, but doing both seems > redundant. > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:23: SK_API extern long* traceSamplingState[3]; > I don't see where we need this here? > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:30: static void SetInstance(SkEventTracer* tracer) > { > Can this be private? Who calls it? > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:35: virtual ~SkEventTracer() { > SkDELETE(SkEventTracer::_instance); } > Does this ever get called? Is it not an infinite deletion loop? > > You can pass another void(*)(void) to SkOnce as the fourth argument to register > it to be called at process exit, if you want to clean up the global. > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:41: enum CategoryGroupEnabledFlags { > We'd normally call these kEnabledForRecording_CategoryGroupEnabledFlags, etc. > > Is there any particular reason to keep them LIKE_THIS? > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:50: virtual const unsigned char* > GetCategoryGroupEnabled(const char* name) = 0; > So there's one global SkEventTracer that's an interface to one of several > concrete types? > > Can you make an empty protected constructor for SkEventTracer? > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:50: virtual const unsigned char* > GetCategoryGroupEnabled(const char* name) = 0; > These methods all want to be firstLetterLowercase. > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:55: AddTraceEvent(char phase, > I'm getting a little thrown by the types here > > some places we have const char*, some const unsigned char*, some long, some > unsigned long long, etc. > > Where the type matters and it's not semantically an ASCII character or a string > (char or const char*), can you use the types from stdint instead? (uint8_t, > int64_t, uint64_t)? > > https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... > include/utils/SkEventTracer.h:70: static SkEventTracer *_instance; > -> > > static SkEventTracer* fInstance; > > or > > static SkEventTracer* gInstance; ? > > https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h > File src/core/SkTraceEvent.h (right): > > https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... > src/core/SkTraceEvent.h:116: // TRACE_EVENT_INSTANT0("SUBSYSTEM", str); // > BAD! > Just curious. Would it be incompatible with Chromium if we use the "" suffix > trick to force compile-time constants? Is it common that you want to pass a > non-static const char*? > > https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... > src/core/SkTraceEvent.h:145: // class MyData : public > skia::tracing::ConvertableToTraceFormat { > : SkNonCopyable, public skia::tracing::... > > https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... > src/core/SkTraceEvent.h:148: // virtual void > AppendAsTraceFormat(std::string* out) const OVERRIDE { > SK_OVERRIDE > > https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... > src/core/SkTraceEvent.h:157: // > scoped_refptr<ConvertableToTraceFormat>(new MyData())); > I think we're pretty unlikely to be using scoped_refptr. Can you translate to > Skiaese? > > https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.cpp > File src/utils/SkEventTracer.cpp (right): > > https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... > src/utils/SkEventTracer.cpp:2: * Copyright 2013 Google Inc. > I bet you can bump this to 2014 now. > > https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... > src/utils/SkEventTracer.cpp:15: virtual TraceEventHandle > Doesn't hurt to tack on SK_OVERRIDE to these. > > You don't really need virtual unless you're planning on subclassing > SkDefaultEventTracer, but I guess they don't hurt. > > https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... > src/utils/SkEventTracer.cpp:44: static void intiailize_default_tracer(void**) { > If you're not going to use the argument, we usually make it int and pass 0. I > guess you can call it void****** if you like. > > https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... > src/utils/SkEventTracer.cpp:48: SkEventTracer *SkEventTracer::GetInstance() { > We almost always put the * on the left. > > https://codereview.chromium.org/149563004/diff/60001/src/utils/SkEventTracer.... > src/utils/SkEventTracer.cpp:55: } > If you keep the namespaces, can you add > > } // namespace tracing > } // namespace skia > > ? *thin
I think its safe to customize the header to to skia style, either as part of the initial or eventually. We dont change these apis that much and I think there're already enough subtle tweaks from chrome to here that it'd never be a straight copy anyway.
Also I added a test, which is trivial right now but it actually includes and calls one of the macros; a number of compile-time errors are only caught this way (for now, until we start using this). https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... File include/utils/SkEventTracer.h (right): https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:8: #ifndef EventTracer_h On 2014/01/29 20:57:55, mtklein wrote: > SkEventTracer_DEFINED Done. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:12: On 2014/01/29 20:57:55, mtklein wrote: > I'm getting the feeling users aren't supposed to even look at this file. If > that's so, can you add a big comment that points them over to the macros they > want to be using? Sure -- this isn't a standard skia user-facing API, but Chrome will call it. I can add a comment to the effect that this is likely not the place that people want to be looking. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:15: #define TRACE_DISABLED_BY_DEFAULT(name) "disabled-by-default-" name On 2014/01/29 20:57:55, mtklein wrote: > Do we not double-define this? Also in SkTraceEvent. Done. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:17: namespace skia { On 2014/01/29 20:57:55, mtklein wrote: > I don't mind namespace namespacing or Sk namespacing, but doing both seems > redundant. I'll remove the namespaces. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:23: SK_API extern long* traceSamplingState[3]; On 2014/01/29 20:57:55, mtklein wrote: > I don't see where we need this here? Deleted. I'll reintroduce it when I tackle the part of the tracing system that uses it, in a later phase of this integration. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:30: static void SetInstance(SkEventTracer* tracer) { On 2014/01/29 20:57:55, mtklein wrote: > Can this be private? Who calls it? Chrome will call it. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:35: virtual ~SkEventTracer() { SkDELETE(SkEventTracer::_instance); } On 2014/01/29 20:57:55, mtklein wrote: > Does this ever get called? Is it not an infinite deletion loop? > > You can pass another void(*)(void) to SkOnce as the fourth argument to register > it to be called at process exit, if you want to clean up the global. Good catch. Will move to SkOnce. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:41: enum CategoryGroupEnabledFlags { On 2014/01/29 20:57:55, mtklein wrote: > We'd normally call these kEnabledForRecording_CategoryGroupEnabledFlags, etc. > > Is there any particular reason to keep them LIKE_THIS? As long as the numbers match Chromium, we can call them whatever we want. I'll clean them up. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:50: virtual const unsigned char* GetCategoryGroupEnabled(const char* name) = 0; On 2014/01/29 20:57:55, mtklein wrote: > So there's one global SkEventTracer that's an interface to one of several > concrete types? > > Can you make an empty protected constructor for SkEventTracer? Sure. is that necessary since SkEventTracer is pure virtual? https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:50: virtual const unsigned char* GetCategoryGroupEnabled(const char* name) = 0; On 2014/01/29 20:57:55, mtklein wrote: > These methods all want to be firstLetterLowercase. Done. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:55: AddTraceEvent(char phase, On 2014/01/29 20:57:55, mtklein wrote: > I'm getting a little thrown by the types here > > some places we have const char*, some const unsigned char*, some long, some > unsigned long long, etc. > > Where the type matters and it's not semantically an ASCII character or a string > (char or const char*), can you use the types from stdint instead? (uint8_t, > int64_t, uint64_t)? Done. https://codereview.chromium.org/149563004/diff/60001/include/utils/SkEventTra... include/utils/SkEventTracer.h:70: static SkEventTracer *_instance; On 2014/01/29 20:57:55, mtklein wrote: > -> > > static SkEventTracer* fInstance; > > or > > static SkEventTracer* gInstance; ? Done. https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h File src/core/SkTraceEvent.h (right): https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:116: // TRACE_EVENT_INSTANT0("SUBSYSTEM", str); // BAD! On 2014/01/29 20:57:55, mtklein wrote: > Just curious. Would it be incompatible with Chromium if we use the "" suffix > trick to force compile-time constants? Is it common that you want to pass a > non-static const char*? I don't know the answer to this -- shoudl probably ask Nat. https://codereview.chromium.org/149563004/diff/60001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:145: // class MyData : public skia::tracing::ConvertableToTraceFormat { On 2014/01/29 20:57:55, mtklein wrote: > : SkNonCopyable, public skia::tracing::... Yeha, this convertable stuff was stripped pending a future phase of this integration, but I missed the comment related to it. I'll strip this too for now.
lgtm, with the caveat that my opinion of things set down here may change as tracing sees more use in Skia. https://codereview.chromium.org/149563004/diff/80001/include/utils/SkEventTra... File include/utils/SkEventTracer.h (right): https://codereview.chromium.org/149563004/diff/80001/include/utils/SkEventTra... include/utils/SkEventTracer.h:25: typedef uint32_t SkTraceEventHandle; Suggestion: make this SkEventTracer::Handle ? https://codereview.chromium.org/149563004/diff/80001/include/utils/SkEventTra... include/utils/SkEventTracer.h:37: virtual ~SkEventTracer() { } Sorry, somehow I'd brainfarted and not realized it was an abstract class. Keeping the destructor here is not currently necessary but good practice. https://codereview.chromium.org/149563004/diff/80001/include/utils/SkEventTra... include/utils/SkEventTracer.h:42: // These values must be in sync with macro values in trace_event.h in chromium. Sorry, I hadn't realized these have to stay in sync with something else. Is there a way to eliminate that with a direct dependency? Can we have this file #include SkTraceEvent.h? Or have them canonically defined here instead of in SkTraceEvent.h? If not, let's make an exception and go back to having the enum value names match the #defines exactly. https://codereview.chromium.org/149563004/diff/80001/src/core/SkTraceEvent.h File src/core/SkTraceEvent.h (right): https://codereview.chromium.org/149563004/diff/80001/src/core/SkTraceEvent.h#... src/core/SkTraceEvent.h:285: TraceEventSamplingStateScope<bucket_number>::Set(category "\0" name) This is a super cute way to require category and name to both be compile time strings and pass them as one pointer.
https://codereview.chromium.org/149563004/diff/80001/include/utils/SkEventTra... File include/utils/SkEventTracer.h (right): https://codereview.chromium.org/149563004/diff/80001/include/utils/SkEventTra... include/utils/SkEventTracer.h:25: typedef uint32_t SkTraceEventHandle; On 2014/01/30 14:18:26, mtklein wrote: > Suggestion: make this SkEventTracer::Handle ? I like that a lot better; done. https://codereview.chromium.org/149563004/diff/80001/include/utils/SkEventTra... include/utils/SkEventTracer.h:42: // These values must be in sync with macro values in trace_event.h in chromium. On 2014/01/30 14:18:26, mtklein wrote: > Sorry, I hadn't realized these have to stay in sync with something else. Is > there a way to eliminate that with a direct dependency? Can we have this file > #include SkTraceEvent.h? Or have them canonically defined here instead of in > SkTraceEvent.h? > > If not, let's make an exception and go back to having the enum value names match > the #defines exactly. I believe the design of the Chrome tracing system is such that these values are locked down; changing them would break tracing integration with Blink and V8 also so I think the chances of that are extremely low.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/humper@google.com/149563004/90001
Presubmit check for 149563004-90001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com', 'rmistry@google.com') Was the presubmit check useful? If not, run "git cl presubmit -v" to figure out which PRESUBMIT.py was run, then run git blame on the file to figure out who to ask for help.
api lgtm https://codereview.chromium.org/149563004/diff/90001/include/utils/SkEventTra... File include/utils/SkEventTracer.h (right): https://codereview.chromium.org/149563004/diff/90001/include/utils/SkEventTra... include/utils/SkEventTracer.h:56: virtual SkEventTracer::Handle I doubt any compiler will complain but SkEventTracer:: isn't necessary here.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/humper@google.com/149563004/90001
Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for step(s) BuildEverything http://108.170.219.164:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/humper@google.com/149563004/90001
Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for step(s) BuildTests http://108.170.219.164:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/humper@google.com/149563004/110001
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/humper@google.com/149563004/130001
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/humper@google.com/149563004/150001
Message was sent while issue was closed.
Change committed as 13256
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
