Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2229553004:
cc: Implement UnionTraits for DrawQuads (Closed)
|
Created:
4 years, 4 months ago by Fady Samuel Modified:
4 years, 4 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, cc-bugs_chromium.org, ben+mojo_chromium.org, darin (slow to review) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Implement UnionTraits for DrawQuads
Previously DrawQuad inheritance was modeled in mojom as a composition of
optional fields. This resulted in a lot of validation overhead.
With the newly introduced UnionTraits, we can check the tag indicating the
type to serialize and deserialize once, and only validate that type.
This results in a significant performance win for CompositorFrame serialization.
In many cases, serialization is 20-40% faster than ParamTraits, and
deserialization is 5-10% faster.
This CL also removes PICTURE_CONTENT and INVALID material
types from the mojom as we cannot serialize those types of
materials.
BUG=624459
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/0abffe077031a99cf914b683b327312828764e75
Cr-Commit-Position: refs/heads/master@{#411169}
Patch Set 1 #Patch Set 2 : Fixed comment #
Total comments: 2
Patch Set 3 : Cleanup according to Yuzhu's suggestion #
Total comments: 4
Patch Set 4 : More cleanup: Got rid of material and collapsed a method call #
Total comments: 7
Patch Set 5 : Addressed alphabetization #
Messages
Total messages: 32 (14 generated)
Description was changed from ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. BUG=624459 ========== to ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
fsamuel@chromium.org changed reviewers: + yzshen@chromium.org
How do feel about this Yuzhu? I'm considering moving methods out to the cc file but I also want to inline them, I think.
Description was changed from ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of over validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
https://codereview.chromium.org/2229553004/diff/20001/cc/ipc/quads.mojom File cc/ipc/quads.mojom (right): https://codereview.chromium.org/2229553004/diff/20001/cc/ipc/quads.mojom#newc... cc/ipc/quads.mojom:123: Material material; Could we remove this enum entirely?
PTAL Yuzhu! I've also removed the Null methods since they are no longer used. https://codereview.chromium.org/2229553004/diff/20001/cc/ipc/quads.mojom File cc/ipc/quads.mojom (right): https://codereview.chromium.org/2229553004/diff/20001/cc/ipc/quads.mojom#newc... cc/ipc/quads.mojom:123: Material material; On 2016/08/09 17:42:04, yzshen1 wrote: > Could we remove this enum entirely? Done.
LGTM with two nits Thanks! https://codereview.chromium.org/2229553004/diff/40001/cc/ipc/quads.mojom File cc/ipc/quads.mojom (right): https://codereview.chromium.org/2229553004/diff/40001/cc/ipc/quads.mojom#newc... cc/ipc/quads.mojom:100: enum Material { Could we also remove this definition? https://codereview.chromium.org/2229553004/diff/40001/cc/ipc/quads_struct_tra... File cc/ipc/quads_struct_traits.cc (right): https://codereview.chromium.org/2229553004/diff/40001/cc/ipc/quads_struct_tra... cc/ipc/quads_struct_traits.cc:237: if (!ReadDrawQuad(data, out)) optional nit: it may make sense to merge ReadDrawQuad into this method.
Done, Yuzhu, could you please run this locally? I want to confirm someone other than me is seeing the same performance I'm seeing because I'm a bit skeptical at the improvement. https://codereview.chromium.org/2229553004/diff/40001/cc/ipc/quads.mojom File cc/ipc/quads.mojom (right): https://codereview.chromium.org/2229553004/diff/40001/cc/ipc/quads.mojom#newc... cc/ipc/quads.mojom:100: enum Material { On 2016/08/09 18:29:53, yzshen1 wrote: > Could we also remove this definition? Done. https://codereview.chromium.org/2229553004/diff/40001/cc/ipc/quads_struct_tra... File cc/ipc/quads_struct_traits.cc (right): https://codereview.chromium.org/2229553004/diff/40001/cc/ipc/quads_struct_tra... cc/ipc/quads_struct_traits.cc:237: if (!ReadDrawQuad(data, out)) On 2016/08/09 18:29:53, yzshen1 wrote: > optional nit: it may make sense to merge ReadDrawQuad into this method. Done.
fsamuel@chromium.org changed reviewers: + danakj@chromium.org, tsepez@chromium.org
+danakj@ for cc :-) +tsepez@ for
On 2016/08/09 18:53:35, Fady Samuel wrote: > +danakj@ for cc :-) > +tsepez@ for +tsepez@ for ipc
traits lgtm.
LG but can we use SetAll? https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads.mojom File cc/ipc/quads.mojom (right): https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads.mojom#newc... cc/ipc/quads.mojom:107: StreamVideoQuadState stream_video_quad_state; nit: keep it sorted? easier to find things then. https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads_struct_tra... File cc/ipc/quads_struct_traits.cc (right): https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads_struct_tra... cc/ipc/quads_struct_traits.cc:57: quad->color = data.color(); can you not use the SetAll method on the DrawQuads? This will compile if we add new fields, but SetAll() would not. https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads_struct_tra... cc/ipc/quads_struct_traits.cc:229: return data.ReadDrawQuadState(out); you'll maybe need to pass the base class data to this to use SetAll
PTAL Dana, Yuzhu. Dana, Yuzhu, the DataView API is set up such that I cannot pass its output directly into another function so if I do Dana's suggestion, I would need to create a bunch of temporaries which could impact deserialization performance. WDYT? https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads.mojom File cc/ipc/quads.mojom (right): https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads.mojom#newc... cc/ipc/quads.mojom:107: StreamVideoQuadState stream_video_quad_state; On 2016/08/09 21:37:23, danakj wrote: > nit: keep it sorted? easier to find things then. Done. https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads_struct_tra... File cc/ipc/quads_struct_traits.cc (right): https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads_struct_tra... cc/ipc/quads_struct_traits.cc:57: quad->color = data.color(); On 2016/08/09 21:37:23, danakj wrote: > can you not use the SetAll method on the DrawQuads? This will compile if we add > new fields, but SetAll() would not. I've actually deliberately punted on using SetAll. My concern is the DataView API takes in an out parameter which means I need to create a temporary variable to store the output and then pass it along to SetAll. That will potentially add overhead. Maybe the compiler might be smart enough to optimize that out, but I suspect not, especially if SetAll is not inlined. +yzshen@ for thoughts? https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads_struct_tra... cc/ipc/quads_struct_traits.cc:229: return data.ReadDrawQuadState(out); On 2016/08/09 21:37:23, danakj wrote: > you'll maybe need to pass the base class data to this to use SetAll See comment above. I think we should avoid using SetAll here.
https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads_struct_tra... File cc/ipc/quads_struct_traits.cc (right): https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads_struct_tra... cc/ipc/quads_struct_traits.cc:57: quad->color = data.color(); On 2016/08/10 13:29:03, Fady Samuel wrote: > On 2016/08/09 21:37:23, danakj wrote: > > can you not use the SetAll method on the DrawQuads? This will compile if we > add > > new fields, but SetAll() would not. > > I've actually deliberately punted on using SetAll. My concern is the DataView > API takes in an out parameter which means I need to create a temporary variable > to store the output and then pass it along to SetAll. I think you are talking about things like gfx::Rects? Yeah, that will probably make a copy. > That will potentially add > overhead. Maybe the compiler might be smart enough to optimize that out, but I > suspect not, especially if SetAll is not inlined. Not sure how big the impact of SetAll not being inlined is. It is probably small but without data I cannot really tell. Overall, the current approach looks good to me. > > +yzshen@ for thoughts?
On Wed, Aug 10, 2016 at 6:29 AM, <fsamuel@chromium.org> wrote: > PTAL Dana, Yuzhu. > > Dana, Yuzhu, the DataView API is set up such that I cannot pass its output > directly into another function so if I do Dana's suggestion, I would need > to > create a bunch of temporaries which could impact deserialization > performance. > WDYT? > You could measure and see? Is there a diff api on DrawQuads that could provide the same compiler guarantees but be more mojo-friendly if it is slower to use SetAll? > > > > https://codereview.chromium.org/2229553004/diff/60001/cc/ipc/quads.mojom > File cc/ipc/quads.mojom (right): > > https://codereview.chromium.org/2229553004/diff/60001/cc/ > ipc/quads.mojom#newcode107 > cc/ipc/quads.mojom:107: StreamVideoQuadState stream_video_quad_state; > On 2016/08/09 21:37:23, danakj wrote: > > nit: keep it sorted? easier to find things then. > > Done. > > https://codereview.chromium.org/2229553004/diff/60001/cc/ > ipc/quads_struct_traits.cc > File cc/ipc/quads_struct_traits.cc (right): > > https://codereview.chromium.org/2229553004/diff/60001/cc/ > ipc/quads_struct_traits.cc#newcode57 > cc/ipc/quads_struct_traits.cc:57: quad->color = data.color(); > On 2016/08/09 21:37:23, danakj wrote: > > can you not use the SetAll method on the DrawQuads? This will compile > if we add > > new fields, but SetAll() would not. > > I've actually deliberately punted on using SetAll. My concern is the > DataView API takes in an out parameter which means I need to create a > temporary variable to store the output and then pass it along to SetAll. > That will potentially add overhead. Maybe the compiler might be smart > enough to optimize that out, but I suspect not, especially if SetAll is > not inlined. > > +yzshen@ for thoughts? > > https://codereview.chromium.org/2229553004/diff/60001/cc/ > ipc/quads_struct_traits.cc#newcode229 > cc/ipc/quads_struct_traits.cc:229: return data.ReadDrawQuadState(out); > On 2016/08/09 21:37:23, danakj wrote: > > you'll maybe need to pass the base class data to this to use SetAll > > See comment above. I think we should avoid using SetAll here. > > https://codereview.chromium.org/2229553004/ > -- 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.
offline summary: param traits are also not using SetAll, what *is* using SetAll is tests, so if a new field is added, tests will fail to compile, and would fail to see the field serialized if it wasn't also added to the mojom stuff. So that should be sufficient. LGTM
The CQ bit was checked by fsamuel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2229553004/#ps80001 (title: "Addressed alphabetization")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fsamuel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Implement UnionTraits for DrawQuads Previously DrawQuad inheritance was modeled in mojom as a composition of optional fields. This resulted in a lot of validation overhead. With the newly introduced UnionTraits, we can check the tag indicating the type to serialize and deserialize once, and only validate that type. This results in a significant performance win for CompositorFrame serialization. In many cases, serialization is 20-40% faster than ParamTraits, and deserialization is 5-10% faster. This CL also removes PICTURE_CONTENT and INVALID material types from the mojom as we cannot serialize those types of materials. BUG=624459 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/0abffe077031a99cf914b683b327312828764e75 Cr-Commit-Position: refs/heads/master@{#411169} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0abffe077031a99cf914b683b327312828764e75 Cr-Commit-Position: refs/heads/master@{#411169} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
