Create a single command buffer for GrInOrderDrawBuffer

src/gpu/GrCmdBuffer.h

+/*
+ * Copyright 2014 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef GrCmdBuffer_DEFINED
+#define GrCmdBuffer_DEFINED
+
+#include "SkTemplates.h"
+#include "SkTypes.h"
+
+class GrDrawTarget;
+
+/**
+ * Ordered list of commands for a GrDrawTarget.
+ *
+ * The commands are stored contiguously within large blocks of memory, in order avoid
+ * excessive calls to malloc().
+ *
+ * @param TAlign  A type whose size is the desired alignment for command allocations.
+ *                Using long double will always meet alignment requirements, but this may
+ *                be smaller if the largest alignment requirement is know ahead of time.
+ */
+template<typename TAlign = long double> class GrCmdBuffer : SkNoncopyable {

[bsalomon, 2014/10/03 15:04:45]

Do long doubles require alignment? Wondering if void* is always sufficient.

[Chris Dalton, 2014/10/03 16:16:24]

According to this, long double gets aligned at 16 bytes:

http://www.codesynthesis.com/~boris/blog/2009/04/06/cxx-data-alignment-portab...

Whether that is necessary is questionable, especially since it just boils down
to performance and the likelihood of ever storing a long double in here is
basically nil. I think I'd be happy to remove that template parameter and align
everything at void* if that's the route we decide to take.

[bsalomon, 2014/10/03 18:43:03]

Interesting. Maybe just have no default?

[Chris Dalton, 2014/10/03 19:38:40]

Sure

[Chris Dalton, 2014/10/08 19:29:02]

Done.

+public:
+    class Cmd;
+    class Iter;
+
+    /**
+     * Create a command buffer
+     *
+     * @param initialSizeInBytes  The amount of memory reserved by the command buffer
+     *                            initially, and after calls to reset().
+     */
+    GrCmdBuffer(int initialSizeInBytes)
+        : fHeadBlock(bytes_to_length(initialSizeInBytes)),
+          fTailBlock(&fHeadBlock),
+          fBack(0),
+          fFirstCmd(NULL),
+          fLastCmd(NULL) {}
+
+    ~GrCmdBuffer() { this->reset(); }
+
+    bool empty() {
+        SkASSERT((NULL == fFirstCmd) == (NULL == fLastCmd));
+        return NULL == fFirstCmd;
+    }
+
+    Cmd& front() {
+        SkASSERT(!this->empty());
+        return *fFirstCmd;
+    }
+
+    Cmd& back() {
+        SkASSERT(!this->empty());
+        return *fLastCmd;
+    }
+
+    /**
+     * Destruct all commands in the command buffer and reset to empty.
+     */
+    void reset();
+
+    /**
+     * Push a command to the back of the buffer, with optional arguments to for its
+     * constructor. TCmd must be a subclass of GrCmdBuffer::Cmd.
+     */
+    template<typename TCmd>
+    TCmd& push_back() { return this->push_back_size<TCmd>(sizeof(TCmd)); }

[bsalomon, 2014/10/03 15:04:45]

Instead of all these could we use something like a new override and a macro that
does in-place new?

(e.g. SkNEW_APPEND_TO_TARRAY, SkNEW_INSERT_AT_LLIST_HEAD, ...)

[Chris Dalton, 2014/10/03 16:16:24]

I did consider that approach. The only problem is that (without getting really
creative) push_back_raw() and init_back() then need to become public, and they
can mess things up if used wrong. They're meant to only be used in a very
specific order: reserve data, in-place new, initialize Cmd base class. So for
that reason I opted to do it like SkSmallAllocator.

Has Skia adopted C++11? If so we could use variadic temples...

[bsalomon, 2014/10/03 18:43:03]

In the other instances where we've done this we just make the op new overload be
friends with the container class to access a private raw push.
Not yet, hopefully soon.

[Chris Dalton, 2014/10/03 19:38:40]

Ok, this will work now. (The issue used to be that there was data to initialize
in the Cmd class after the constructor (and therefore after operator new
returned), but now that we're taking out fType, this isn't a problem anymore.)

[Chris Dalton, 2014/10/08 19:29:02]

Done.

+    template<typename TCmd, typename TArg1>
+    TCmd& push_back(const TArg1& a) { return this->push_back_size<TCmd>(sizeof(TCmd), a); }
+    template<typename TCmd, typename TArg1, typename TArg2>
+    TCmd& push_back(const TArg1& a, const TArg2& b) { return this->push_back_size<TCmd>(sizeof(TCmd), a, b); }
+    template<typename TCmd, typename TArg1, typename TArg2, typename TArg3>
+    TCmd& push_back(const TArg1& a, const TArg2& b, const TArg3& c) { return this->push_back_size<TCmd>(sizeof(TCmd), a, b, c); }
+
+    /**
+     * Push a variable-size command to the back of the buffer, with optional arguments for
+     * its constructor. TCmd must be a subclass of GrCmdBuffer::Cmd.
+     *
+     * @param sizeInBytes  The amount of memory to allocate for the command. This may be

[bsalomon, 2014/10/03 15:04:45]

Wonder whether we should only take the extra payload size and not the total
size.

The fData[0] thing feels overly tricky to me.  What about 

virtual void execute(GrDrawTarget*, void* payload) ?

[Chris Dalton, 2014/10/03 16:16:24]

The Cmd base class could also maybe know where to find its payload.

[bsalomon, 2014/10/03 18:43:03]

Sure but then we need to store ptr. If it's a param then it just needs to exist
at execute time.

[Chris Dalton, 2014/10/03 19:38:40]

So at execute time, the client code doesn't know where the payload is stored,
and neither does the command buffer. (It would be right after the end of the Cmd
subclass, but the full size of that subclass is unknown, and the Cmd* address
might not be the same as its subclass's address.)

It was my understanding that a zero-length array is standard for a struct that
is the header of a variable-length object, but if we don't go that route, it
seems we would either need to store a ptr/offset or else the subclass would have
to find it with pointer arithmetic during execute().

[Chris Dalton, 2014/10/08 19:29:02]

Left as-is for now, to resemble the way this would be handled with malloc:

  malloc(sizeof(T) + payload);

+     *                     larger than sizeof(TCmd) to accomodate variable-length arrays.
+     */
+    template<typename TCmd>
+    TCmd& push_back_size(int sizeInBytes);
+    template<typename TCmd, typename TArg1>
+    TCmd& push_back_size(int sizeInBytes, const TArg1&);
+    template<typename TCmd, typename TArg1, typename TArg2>
+    TCmd& push_back_size(int sizeInBytes, const TArg1&, const TArg2&);
+    template<typename TCmd, typename TArg1, typename TArg2, typename TArg3>
+    TCmd& push_back_size(int sizeInBytes, const TArg1&, const TArg2&, const TArg3&);
+
+private:
+    static int bytes_to_length(int bytes) { return (bytes + sizeof(TAlign) - 1) / sizeof(TAlign); }
+
+    void* push_back_raw(int sizeInBytes);
+    template<typename T> T& init_back(T* cmd);
+
+    struct Block {
+        Block(int length) : fLength(length), fBuffer(fLength) {}
+        const int             fLength;
+        SkAutoTMalloc<TAlign> fBuffer;
+        SkAutoTDelete<Block>  fNext;
+    };
+    Block  fHeadBlock;
+    Block* fTailBlock;
+    int    fBack;
+
+    Cmd*   fFirstCmd;
+    Cmd*   fLastCmd;
+
+    friend class Iter;
+};
+
+////////////////////////////////////////////////////////////////////////////////
+
+template<typename TAlign>
+class GrCmdBuffer<TAlign>::Cmd : SkNoncopyable {
+public:
+    Cmd() : fNext(NULL) {}
+    virtual ~Cmd() {}
+    virtual void execute(GrDrawTarget*) = 0;
+
+    uint8_t type() const { return fType; }
+    void resetType(uint8_t type) { fType = type; }
+
+private:
+    Cmd*    fNext;
+    uint8_t fType;

[bsalomon, 2014/10/03 15:04:45]

Is the type really just just used to hang the debug marker bit?  If so could we
do something like have the cmds in GrIODB that may need to have a dbg marker
inherit from a base cmd type that has a bool?

I ask because it feels like if we're paying for virtuality then it would be
nicer to go all the way and not have to know the type being stored.

[Chris Dalton, 2014/10/03 16:16:24]

At least for now, concatInstancedDraw() also checks the type:

    if (kDraw_Cmd != strip_trace_bit(fCmdBuffer.back().type())) {
        return 0;
    }

[bsalomon, 2014/10/03 18:43:03]

The GrIODB could just have a bool, fLastCmdWasDraw. Or the GrIODB's cmd structs
could share a base class and that could have fType. It seems like having fType
here is baking GrIODB's particulars into a reusable component.

[Chris Dalton, 2014/10/03 19:38:40]

Oh, fLastCmdWasDraw, I like that

[Chris Dalton, 2014/10/08 19:29:02]

I ended up leaving fType in the base Cmd class. It's used to track trace markers
as well as make asserts during flush. I'm still open to removing it.

+
+    friend class GrCmdBuffer;
+};
+
+template<typename TAlign>
+class GrCmdBuffer<TAlign>::Iter {
+public:
+    Iter(GrCmdBuffer& cmdBuffer) : fCmd(NULL), fNext(cmdBuffer.fFirstCmd) {}
+
+    bool next() {
+        if (NULL == fNext) {
+            return false;
+        }
+        fCmd = fNext;
+        fNext = fCmd->fNext;
+        return true;
+    }
+
+    Cmd* operator->() {
+        SkASSERT(fCmd);
+        return fCmd;
+    }
+
+private:
+    Cmd* fCmd;
+    Cmd* fNext;
+};
+
+template<typename TAlign>
+void GrCmdBuffer<TAlign>::reset() {
+    Iter iter(*this);
+    while (iter.next()) {
+        iter->~Cmd();
+    }
+    fHeadBlock.fNext.free();
+    fTailBlock = &fHeadBlock;
+    fBack = 0;
+    fFirstCmd = fLastCmd = NULL;
+}
+
+template<typename TAlign>
+void* GrCmdBuffer<TAlign>::push_back_raw(int sizeInBytes) {
+    const int cmdLength = bytes_to_length(sizeInBytes);
+
+    if (fBack + cmdLength > fTailBlock->fLength) {
+        SkASSERT(NULL == fTailBlock->fNext.get());
+        Block* next = SkNEW_ARGS(Block, (SkTMax(2 * fTailBlock->fLength, cmdLength)));
+        fTailBlock->fNext.reset(next);
+        fTailBlock = next;
+        fBack = 0;
+    }
+
+    void* data = &fTailBlock->fBuffer[fBack];
+    fBack += cmdLength;
+    return data;
+}
+
+template<typename TAlign>
+template<typename T>
+T& GrCmdBuffer<TAlign>::init_back(T* cmd) {
+    cmd->fType = T::kCmdType;
+    if (NULL == fFirstCmd) {
+        SkASSERT(NULL == fLastCmd);
+        fFirstCmd = fLastCmd = cmd;
+    } else {
+        SkASSERT(NULL != fLastCmd);
+        fLastCmd->fNext = cmd;
+        fLastCmd = cmd;
+    }
+    return *cmd;
+}
+
+template<typename TAlign>
+template<typename TCmd>
+TCmd& GrCmdBuffer<TAlign>::push_back_size(int sizeInBytes) {
+    SkASSERT(sizeInBytes >= (int)sizeof(TCmd));
+    TCmd* cmd = SkNEW_PLACEMENT(this->push_back_raw(sizeInBytes), TCmd);
+    return this->init_back(cmd);
+}
+
+template<typename TAlign>
+template<typename TCmd, typename TArg1>
+TCmd& GrCmdBuffer<TAlign>::push_back_size(int sizeInBytes, const TArg1& a) {
+    SkASSERT(sizeInBytes >= (int)sizeof(TCmd));
+    TCmd* cmd = SkNEW_PLACEMENT_ARGS(this->push_back_raw(sizeInBytes), TCmd, (a));
+    return this->init_back(cmd);
+}
+
+template<typename TAlign>
+template<typename TCmd, typename TArg1, typename TArg2>
+TCmd& GrCmdBuffer<TAlign>::push_back_size(int sizeInBytes, const TArg1& a, const TArg2& b) {
+    SkASSERT(sizeInBytes >= (int)sizeof(TCmd));
+    TCmd* cmd = SkNEW_PLACEMENT_ARGS(this->push_back_raw(sizeInBytes), TCmd, (a, b));
+    return this->init_back(cmd);
+}
+
+template<typename TAlign>
+template<typename TCmd, typename TArg1, typename TArg2, typename TArg3>
+TCmd& GrCmdBuffer<TAlign>::push_back_size(int sizeInBytes, const TArg1& a, const TArg2& b, const TArg3& c) {
+    SkASSERT(sizeInBytes >= (int)sizeof(TCmd));
+    TCmd* cmd = SkNEW_PLACEMENT_ARGS(this->push_back_raw(sizeInBytes), TCmd, (a, b, c));
+    return this->init_back(cmd);
+}
+
+#endif