Speed up quad deserialization.

Speed up quad deserialization.

Separate the largest subclass quad size for allocation and serialization since they are different numbers.

BUG=644443
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

[hubbe, 2016-09-08 06:47:11]

Description was changed from

==========
Speed up quad deserialization.

Separate the largest subclass quad size for allocation and serialization since
they are different numbers.

BUG=644443
==========

to

==========
Speed up quad deserialization.

Separate the largest subclass quad size for allocation and serialization since
they are different numbers.

BUG=644443
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[hubbe, 2016-09-08 06:51:20]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 06:51:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 07:06:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 07:06:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hubbe, 2016-09-08 17:42:48]

The CQ bit was checked by hubbe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 17:43:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-09-08 18:57:12]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-09-08 18:57:13]

One drive-by comment

Thanks for working on this!

https://codereview.chromium.org/2324723002/diff/20001/cc/quads/largest_draw_q...
File cc/quads/largest_draw_quad.h (right):

https://codereview.chromium.org/2324723002/diff/20001/cc/quads/largest_draw_q...
cc/quads/largest_draw_quad.h:15: CC_EXPORT size_t
LargestDrawQuadSerializedSize();
does it make sense to add comment that this "serialized size" is only used by
the IPC::ParamTraits, but not Mojo serialization?

[commit-bot: I haz the power, 2016-09-08 18:57:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 18:57:39]

Dry run: This issue passed the CQ dry run.

[hubbe1, 2016-09-08 21:05:46]

hubbe@google.com changed reviewers:
+ hubbe@google.com

[hubbe1, 2016-09-08 21:05:47]

https://codereview.chromium.org/2324723002/diff/20001/cc/quads/largest_draw_q...
File cc/quads/largest_draw_quad.h (right):

https://codereview.chromium.org/2324723002/diff/20001/cc/quads/largest_draw_q...
cc/quads/largest_draw_quad.h:15: CC_EXPORT size_t
LargestDrawQuadSerializedSize();
On 2016/09/08 18:57:13, yzshen1 wrote:
> does it make sense to add comment that this "serialized size" is only used by
> the IPC::ParamTraits, but not Mojo serialization?
> 

Probably, assuming that's actually true. :)

[hubbe, 2016-09-08 21:08:51]

hubbe@chromium.org changed reviewers:
+ danakj@chromium.org
- hubbe@google.com, yzshen@chromium.org

[danakj, 2016-09-08 21:14:26]

I'm going to have to understand https://codereview.chromium.org/2229953003
first. It's claiming the largest quad is YUVQuad + ColorSpace. If ColorSpace is
part of the quad then it's already accounted for. If it's not then it's not part
of the largest size. I'm super confused why that was done, and suspect that it's
not what you intended. Esp cuz that CL does not actually change the defn of any
DrawQuads.

[hubbe, 2016-09-08 21:25:21]

On 2016/09/08 21:14:26, danakj wrote:
> I'm going to have to understand https://codereview.chromium.org/2229953003
> first. It's claiming the largest quad is YUVQuad + ColorSpace. If ColorSpace
is
> part of the quad then it's already accounted for. If it's not then it's not
part
> of the largest size. I'm super confused why that was done, and suspect that
it's
> not what you intended. Esp cuz that CL does not actually change the defn of
any
> DrawQuads.

Hmm. A valid point.
I was thinking that the YuvVideoDrawQuad contained a pointer to a
gfx::ColorSpace, but that's not the case.
My earlier CL was reverted because of an IPC encoding error, so I had thought
that I needed to increase the
largest quad size to fix it, which it did. However, I now realize that my
assumptions were incorrect.

Unfortunately the ipc error only occured in some obscure webrtc test, so it's
not exactly easy to reproduce.
It seems like just removing the "+ sizeof(gfx::ColorSpace)" should work, but
will likely trigger the previous
IPC error, but perhaps I should focus on trying to figure that one out
instead....

[danakj, 2016-09-08 21:27:38]

On Thu, Sep 8, 2016 at 2:25 PM, <hubbe@chromium.org> wrote:

> On 2016/09/08 21:14:26, danakj wrote:
> > I'm going to have to understand https://codereview.chromium.
> org/2229953003
> > first. It's claiming the largest quad is YUVQuad + ColorSpace. If
> ColorSpace
> is
> > part of the quad then it's already accounted for. If it's not then it's
> not
> part
> > of the largest size. I'm super confused why that was done, and suspect
> that
> it's
> > not what you intended. Esp cuz that CL does not actually change the defn
> of
> any
> > DrawQuads.
>
> Hmm. A valid point.
> I was thinking that the YuvVideoDrawQuad contained a pointer to a
> gfx::ColorSpace, but that's not the case.
> My earlier CL was reverted because of an IPC encoding error, so I had
> thought
> that I needed to increase the
> largest quad size to fix it, which it did. However, I now realize that my
> assumptions were incorrect.
>
> Unfortunately the ipc error only occured in some obscure webrtc test, so
> it's
> not exactly easy to reproduce.
> It seems like just removing the "+ sizeof(gfx::ColorSpace)" should work,
> but
> will likely trigger the previous
> IPC error, but perhaps I should focus on trying to figure that one out
> instead....
>

Ya I think so. (And ya, DrawQuads can't have pointers for IPC reasons)


>
>
> https://codereview.chromium.org/2324723002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hubbe, 2016-09-10 05:10:52]

Description was changed from

==========
Speed up quad deserialization.

Separate the largest subclass quad size for allocation and serialization since
they are different numbers.

BUG=644443
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Speed up quad deserialization.

Separate the largest subclass quad size for allocation and serialization since
they are different numbers.

BUG=644443
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========