Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2210673002:
Improve gfx::Transform StructTraits. (Closed)
|
Created:
4 years, 4 months ago by yzshen1 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, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove gfx::Transform StructTraits.
This change significantly improves the performance of cc_serialization_perftest:
Two test cases are mostly not affected. For the rest, the serialization and deserialization performance is improved by 10%~20%.
BUG=634024
Committed: https://crrev.com/9cbbefdc8f2ee2b82b21d188b29bd9ae2629da6f
Cr-Commit-Position: refs/heads/master@{#409898}
Patch Set 1 #
Total comments: 2
Patch Set 2 : make gn check happy #Patch Set 3 : . #
Total comments: 2
Messages
Total messages: 28 (17 generated)
The CQ bit was checked by yzshen@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Improve gfx::Transform StructTraits. This change significantly improves the performance of cc_serialization_perftest. BUG=634024 ========== to ========== Improve gfx::Transform StructTraits. This change significantly improves the performance of cc_serialization_perftest: Two test cases are mostly not affected. For the rest, the serialization and deserialization performance is improved by 10%~20%. BUG=634024 ==========
yzshen@chromium.org changed reviewers: + fsamuel@chromium.org, tsepez@chromium.org
Average numbers from three runs of cc_serialization_perftest: Serialization: ========================================== ' StructTraits serialization: num runs in 2 seconds: DelegatedFrame_ManyQuads_100000_100000': base: 69.0 new: 78.66666666666667 imprv: 1.140096618357488 ' StructTraits serialization: num runs in 2 seconds: DelegatedFrame_ManyQuads_1_100000': base: 143.0 new: 142.0 imprv: 0.993006993006993 ' StructTraits serialization: num runs in 2 seconds: DelegatedFrame_ManyQuads_1_4000': base: 3769.3333333333335 new: 3702.0 imprv: 0.9821365405022993 ' StructTraits serialization: num runs in 2 seconds: DelegatedFrame_ManyQuads_4000_4000': base: 1940.0 new: 2297.6666666666665 imprv: 1.184364261168385 ' StructTraits serialization: num runs in 2 seconds: DelegatedFrame_ManyRenderPasses_10000_100': base: 67.66666666666667 new: 80.33333333333333 imprv: 1.1871921182266008 ' StructTraits serialization: num runs in 2 seconds: DelegatedFrame_ManyRenderPasses_10_500': base: 1519.6666666666667 new: 1809.0 imprv: 1.190392629962711 ' StructTraits serialization: num runs in 2 seconds: DelegatedFrame_ManyRenderPasses_5_100': base: 14767.333333333334 new: 18256.333333333332 imprv: 1.2362647284546973 Deserialization: ========================================== ' StructTraits deserialization: num runs in 2 seconds: DelegatedFrame_ManyQuads_100000_100000': base: 48.666666666666664 new: 55.333333333333336 imprv: 1.1369863013698631 ' StructTraits deserialization: num runs in 2 seconds: DelegatedFrame_ManyQuads_1_100000': base: 111.33333333333333 new: 109.66666666666667 imprv: 0.9850299401197605 ' StructTraits deserialization: num runs in 2 seconds: DelegatedFrame_ManyQuads_1_4000': base: 3262.3333333333335 new: 3346.3333333333335 imprv: 1.0257484418105651 ' StructTraits deserialization: num runs in 2 seconds: DelegatedFrame_ManyQuads_4000_4000': base: 1502.6666666666667 new: 1789.3333333333333 imprv: 1.1907719609582963 ' StructTraits deserialization: num runs in 2 seconds: DelegatedFrame_ManyRenderPasses_10000_100': base: 46.333333333333336 new: 53.666666666666664 imprv: 1.158273381294964 ' StructTraits deserialization: num runs in 2 seconds: DelegatedFrame_ManyRenderPasses_10_500': base: 1331.0 new: 1505.3333333333333 imprv: 1.1309792136238417 ' StructTraits deserialization: num runs in 2 seconds: DelegatedFrame_ManyRenderPasses_5_100': base: 14651.333333333334 new: 17997.0 imprv: 1.2283523683851298 ========================================== |base|: without CL |new|: with CL |imprv|: |new|/|base|
The CQ bit was checked by yzshen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM + question https://codereview.chromium.org/2210673002/diff/1/mojo/public/tools/bindings/... File mojo/public/tools/bindings/generators/cpp_templates/struct_declaration.tmpl (right): https://codereview.chromium.org/2210673002/diff/1/mojo/public/tools/bindings/... mojo/public/tools/bindings/generators/cpp_templates/struct_declaration.tmpl:51: {{class_name}}() { Does inlining the constructor help here? Can you add these things to the initializer list?
The CQ bit was checked by yzshen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2210673002/diff/1/mojo/public/tools/bindings/... File mojo/public/tools/bindings/generators/cpp_templates/struct_declaration.tmpl (right): https://codereview.chromium.org/2210673002/diff/1/mojo/public/tools/bindings/... mojo/public/tools/bindings/generators/cpp_templates/struct_declaration.tmpl:51: {{class_name}}() { On 2016/08/03 23:44:05, Fady Samuel wrote: > Does inlining the constructor help here? Can you add these things to the > initializer list? It doesn't help much actually. I saw some New() calls (line 5, which calls this constructor) showed up in profiling. So I made this change. I have changed to use initializer list. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
yzshen@chromium.org changed reviewers: + sky@chromium.org
Hi, Tom and Scott. Would you please take a look? Thanks! Tom: struct_traits review. Scott: owner review.
https://codereview.chromium.org/2210673002/diff/40001/ui/gfx/mojo/transform_s... File ui/gfx/mojo/transform_struct_traits.h (right): https://codereview.chromium.org/2210673002/diff/40001/ui/gfx/mojo/transform_s... ui/gfx/mojo/transform_struct_traits.h:34: data.GetMatrixDataView(&matrix); How do we know that data is big enough, e.g well-formed coming off the wire?
https://codereview.chromium.org/2210673002/diff/40001/ui/gfx/mojo/transform_s... File ui/gfx/mojo/transform_struct_traits.h (right): https://codereview.chromium.org/2210673002/diff/40001/ui/gfx/mojo/transform_s... ui/gfx/mojo/transform_struct_traits.h:34: data.GetMatrixDataView(&matrix); On 2016/08/04 17:36:03, Tom Sepez wrote: > How do we know that data is big enough, e.g well-formed coming off the wire? Before we reach here, the standard validation logic has been run. If it is not well-formed, the message has already been rejected and we won't get here.
On 2016/08/04 17:38:16, yzshen1 wrote: > https://codereview.chromium.org/2210673002/diff/40001/ui/gfx/mojo/transform_s... > File ui/gfx/mojo/transform_struct_traits.h (right): > > https://codereview.chromium.org/2210673002/diff/40001/ui/gfx/mojo/transform_s... > ui/gfx/mojo/transform_struct_traits.h:34: data.GetMatrixDataView(&matrix); > On 2016/08/04 17:36:03, Tom Sepez wrote: > > How do we know that data is big enough, e.g well-formed coming off the wire? > > Before we reach here, the standard validation logic has been run. If it is not > well-formed, the message has already been rejected and we won't get here. lgtm
LGTM
The CQ bit was checked by yzshen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2210673002/#ps40001 (title: ".")
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 ========== Improve gfx::Transform StructTraits. This change significantly improves the performance of cc_serialization_perftest: Two test cases are mostly not affected. For the rest, the serialization and deserialization performance is improved by 10%~20%. BUG=634024 ========== to ========== Improve gfx::Transform StructTraits. This change significantly improves the performance of cc_serialization_perftest: Two test cases are mostly not affected. For the rest, the serialization and deserialization performance is improved by 10%~20%. BUG=634024 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Improve gfx::Transform StructTraits. This change significantly improves the performance of cc_serialization_perftest: Two test cases are mostly not affected. For the rest, the serialization and deserialization performance is improved by 10%~20%. BUG=634024 ========== to ========== Improve gfx::Transform StructTraits. This change significantly improves the performance of cc_serialization_perftest: Two test cases are mostly not affected. For the rest, the serialization and deserialization performance is improved by 10%~20%. BUG=634024 Committed: https://crrev.com/9cbbefdc8f2ee2b82b21d188b29bd9ae2629da6f Cr-Commit-Position: refs/heads/master@{#409898} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9cbbefdc8f2ee2b82b21d188b29bd9ae2629da6f Cr-Commit-Position: refs/heads/master@{#409898} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
