New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 747525 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

Mojo Bindings perf regression from switch to libc++

Project Member Reported by roc...@chromium.org, Jul 21 2017

Issue description

We 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.
 

Comment 1 by thakis@chromium.org, Jul 21 2017

Android (and Mac) uses libc++, so hopefully fixing this should also improve mojo perf on Android. If so , then this would support the "if we make linux use libc++ it'll have perf characteristics more similar to Android, and since most devs are on linux they'll then optimize for Android better" theory which was part of the motivation for the switch.

Comment 2 by yzshen@chromium.org, Jul 21 2017

Cc: fsam...@chromium.org
+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.
Cc: weiliangc@chromium.org
+weiliangc@ who has worked on CCSerializationPerfTest recently.
Cc: enne@chromium.org
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?

Comment 5 by yzshen@chromium.org, 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.

Comment 6 by roc...@chromium.org, 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++

Comment 7 by roc...@chromium.org, Jul 21 2017

Nevermind... misreading symbols. I don't have useful data yet.
I like a cc_serialization_perftests too

Comment 9 by roc...@chromium.org, 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.
Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Project Member

Comment 13 by bugdroid1@chromium.org, 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

Project Member

Comment 14 by bugdroid1@chromium.org, 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

FYI we are looking at stop running cc_perftests on bots on  crbug.com/784454 . 
Owner: rockot@google.com

Sign in to add a comment