Add Gpu Tracing to Ganesh

src/gpu/GrDrawTarget.h

Index: src/gpu/GrDrawTarget.h
diff --git a/src/gpu/GrDrawTarget.h b/src/gpu/GrDrawTarget.h
index 19ce16f64a68818512f0d32b7192c90a06dd6b4e..143562f63da89531b49f2379fe00c1da651815a6 100644
--- a/src/gpu/GrDrawTarget.h
+++ b/src/gpu/GrDrawTarget.h
@@ -22,10 +22,27 @@
 
 class GrClipData;
 class GrDrawTargetCaps;
+// class GrDrawTarget;

[bsalomon, 2014/03/17 17:50:38]

?

[egdaniel, 2014/03/17 19:49:12]

Woops. Left over from some old test iteration. Removed.
On 2014/03/17 17:50:38, bsalomon wrote:

 class GrPath;
 class GrVertexBuffer;
 class SkStrokeRec;
 
+class GpuTraceMarker {
+public:
+    GpuTraceMarker(const char* marker, int idCounter) : fMarker(marker), fID(idCounter) {}
+
+    bool operator<(const GpuTraceMarker& rhs) const {
+        return this->fMarker < rhs.fMarker || (this->fMarker == rhs.fMarker && this->fID < rhs.fID);
+    }
+
+    bool operator==(const GpuTraceMarker& rhs) const {
+        return (this->fID == rhs.fID && this->fMarker == rhs.fMarker);
+    }
+
+    const char* fMarker;
+    int fID;
+};
+
 class GrDrawTarget : public SkRefCnt {
 protected:
     class DrawInfo;
@@ -423,18 +440,19 @@ public:
                        GrRenderTarget* renderTarget = NULL) = 0;
 
     /**
-     * instantGpuTraceEvent places a single "sign post" type marker into command stream. The
-     * argument marker will be the name of the annotation that is added.
-     */
-    void instantGpuTraceEvent(const char* marker);
-    /**
-     * The following two functions are used for marking groups of commands. Use pushGpuTraceEvent
-     * to set the beginning of a command set, and popGpuTraceEvent is be called at end of the
-     * command set. The argument marker is the name for the annotation that is added. The push and
-     * pops can be used hierarchically, but every push must have a match pop.
+     * Called at start and end of gpu trace marking
+     * GR_CREATE_GPU_TRACE_MARKER(marker_str, target) will automatically call these at the start
+     * and end of a code block respectively
      */
-    void pushGpuTraceEvent(const char* marker);
-    void popGpuTraceEvent();
+    void addGpuTraceMarker(GpuTraceMarker* marker);
+    void removeGpuTraceMarker(GpuTraceMarker* marker);
+
+    bool gpuTracingEnabled() const;

[bsalomon, 2014/03/17 17:50:38]

I wonder if we really need enable/disable control below GrContext level. To have
it on GrDT might make for weird combinations (e.g. enabled on GrIODB but not the
GrGpu object).

[egdaniel, 2014/03/17 19:49:12]

This function is for controlling whether or not it is enabled, but just to check
if it is. Regardless, this is only used in the macro to make a trace event, so
that will get bubbled up to GrContext in the next iteration anyways.

On 2014/03/17 17:50:38, bsalomon wrote:

[bsalomon, 2014/03/19 13:28:44]

sgtm

+
+    SkTDArray<GpuTraceMarker>* getActiveTraceMarkers() { return &fActiveTraceMarkers; }

[bsalomon, 2014/03/17 17:50:38]

const &?

[egdaniel, 2014/03/17 19:49:12]

in gl/GrGpuGl.cpp I call getActiveTraceMarkers(), and then send them to
getTraceString which is not const (see comment below).

On 2014/03/17 17:50:38, bsalomon wrote:

+
+    static SkString getTraceString(SkTDArray<GpuTraceMarker>* markerArray,

[bsalomon, 2014/03/17 17:50:38]

Def. const & for markerArray. Also static functions begin with a capital letter.

Can this be protected?

[egdaniel, 2014/03/17 19:49:12]

markerArray ends up getting sorted in the function so it won't be const. Maybe
function could be changed so that it assumes the markers are sorted and thus
just extracts a string. Sorting would take place in some other call.

On 2014/03/17 17:50:38, bsalomon wrote:

[bsalomon, 2014/03/19 13:28:44]

Ok that makes sense. It'd be nice to hide this somehow, though, so that we don't
have a public function on this class that returns a non-const ptr to the array.
Maybe getTraceString should be a non-static member that sorts the array and
converts it into a string?

[egdaniel, 2014/03/19 14:27:03]

So the function has already been moved to protected to hide it a little. But at
this point what does changing it from static to non-static buy us? It doesn't
need access to any specific data member, and it already "sorts the array and
converts it into a string".

So another option is to just not have it do any sorting and just do string
conversion. In this case it will only merge strings/ID's together if they are
contiguous in the array itself. Thus it is on the caller to sort the array
themselves. We can also just claim the function will run but with undefined
behavior if array is not sorted.

On 2014/03/19 13:28:44, bsalomon wrote:

[bsalomon, 2014/03/19 14:43:43]

It just seems to me like sorting-and-stringifying go together logically and by
separating them we have to have this weird accessor that allows a caller to do
arbitrary manipulation to the array. Really the only manipulation a caller wants
to do is sort in order to call  getTraceString(). Sorting is OK since the
markers are effectively a set and unordered. The fact that the string produced
by getTraceString() depends on the order of the array feels like implementation
details bleeding out into the interface. That it is an array, that it is ok to
reorder the array, and that getting the desired string requires sorting the
array all seem like things that should be hidden in as few related functions as
possible.

I actually kind of wonder if exposing the trace markers as an array rather than
as an iterator is a mistake... if we ever decide to store them differently then
all callers that walk the array have to be changed. I guess if
getActiveTraceMarkers() is protected then all the callers are in the GrDT family
and that's not so bad.

[egdaniel, 2014/03/19 15:16:30]

Okay so here is my proposal (GpuTM is our trace marker class):

1) const GpuTM* activeTraceMarkersBegin()
   const GpuTM* activeTraceMarkersEnd()
   // thus iterating will be done by just incrementing the pointer

2) SkString getTraceStringFromActiveMarkers() // would do sorting internally

3) SkString getTraceString(GpuTM* start, GpuTM* end)
   // will sort between start and end, and then return trace string

On 2014/03/19 14:43:43, bsalomon wrote:

[bsalomon, 2014/03/19 15:27:01]

How about getActiveMarkersTraceString()? 
Oh, I didn't realize you needed this. Why do you need a string of a subset?

[egdaniel, 2014/03/19 15:33:53]

So internally I have a single array that stores the markers for each command
(different from the active set). A second array stores the marker count for each
command. So for a given command I am only interested in a subset of the entire
array. Checkout flush() in GrDrawTarget.h for a better understanding of how I
use it.

On 2014/03/19 15:27:01, bsalomon wrote:

[bsalomon, 2014/03/19 15:50:39]

Got it. Do you think you get a lot of value out of a combined array? I'm
wondering if what really this is asking for is class GrTraceMarkerSet that has
add, remove, toString, and an iterator.

+                                   int startIdx, int numMarkers);
 
     /**
      * Copies a pixel rectangle from one surface to another. This call may finalize
@@ -868,9 +886,8 @@ private:
     virtual void onDrawPath(const GrPath*, SkPath::FillType,
                             const GrDeviceCoordTexture* dstCopy) = 0;
 
-    virtual void onInstantGpuTraceEvent(const char* marker) = 0;
-    virtual void onPushGpuTraceEvent(const char* marker) = 0;
-    virtual void onPopGpuTraceEvent() = 0;
+    virtual void onAddGpuTraceMarker() = 0;

[bsalomon, 2014/03/17 17:50:38]

It's unclear from the header why we have all three of

addTraceMarkerActiveSet()
onAddGpuTraceMarker()
addGpuTraceMarker(GpuTraceMarker* marker)

(I haven't read the cpp yet but it should make sense from the .h)

If on[Add|Remove]GpuTraceMarker() are notifiers that are called before or after
some action in [add|remove]GpuTraceMarker we name them something like
willAddGpuTraceMarker (if it is call before) or didAddGpuTraceMarker (if it is
called after).

[egdaniel, 2014/03/17 19:49:12]

So the basic structure here is: addGpuTraceMarker is the public interface to
create one. Then the add/removeTraceMarkerActiveSet one performed all actions
universal to all all DrawTargets. At one point these had a few commands for
add/remove but at this point there is only a push() in addTMActiveSet() and a
find() & remove(index) in removeTMActiveSet(), so we could just put these
directly into add/removeGpuTraceMarker().

the onAddGpuTraceMarkers() call is used to perform code related to the specific
DrawTarget. For example, GrIODB does nothing here, but GrGpuGL does have
specilized code (like making the gl calls).

Basically the code looks like:
addGpuTraceMarker(marker) {
    addTraceMarkerActiveSet(marker); //done for all targets
    onAddGpuTracemarker(); // calls target specific code    
}

Given this is there a standard way to name these things?
On 2014/03/17 17:50:38, bsalomon wrote:

[bsalomon, 2014/03/19 13:28:44]

Ok so addTraceMarkerActiveSet is just a helper/implementation function for
addTraceMarker().  That seems fine. I'd name the virtual didAddGpuTraceMarker()
since it is a notification to the subclass that the action was already
performed.

+    virtual void onRemoveGpuTraceMarker() = 0;
 
     // helpers for reserving vertex and index space.
     bool reserveVertexSpace(size_t vertexSize,
@@ -897,6 +914,12 @@ private:
     // Check to see if this set of draw commands has been sent out
     virtual bool       isIssued(uint32_t drawID) { return true; }
 
+    // Add new marker to our current active set
+    void addTraceMarkerActiveSet(GpuTraceMarker* marker);
+
+    // Remove marker from our current active set
+    void removeTraceMarkerActiveSet(GpuTraceMarker* marker);
+
     enum {
         kPreallocGeoSrcStateStackCnt = 4,
     };
@@ -906,10 +929,46 @@ private:
     GrDrawState                                                     fDefaultDrawState;
     // The context owns us, not vice-versa, so this ptr is not ref'ed by DrawTarget.
     GrContext*                                                      fContext;
-    // To keep track that we always have at least as many debug marker pushes as pops
-    int                                                             fPushGpuTraceCount;
+    // To keep track that we always have at least as many debug marker adds as removes
+    int                                                             fGpuTraceMarkerCount;
+    SkTDArray<GpuTraceMarker>            fActiveTraceMarkers;
 
     typedef SkRefCnt INHERITED;
 };
 
+/**
+ * Marker generation class used for adding and removing markers around code blocks
+ */
+class TraceMarkerGenerator : public ::SkNoncopyable {
+public:
+    TraceMarkerGenerator(const char* marker_str, int* marker_counter, GrDrawTarget* target)
+        : fTarget(target), fTraceMarker(marker_str, *marker_counter) {
+        if (fTarget->gpuTracingEnabled()) {
+            sk_atomic_inc(marker_counter);
+            fTarget->addGpuTraceMarker(&fTraceMarker);
+        }
+    }
+    ~TraceMarkerGenerator() {
+        if (fTarget->gpuTracingEnabled()) {
+            fTarget->removeGpuTraceMarker(&fTraceMarker);
+        }
+    }
+private:
+    GrDrawTarget* fTarget;
+    GpuTraceMarker fTraceMarker;
+};
+
+////////// macros to place around the internal draw calls //////////////////
+// marker is of type const char* and target is of type GrDrawTarget*
+
+#define GR_CREATE_GPU_TRACE_MARKER(marker_str, target)                        \
+        static const char* static_str = marker_str;                           \
+        static int marker_counter = 0;                                       \
+        TraceMarkerGenerator SK_MACRO_APPEND_LINE(TMG)(static_str,            \
+                                                       &marker_counter,       \
+                                                       target);               \
+
+////////////////////////////////////////////////////////////////////////////
+
+
 #endif