Mojo Bindings perf regression from switch to libc++ |
|||||
Issue descriptionWe use cc_perftests (e.g. CCSerializationPerfTest.DelegatedFrame_Complex) as good a way to benchmark Mojo bindings serialization performance. Recently I noticed that we've have a substantial regression in performance here, and I bisected it to the Linux switch to libc++: https://chromium.googlesource.com/chromium/src/+/39fcd6af4d8c9ddcf909b8d489c6ba684d046157 I haven't done any more in-depth investigation yet, but this is surprising and significant: it's a consistent ~3.5x slowdown.
,
Jul 21 2017
+CC Fady: Looking at the perf graphs, the cc_perftests was no longer run on our Linux perf bot (or failed to upload data) since January 2017. Otherwise, we would have caught this regression when it happened.
,
Jul 21 2017
+weiliangc@ who has worked on CCSerializationPerfTest recently.
,
Jul 21 2017
cc_perftests has been taking too long/too flaky and people didn't have time to maintain those microbenchmarks on the perf bots. If that one test is good for benchmarking Mojo bindings serialization performance, would it make sense to pull it out of cc perftests into mojo perftests?
,
Jul 21 2017
It depends on all those cc types. Does it make sense to pull it out of cc_perftests into, say, cc_serialization_perftests, and still put it under cc/? This test shouldn't take long or be flaky.
,
Jul 21 2017
My linux profiling-fu is pretty weak, but it looks like way more time is being spent in std::vector allocation with libc++
,
Jul 21 2017
Nevermind... misreading symbols. I don't have useful data yet.
,
Jul 21 2017
I like a cc_serialization_perftests too
,
Jul 21 2017
One thing I'm noticing is that the perftests actually do an extra unnecessary memcpy in the test code for the StructTraits case but not the ParamTraits case - i.e. after serialization, we copy the data out of the mojom message object but don't copy it out of the IPC::Message object. Eliminating this shaves 25% off of the StructTraits cost, and the bulk of that cost does in fact seem to come from allocating the target vector.
,
Jul 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8a0f44c5d3ccc6f345472ffd9591c9d748b90618 commit 8a0f44c5d3ccc6f345472ffd9591c9d748b90618 Author: Ken Rockot <rockot@chromium.org> Date: Wed Jul 26 00:30:51 2017 Mojo bindings: API to serialize struct -> Message This allows callers to serialize a mojom struct directly into a Message object, ensuring that the struct's on-demand serialization logic can mirror what is used in production bindings code when serializing full messages. Updates cc_perftests to use this API in its serialization perf tests, eliminating a large amount of redundant allocation, writing, and copying that is not done by the analogous ParamTraits test cases. This does not resolve the Linux perf regression introduced by switching to libc++, but it does bring the work done by StructTraits test cases in line with the work done by ParamTraits test cases, revealing the actual regression to be quite a bit smaller (~2x rather than ~3.5x). BUG=747525 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ia0cd5cdaa9d92e210ffbc9a3b031431fff781fb5 Reviewed-on: https://chromium-review.googlesource.com/584004 Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: Yuzhu Shen <yzshen@chromium.org> Commit-Queue: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#489497} [modify] https://crrev.com/8a0f44c5d3ccc6f345472ffd9591c9d748b90618/cc/ipc/cc_serialization_perftest.cc [modify] https://crrev.com/8a0f44c5d3ccc6f345472ffd9591c9d748b90618/mojo/public/cpp/bindings/lib/serialization.h [modify] https://crrev.com/8a0f44c5d3ccc6f345472ffd9591c9d748b90618/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
,
Jul 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8aadba6567f0e484c76c3ca19a1303c3c717cef commit e8aadba6567f0e484c76c3ca19a1303c3c717cef Author: Pavel Kalinnikov <pkalinnikov@chromium.org> Date: Wed Jul 26 09:48:54 2017 Revert "Mojo bindings: API to serialize struct -> Message" This reverts commit 8a0f44c5d3ccc6f345472ffd9591c9d748b90618. Reason for revert: Might be the cause of StructTest.Serialization_PublicAPI failing on "Win7 Tests (dbg)": https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/61850 Original change's description: > Mojo bindings: API to serialize struct -> Message > > This allows callers to serialize a mojom struct directly into a Message > object, ensuring that the struct's on-demand serialization logic can > mirror what is used in production bindings code when serializing full > messages. > > Updates cc_perftests to use this API in its serialization perf tests, > eliminating a large amount of redundant allocation, writing, and copying > that is not done by the analogous ParamTraits test cases. > > This does not resolve the Linux perf regression introduced by switching > to libc++, but it does bring the work done by StructTraits test cases in > line with the work done by ParamTraits test cases, revealing the actual > regression to be quite a bit smaller (~2x rather than ~3.5x). > > BUG=747525 > > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel > Change-Id: Ia0cd5cdaa9d92e210ffbc9a3b031431fff781fb5 > Reviewed-on: https://chromium-review.googlesource.com/584004 > Reviewed-by: danakj <danakj@chromium.org> > Reviewed-by: Yuzhu Shen <yzshen@chromium.org> > Commit-Queue: Ken Rockot <rockot@chromium.org> > Cr-Commit-Position: refs/heads/master@{#489497} TBR=danakj@chromium.org,rockot@chromium.org,yzshen@chromium.org Change-Id: Ic006cf30fc186e720b334fcbe7280e075027a9ec No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 747525 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/586267 Reviewed-by: Pavel Kalinnikov <pkalinnikov@chromium.org> Commit-Queue: Pavel Kalinnikov <pkalinnikov@chromium.org> Cr-Commit-Position: refs/heads/master@{#489593} [modify] https://crrev.com/e8aadba6567f0e484c76c3ca19a1303c3c717cef/cc/ipc/cc_serialization_perftest.cc [modify] https://crrev.com/e8aadba6567f0e484c76c3ca19a1303c3c717cef/mojo/public/cpp/bindings/lib/serialization.h [modify] https://crrev.com/e8aadba6567f0e484c76c3ca19a1303c3c717cef/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6a0615dfc0f68a2cad6ec8e1a60a775f0efa9c6 commit a6a0615dfc0f68a2cad6ec8e1a60a775f0efa9c6 Author: Ken Rockot <rockot@chromium.org> Date: Thu Jul 27 17:31:47 2017 Reland "Mojo bindings: API to serialize struct -> Message" Reland of https://chromium-review.googlesource.com/c/584004, which was reverted due to a failure on a Win7 builder still using VC. The problem was an invalid call to std::vector::front() on a potentially empty vector, which triggers an assertion in VC debug builds but is silently ignored in clang builds. This reland fixes the problem by avoiding said call when the vector in question is empty. BUG=747525 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Icc098223b125260a944146729d128f6b6e80ffde TBR=danakj@chromium.org TBR=yzshen@chromium.org Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Icc098223b125260a944146729d128f6b6e80ffde Reviewed-on: https://chromium-review.googlesource.com/587406 Commit-Queue: Ken Rockot <rockot@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#490011} [modify] https://crrev.com/a6a0615dfc0f68a2cad6ec8e1a60a775f0efa9c6/cc/ipc/cc_serialization_perftest.cc [modify] https://crrev.com/a6a0615dfc0f68a2cad6ec8e1a60a775f0efa9c6/mojo/public/cpp/bindings/lib/serialization.h [modify] https://crrev.com/a6a0615dfc0f68a2cad6ec8e1a60a775f0efa9c6/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b77c36ce87e2fb4ab23daea9dd13906241bbd2b commit 8b77c36ce87e2fb4ab23daea9dd13906241bbd2b Author: Ken Rockot <rockot@chromium.org> Date: Thu Jul 27 18:28:32 2017 Revert "Reland "Mojo bindings: API to serialize struct -> Message"" This reverts commit a6a0615dfc0f68a2cad6ec8e1a60a775f0efa9c6. Reason for revert: :( Have to revert the bindings change again due to another uninitialized byte hiding some padding. This depends on that, so, revert. Original change's description: > Reland "Mojo bindings: API to serialize struct -> Message" > > Reland of https://chromium-review.googlesource.com/c/584004, which > was reverted due to a failure on a Win7 builder still using VC. > > The problem was an invalid call to std::vector::front() on a > potentially empty vector, which triggers an assertion in VC debug > builds but is silently ignored in clang builds. > > This reland fixes the problem by avoiding said call when the > vector in question is empty. > > BUG=747525 > > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel > Change-Id: Icc098223b125260a944146729d128f6b6e80ffde > > TBR=danakj@chromium.org > TBR=yzshen@chromium.org > > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel > Change-Id: Icc098223b125260a944146729d128f6b6e80ffde > Reviewed-on: https://chromium-review.googlesource.com/587406 > Commit-Queue: Ken Rockot <rockot@chromium.org> > Reviewed-by: Ken Rockot <rockot@chromium.org> > Cr-Commit-Position: refs/heads/master@{#490011} TBR=danakj@chromium.org,rockot@chromium.org,yzshen@chromium.org Change-Id: I45f9728c3ee040a1c0c590ae69607f83d6f60cff No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 747525 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Reviewed-on: https://chromium-review.googlesource.com/590095 Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#490019} [modify] https://crrev.com/8b77c36ce87e2fb4ab23daea9dd13906241bbd2b/cc/ipc/cc_serialization_perftest.cc [modify] https://crrev.com/8b77c36ce87e2fb4ab23daea9dd13906241bbd2b/mojo/public/cpp/bindings/lib/serialization.h [modify] https://crrev.com/8b77c36ce87e2fb4ab23daea9dd13906241bbd2b/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
,
Jul 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/152536c49f55b8be4758892c0f44f336145cca67 commit 152536c49f55b8be4758892c0f44f336145cca67 Author: Ken Rockot <rockot@chromium.org> Date: Fri Jul 28 18:22:38 2017 Reland "Reland "Mojo bindings: API to serialize struct -> Message"" This is a reland of a6a0615dfc0f68a2cad6ec8e1a60a775f0efa9c6 Original change's description: > Reland "Mojo bindings: API to serialize struct -> Message" > > Reland of https://chromium-review.googlesource.com/c/584004, which > was reverted due to a failure on a Win7 builder still using VC. > > The problem was an invalid call to std::vector::front() on a > potentially empty vector, which triggers an assertion in VC debug > builds but is silently ignored in clang builds. > > This reland fixes the problem by avoiding said call when the > vector in question is empty. > > BUG=747525 > > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel > Change-Id: Icc098223b125260a944146729d128f6b6e80ffde > > TBR=danakj@chromium.org > TBR=yzshen@chromium.org > > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel > Change-Id: Icc098223b125260a944146729d128f6b6e80ffde > Reviewed-on: https://chromium-review.googlesource.com/587406 > Commit-Queue: Ken Rockot <rockot@chromium.org> > Reviewed-by: Ken Rockot <rockot@chromium.org> > Cr-Commit-Position: refs/heads/master@{#490011} Bug: 747525 Change-Id: Iee153883aff3f7285f6dc54912b4c360acd184a0 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel TBR=danakj@chromium.org TBR=yzshen@chromium.org Change-Id: Iee153883aff3f7285f6dc54912b4c360acd184a0 Reviewed-on: https://chromium-review.googlesource.com/591787 Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#490470} [modify] https://crrev.com/152536c49f55b8be4758892c0f44f336145cca67/cc/ipc/cc_serialization_perftest.cc [modify] https://crrev.com/152536c49f55b8be4758892c0f44f336145cca67/mojo/public/cpp/bindings/lib/serialization.h [modify] https://crrev.com/152536c49f55b8be4758892c0f44f336145cca67/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl
,
Apr 19 2018
FYI we are looking at stop running cc_perftests on bots on crbug.com/784454 .
,
Oct 17
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by thakis@chromium.org
, Jul 21 2017