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

Issue 594244 link

Starred by 1 user
Status: Fixed
Owner:
█ yzshen@chromium.org
Last visit > 30 days ago
Closed: Nov 2016
Cc:
█ roc...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Mojo C++ bindings: consider unifying Router and MultipleRouter.

Project Member Reported by yzshen@chromium.org, Mar 11 2016 
Currently Router is used for InterfacePtr/Binding which don't pass associated interface endpoints. It doesn't need lock and endpoint bookkeeping so it is expected to be more efficient. However, with features such as sync method calls, Router becomes more complex. It is quite some work to maintain two different router implementation and ensure they are consistent.

It would be nice to measure/optimize MultiplexRouter for the non-associated case, and remove Router.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 6 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8080848e3e4694cfb6fb84cd842b0b526b60263c

commit 8080848e3e4694cfb6fb84cd842b0b526b60263c
Author: yzshen <yzshen@chromium.org>
Date: Tue Sep 06 23:15:38 2016

Mojo C++ bindings: add perf tests to compare performance of Router/MultiplexRouter.

These two test cases do same-thread ping-pong using Router and MultiplexRouter:

MultiplexRouter is about 95% as fast as Router:

[ RUN      ] MojoBindingsPerftest.RouterPingPong
RouterPingPong/ 144546  pings/second
[       OK ] MojoBindingsPerftest.RouterPingPong (702 ms)
[ RUN      ] MojoBindingsPerftest.MultiplexRouterPingPong
MultiplexRouterPingPong/  137701  pings/second

BUG= 594244 

Review-Url: https://codereview.chromium.org/2314763003
Cr-Commit-Position: refs/heads/master@{#416761}

[modify] https://crrev.com/8080848e3e4694cfb6fb84cd842b0b526b60263c/mojo/public/cpp/bindings/tests/bindings_perftest.cc

Comment 2 by yzshen@chromium.org, Sep 13 2016 

Found that on windows, the difference is bigger:
[ RUN      ] MojoBindingsPerftest.RouterPingPong
RouterPingPong  275579  pings/second
[       OK ] MojoBindingsPerftest.RouterPingPong (367 ms)
[ RUN      ] MojoBindingsPerftest.MultiplexRouterPingPong
MultiplexRouterPingPong 191822  pings/second
[       OK ] MojoBindingsPerftest.MultiplexRouterPingPong (528 ms)

MultiplexRouter is about 70% as fast as Router.

I will profile and see whether I can make some improvement.

Comment 3 by yzshen@chromium.org, Sep 13 2016 

Tried:
1) removing the lock (in both MultiplexRouter and Connector);
2) using base::SmallMap instead of std::map;
3) removing the endpoints_ map with a single endpoint (the master one).

But all these didn't make any significant difference:
(With #1 and #3 changes)
[ RUN      ] MojoBindingsPerftest.RouterPingPong
RouterPingPong  270813  pings/second
[       OK ] MojoBindingsPerftest.RouterPingPong (11083 ms)
[ RUN      ] MojoBindingsPerftest.MultiplexRouterPingPong
MultiplexRouterPingPong 194605  pings/second
[       OK ] MojoBindingsPerftest.MultiplexRouterPingPong (15422 ms)
[----------] 2 tests from MojoBindingsPerftest (26512 ms total)

Profiling on windows didn't show me much useful information either.

Will need to take a closer look.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/649f34f7c6195cdb40c5c451208baed00dd05084

commit 649f34f7c6195cdb40c5c451208baed00dd05084
Author: yzshen <yzshen@chromium.org>
Date: Thu Sep 15 22:10:23 2016

Mojo C++ bindings: add more perf tests for Router/MultiplexRouter.

These new tests measure performance of dispatching incoming messages on the same
thread.

Currently on Linux:
RouterDispatchCost  3.93483e+06 times/second
MultiplexRouterDispatchCost 3.39754e+06 times/second

BUG= 594244 

Review-Url: https://codereview.chromium.org/2340363002
Cr-Commit-Position: refs/heads/master@{#419007}

[modify] https://crrev.com/649f34f7c6195cdb40c5c451208baed00dd05084/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
[modify] https://crrev.com/649f34f7c6195cdb40c5c451208baed00dd05084/mojo/public/cpp/bindings/lib/multiplex_router.h
[modify] https://crrev.com/649f34f7c6195cdb40c5c451208baed00dd05084/mojo/public/cpp/bindings/lib/router.h
[modify] https://crrev.com/649f34f7c6195cdb40c5c451208baed00dd05084/mojo/public/cpp/bindings/tests/bindings_perftest.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/649f34f7c6195cdb40c5c451208baed00dd05084

commit 649f34f7c6195cdb40c5c451208baed00dd05084
Author: yzshen <yzshen@chromium.org>
Date: Thu Sep 15 22:10:23 2016

Mojo C++ bindings: add more perf tests for Router/MultiplexRouter.

These new tests measure performance of dispatching incoming messages on the same
thread.

Currently on Linux:
RouterDispatchCost  3.93483e+06 times/second
MultiplexRouterDispatchCost 3.39754e+06 times/second

BUG= 594244 

Review-Url: https://codereview.chromium.org/2340363002
Cr-Commit-Position: refs/heads/master@{#419007}

[modify] https://crrev.com/649f34f7c6195cdb40c5c451208baed00dd05084/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
[modify] https://crrev.com/649f34f7c6195cdb40c5c451208baed00dd05084/mojo/public/cpp/bindings/lib/multiplex_router.h
[modify] https://crrev.com/649f34f7c6195cdb40c5c451208baed00dd05084/mojo/public/cpp/bindings/lib/router.h
[modify] https://crrev.com/649f34f7c6195cdb40c5c451208baed00dd05084/mojo/public/cpp/bindings/tests/bindings_perftest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/053dc1466340b8206ed42e1909e49d17712e51ee

commit 053dc1466340b8206ed42e1909e49d17712e51ee
Author: yzshen <yzshen@chromium.org>
Date: Thu Sep 15 23:30:31 2016

Mojo C++ bindings: remove the lock in MultiplexRouter if it only serves a single interface.

BUG= 594244 

Review-Url: https://codereview.chromium.org/2345013002
Cr-Commit-Position: refs/heads/master@{#419036}

[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/BUILD.gn
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/associated_interface_ptr.h
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/lib/binding_state.cc
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/lib/connector.cc
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/lib/interface_ptr_state.h
[add] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/lib/may_auto_lock.h
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/lib/multiplex_router.cc
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/lib/multiplex_router.h
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/tests/associated_interface_unittest.cc
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/tests/bindings_perftest.cc
[modify] https://crrev.com/053dc1466340b8206ed42e1909e49d17712e51ee/mojo/public/cpp/bindings/tests/multiplex_router_unittest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 20 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65085d5fe6a009f01d7630c2905f19e32104ef7c

commit 65085d5fe6a009f01d7630c2905f19e32104ef7c
Author: yzshen <yzshen@chromium.org>
Date: Tue Sep 20 01:25:04 2016

Mojo C++ bindings: fix Connector teardown.

It is allowed to disconnect a Connector instance (by CloseMessagePipe() or
PassMessagePipe()) on the creation thread, and then destroy the instance on any
thread. However, without this fix it doesn't work because a Connector instance
still has got WeakPtr and mojo::Watcher bound to the creation thread.

BUG= 594244 

Review-Url: https://codereview.chromium.org/2350883003
Cr-Commit-Position: refs/heads/master@{#419626}

[modify] https://crrev.com/65085d5fe6a009f01d7630c2905f19e32104ef7c/mojo/public/cpp/bindings/connector.h
[modify] https://crrev.com/65085d5fe6a009f01d7630c2905f19e32104ef7c/mojo/public/cpp/bindings/lib/connector.cc
[modify] https://crrev.com/65085d5fe6a009f01d7630c2905f19e32104ef7c/mojo/public/cpp/bindings/tests/connector_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 21 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/421e2d6ed8e31318f8e8e653d2d892d9b58815df

commit 421e2d6ed8e31318f8e8e653d2d892d9b58815df
Author: yzshen <yzshen@chromium.org>
Date: Wed Sep 21 15:37:59 2016

Mojo C++ bindings: make InterfaceEndpointClient observe message loop destruction.

Otherwise, there is no guarantee that we will issue connection error
notification on message loop destruction.

The observer of mojo::Watcher will need to be removed.

BUG= 594244 ,  604762 

Review-Url: https://codereview.chromium.org/2358693002
Cr-Commit-Position: refs/heads/master@{#420069}

[modify] https://crrev.com/421e2d6ed8e31318f8e8e653d2d892d9b58815df/mojo/public/cpp/bindings/interface_endpoint_client.h
[modify] https://crrev.com/421e2d6ed8e31318f8e8e653d2d892d9b58815df/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 21 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f84285faf99679494e9fdb743ffc03f3959b1a3

commit 4f84285faf99679494e9fdb743ffc03f3959b1a3
Author: yzshen <yzshen@chromium.org>
Date: Wed Sep 21 16:46:59 2016

Mojo C++ bindings: always use MultiplexRouter with InterfacePtr/Binding.

This CL doesn't do the cleanup of removing Router and its usage, so that it is
pretty small and easy to revert if necessary. The cleanup will be in future CLs.

BUG= 594244 

Review-Url: https://codereview.chromium.org/2349293002
Cr-Commit-Position: refs/heads/master@{#420081}

[modify] https://crrev.com/4f84285faf99679494e9fdb743ffc03f3959b1a3/mojo/public/cpp/bindings/binding.h
[modify] https://crrev.com/4f84285faf99679494e9fdb743ffc03f3959b1a3/mojo/public/cpp/bindings/interface_ptr.h
[modify] https://crrev.com/4f84285faf99679494e9fdb743ffc03f3959b1a3/mojo/public/cpp/bindings/lib/binding_state.cc
[modify] https://crrev.com/4f84285faf99679494e9fdb743ffc03f3959b1a3/mojo/public/cpp/bindings/lib/binding_state.h
[modify] https://crrev.com/4f84285faf99679494e9fdb743ffc03f3959b1a3/mojo/public/cpp/bindings/lib/interface_ptr_state.h
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 21 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ac99ca8b3fd4058dce64431dd407ab72ec5c7674

commit ac99ca8b3fd4058dce64431dd407ab72ec5c7674
Author: yzshen <yzshen@chromium.org>
Date: Wed Sep 21 18:21:14 2016

Revert of Mojo C++ bindings: always use MultiplexRouter with InterfacePtr/Binding. (patchset #3 id:40001 of https://codereview.chromium.org/2349293002/ )

Reason for revert:
It probably caused browser_tests and unit_tests failures on https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/30483

Original issue's description:
> Mojo C++ bindings: always use MultiplexRouter with InterfacePtr/Binding.
>
> This CL doesn't do the cleanup of removing Router and its usage, so that it is
> pretty small and easy to revert if necessary. The cleanup will be in future CLs.
>
> BUG= 594244 
>
> Committed: https://crrev.com/4f84285faf99679494e9fdb743ffc03f3959b1a3
> Cr-Commit-Position: refs/heads/master@{#420081}

TBR=rockot@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 594244 

Review-Url: https://codereview.chromium.org/2356323002
Cr-Commit-Position: refs/heads/master@{#420109}

[modify] https://crrev.com/ac99ca8b3fd4058dce64431dd407ab72ec5c7674/mojo/public/cpp/bindings/binding.h
[modify] https://crrev.com/ac99ca8b3fd4058dce64431dd407ab72ec5c7674/mojo/public/cpp/bindings/interface_ptr.h
[modify] https://crrev.com/ac99ca8b3fd4058dce64431dd407ab72ec5c7674/mojo/public/cpp/bindings/lib/binding_state.cc
[modify] https://crrev.com/ac99ca8b3fd4058dce64431dd407ab72ec5c7674/mojo/public/cpp/bindings/lib/binding_state.h
[modify] https://crrev.com/ac99ca8b3fd4058dce64431dd407ab72ec5c7674/mojo/public/cpp/bindings/lib/interface_ptr_state.h
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 30 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a87f4ac9dd870976bbcaab0f416322a7ea60910a

commit a87f4ac9dd870976bbcaab0f416322a7ea60910a
Author: yzshen <yzshen@chromium.org>
Date: Wed Nov 30 19:03:20 2016

Mojo C++ bindings: remove the single-threaded Router.

BUG= 594244 

Review-Url: https://codereview.chromium.org/2532053004
Cr-Commit-Position: refs/heads/master@{#435363}

[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/BUILD.gn
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/binding.h
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/interface_ptr.h
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/lib/binding_state.cc
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/lib/binding_state.h
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/lib/interface_ptr_state.h
[delete] https://crrev.com/6eada0329b5e51ab20254aa20c37fbee6288dff1/mojo/public/cpp/bindings/lib/router.cc
[delete] https://crrev.com/6eada0329b5e51ab20254aa20c37fbee6288dff1/mojo/public/cpp/bindings/lib/router.h
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/strong_binding.h
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/tests/BUILD.gn
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/tests/bindings_perftest.cc
[delete] https://crrev.com/6eada0329b5e51ab20254aa20c37fbee6288dff1/mojo/public/cpp/bindings/tests/router_unittest.cc
[modify] https://crrev.com/a87f4ac9dd870976bbcaab0f416322a7ea60910a/mojo/public/cpp/bindings/tests/validation_unittest.cc

Comment 13 by yzshen@chromium.org, Nov 30 2016

Status: Fixed (was: Untriaged)
►

Sign in to add a comment