Modified GPU command signature hash to use a binary representation.

Modified GPU command signature hash to use a binary representation.

FrameBuffer::GetStatus() was spending most of the time doing in printf,
I've modified it to generate signatures using a binary representation
instead. Now most of the time is spent allocating memory. If further
optimization is necessary we can precalculate the signature size
as well.

BUG= https://code.google.com/p/chromium/issues/detail?id=338035
TEST= profile FrameBuffer::GetStatus()

Committed: https://crrev.com/cb0173111c9234f9718ed14cca6fbf45841deac7
Cr-Commit-Position: refs/heads/master@{#297209}

[David Yen, 2014-09-23 18:53:05]

dyen@chromium.org changed reviewers:
+ vmiura@chromium.org

[David Yen, 2014-09-23 18:53:06]

I was not able to get the exact numbers you were, but with this change it
dropped my numbers GetStatus() numbers from 0.0427ms to 0.0149ms.

Original trace %:
61% spent in StringPrintf
28.2% spent in append

New trace %:
Removed StringPrintf
60% spent in append

If necessary, we can further improve the time by adding a "GetSignatureSize"
function at each level and using it to preallocate the signature string before
we do any appending.

[vmiura, 2014-09-23 20:26:48]

Nice!  I'll run a profile with this.

I've made some additional suggestions.

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:509: std::string signature;
I think it would help to pre-reserve enough space for a typical framebuffer e.g.
1 texture for color buffer + 2 * renderbuffers for depth & stencil.

i.e. signature.reserve(xyz);

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:513: signature.push_back('|');
I think we could remove fillers like "|Target|" and "|".

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/r...
File gpu/command_buffer/service/renderbuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/r...
gpu/command_buffer/service/renderbuffer_manager.cc:56:
signature->append(reinterpret_cast<const char*>(&height_), sizeof(height_));
Could this be improved by packing the values in a struct, then appending to the
signature in one go?  Or if the values were already in a struct ready to append?

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:263: sizeof(usage_));
As with Renderbuffer, would these be better if packed into structs that can be
appended?

[David Yen, 2014-09-23 20:45:24]

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:509: std::string signature;
On 2014/09/23 20:26:47, vmiura wrote:
> I think it would help to pre-reserve enough space for a typical framebuffer
e.g.
> 1 texture for color buffer + 2 * renderbuffers for depth & stencil.
> 
> i.e. signature.reserve(xyz);

Since I will be adding structures for the binary blobs, I'll just implement a
function to return the size of the structures.

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:513: signature.push_back('|');
On 2014/09/23 20:26:47, vmiura wrote:
> I think we could remove fillers like "|Target|" and "|".

I think it's a good idea to keep marker tags to ensure the binary data will be
unique, this can be a problem especially when having binary blobs containing
enums.

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/r...
File gpu/command_buffer/service/renderbuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/r...
gpu/command_buffer/service/renderbuffer_manager.cc:56:
signature->append(reinterpret_cast<const char*>(&height_), sizeof(height_));
On 2014/09/23 20:26:47, vmiura wrote:
> Could this be improved by packing the values in a struct, then appending to
the
> signature in one go?  Or if the values were already in a struct ready to
append?

I'll create a structure to do this.

[David Yen, 2014-09-23 22:22:28]

On 2014/09/23 20:45:24, David Yen wrote:
>
https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
> File gpu/command_buffer/service/framebuffer_manager.cc (right):
> 
>
https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
> gpu/command_buffer/service/framebuffer_manager.cc:509: std::string signature;
> On 2014/09/23 20:26:47, vmiura wrote:
> > I think it would help to pre-reserve enough space for a typical framebuffer
> e.g.
> > 1 texture for color buffer + 2 * renderbuffers for depth & stencil.
> > 
> > i.e. signature.reserve(xyz);
> 
> Since I will be adding structures for the binary blobs, I'll just implement a
> function to return the size of the structures.
> 
>
https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
> gpu/command_buffer/service/framebuffer_manager.cc:513:
signature.push_back('|');
> On 2014/09/23 20:26:47, vmiura wrote:
> > I think we could remove fillers like "|Target|" and "|".
> 
> I think it's a good idea to keep marker tags to ensure the binary data will be
> unique, this can be a problem especially when having binary blobs containing
> enums.
> 
>
https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/r...
> File gpu/command_buffer/service/renderbuffer_manager.cc (right):
> 
>
https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/r...
> gpu/command_buffer/service/renderbuffer_manager.cc:56:
> signature->append(reinterpret_cast<const char*>(&height_), sizeof(height_));
> On 2014/09/23 20:26:47, vmiura wrote:
> > Could this be improved by packing the values in a struct, then appending to
> the
> > signature in one go?  Or if the values were already in a struct ready to
> append?
> 
> I'll create a structure to do this.

I've added the function to get the structure size and now do a reserve
beforehand, it looks like on my Nexus 5 the GetStatus() call has been reduced to
0.00574ms. This makes sense since allocations were taking roughly 60% before.

[vmiura, 2014-09-23 22:37:17]

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:513: signature.push_back('|');
On 2014/09/23 20:45:24, David Yen wrote:
> On 2014/09/23 20:26:47, vmiura wrote:
> > I think we could remove fillers like "|Target|" and "|".
> 
> I think it's a good idea to keep marker tags to ensure the binary data will be
> unique, this can be a problem especially when having binary blobs containing
> enums.

I'd think we need to keep them in the attachments to avoid collisions, but this
"|Target|" ... "|" is a constant that I think we can remove.

[David Yen, 2014-09-23 22:42:51]

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager.cc:513: signature.push_back('|');
On 2014/09/23 22:37:17, vmiura wrote:
> On 2014/09/23 20:45:24, David Yen wrote:
> > On 2014/09/23 20:26:47, vmiura wrote:
> > > I think we could remove fillers like "|Target|" and "|".
> > 
> > I think it's a good idea to keep marker tags to ensure the binary data will
be
> > unique, this can be a problem especially when having binary blobs containing
> > enums.
> 
> I'd think we need to keep them in the attachments to avoid collisions, but
this
> "|Target|" ... "|" is a constant that I think we can remove.

Done. Removed it in the last patch.

[David Yen, 2014-09-24 16:37:45]

On 2014/09/23 22:42:51, David Yen wrote:
>
https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
> File gpu/command_buffer/service/framebuffer_manager.cc (right):
> 
>
https://codereview.chromium.org/593233002/diff/1/gpu/command_buffer/service/f...
> gpu/command_buffer/service/framebuffer_manager.cc:513:
signature.push_back('|');
> On 2014/09/23 22:37:17, vmiura wrote:
> > On 2014/09/23 20:45:24, David Yen wrote:
> > > On 2014/09/23 20:26:47, vmiura wrote:
> > > > I think we could remove fillers like "|Target|" and "|".
> > > 
> > > I think it's a good idea to keep marker tags to ensure the binary data
will
> be
> > > unique, this can be a problem especially when having binary blobs
containing
> > > enums.
> > 
> > I'd think we need to keep them in the attachments to avoid collisions, but
> this
> > "|Target|" ... "|" is a constant that I think we can remove.
> 
> Done. Removed it in the last patch.

I've added a constructor for both structs (Along with a comment) which memsets
the structure before assigning the variables. It's a bit ugly, but I can't think
of a cleaner way to do it.

[vmiura, 2014-09-26 23:36:23]

LGTM modulo nits.

[vmiura, 2014-09-26 23:36:36]

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/framebuffer_manager.cc:538: }
nit: DCHECK(signature.size() == signature_size) will help this from regressing.

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/renderbuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/renderbuffer_manager.cc:17: // The should contain
everything to uniquely identify a Renderbuffer.
nit: This

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/texture_manager.cc:24: // The should contain
everything to uniquely identify a Texture.
nit: This

[David Yen, 2014-09-29 16:42:58]

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/framebuffer_manager.cc:538: }
On 2014/09/26 23:36:36, vmiura wrote:
> nit: DCHECK(signature.size() == signature_size) will help this from
regressing.

Good idea. Done.

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/renderbuffer_manager.cc (right):

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/renderbuffer_manager.cc:17: // The should contain
everything to uniquely identify a Renderbuffer.
On 2014/09/26 23:36:36, vmiura wrote:
> nit: This

Done.

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/593233002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/texture_manager.cc:24: // The should contain
everything to uniquely identify a Texture.
On 2014/09/26 23:36:36, vmiura wrote:
> nit: This

Done.

[David Yen, 2014-09-29 17:36:26]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2014-09-29 17:37:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/593233002/60001

[commit-bot: I haz the power, 2014-09-29 18:16:28]

Committed patchset #4 (id:60001) as c2a116e9f37dee8edc326c2c575454896de51de8

[commit-bot: I haz the power, 2014-09-29 18:17:42]

Patchset 4 (id:??) landed as
https://crrev.com/cb0173111c9234f9718ed14cca6fbf45841deac7
Cr-Commit-Position: refs/heads/master@{#297209}