New issue
Advanced search Search tips

Issue 726489 link

Starred by 4 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Mojo EDK optimization

Project Member Reported by roc...@chromium.org, May 25 2017

Issue description

Umbrella bug to track profiling and optimization of the EDK.
 

Comment 1 by roc...@chromium.org, May 30 2017

To add a little more detail, there are some hot paths we can optimize (and some we've probably yet to identify), some aggressive synchronization we can reduce, and some redundant processing and bookkeeping overhead we can avoid.

Given the broad usage of Mojo across Chromium now, it should be much easier to measure whether any given optimization has a practical impact on performance.

Comment 2 by roc...@chromium.org, May 30 2017

Cc: horo@chromium.org dmu...@chromium.org roc...@chromium.org rsch...@chromium.org
 Issue 616998  has been merged into this issue.

Comment 3 by roc...@chromium.org, May 30 2017

Issue 627082 has been merged into this issue.

Comment 4 by roc...@chromium.org, May 30 2017

Cc: rmcilroy@chromium.org
 Issue 702677  has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, May 31 2017

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

commit 092ac05d47389f995f1a111072622e5ac69a024f
Author: Ken Rockot <rockot@chromium.org>
Date: Wed May 31 11:15:36 2017

Mojo EDK: Avoid redundant local proxies

Previously any time a UserMessageEvent incurred a hop, any ports it
carried would be duplicated to new ports and set up as proxies to the
new duplicates. While this step is necessary to facilitate cross-node
port transfer, it is completely redundant (and substantial computational
overhead) for messages that are only hopping from one local port to
another.

This introduces a simple optimization whereby messages forwarded from
one local port to another do not setup proxies for carried ports.

A quick test shows that this reduces the total number of ports events
processed on browser startup -- roughly up until the NTP is fully
rendered -- by a factor of ~4.5x. Prior to the patch there are
consistently about 1250 events processed in the browser process alone,
between launch and NTP render. After the patch it's about 280.

BUG= 725321 ,726489

Change-Id: Iddb1503c51137eae8e3a67312f59f9e1a5264d50
Reviewed-on: https://chromium-review.googlesource.com/516469
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475875}
[modify] https://crrev.com/092ac05d47389f995f1a111072622e5ac69a024f/mojo/edk/system/ports/node.cc
[modify] https://crrev.com/092ac05d47389f995f1a111072622e5ac69a024f/mojo/edk/system/ports/ports_unittest.cc

For posterity, and based on a significant regression when the browser-GPU channel was switched to go over Mojo IPC, this may be a good microbenchmark to trace against: https://cs.chromium.org/chromium/src/third_party/WebKit/PerformanceTests/Canvas/upload-webgl-to-texture.html
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 7 2017

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

commit e4b53eb762d71e7dc9ea44fd635635a83dad7b90
Author: Ken Rockot <rockot@chromium.org>
Date: Wed Jun 07 02:09:25 2017

Mojo EDK: Refactor Ports synchronization

Every Port in the EDK has its own thread-safe state guarded by its own
base::Lock. There are a few occasions where it becomes necessary to lock
multiple arbitrary Ports' locks simultaneously.

In order to safeguard such overlapping locks in the past, we've treated
the (effectively global) |ports_lock_| -- which also guards access to
the global Port map -- as a necessary precursor for multi-Port lock
acquisition. While this is sufficient, it needlessly complicates several
common codepaths and imposes quite a bit of unnecessary mutual
exclusion across the system.

This CL eliminates such treatment of |ports_lock_|, leaving the relevant
locking constraints as follows:

  - |ports_lock_| still may not be acquired while holding any individual
    Port's lock.
  - Port locks may be acquired while |ports_lock_| is held; it is
    allowed but not necessary to acquire |ports_lock_| first.
  - Individual Port locks must be acquired in a globally consistent
    order.

In order to make the third constraint feasible, Port locks must be
acquired exclusively using a new PortLocker helper. PortLocker takes
care of locking multiple ports simultaneously in a globally consistent
order and ensures that only one PortLocker exists concurrently on any
single thread.

The more granular locking here enables a few interesting possibilities,
including reentrant event forwarding (and thus further simplification of
EDK code) and safe lazy serialization of user message objects.

BUG=726489, 725321 

Change-Id: Iba1e4cecb3e54f617c54cf070bc4b68fcbb12290
Reviewed-on: https://chromium-review.googlesource.com/526299
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477513}
[modify] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/BUILD.gn
[modify] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/message_queue.cc
[modify] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/message_queue.h
[modify] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/node.cc
[modify] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/node.h
[modify] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/port.h
[add] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/port_locker.cc
[add] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/port_locker.h
[modify] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/port_ref.cc
[modify] https://crrev.com/e4b53eb762d71e7dc9ea44fd635635a83dad7b90/mojo/edk/system/ports/port_ref.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 7 2017

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

commit c06dfa08f681ef8052a1aed514db88a56499a5f5
Author: Ken Rockot <rockot@chromium.org>
Date: Wed Jun 07 03:21:48 2017

Mojo EDK: Ensure that NodeDelegate is never called while holding locks

Removes GenerateRandomPortName from NodeDelegate's interface. The
obvious base/crypto RandBytes impl is now baked into Node.

Adds a helper to ensure that a Node's NodeDelegate is never called into
while holding any internal locks (the Node's |ports_lock_| or any
individual Port locks.)

Includes the minimal refactoring necessary (in OnObserveProxy only) to
ensure that the new NodeDelegate calling constraints are met.

BUG=726489

Change-Id: Ib4fdaf3e8bf853987a0ade72f5c3f1548ef70206
Reviewed-on: https://chromium-review.googlesource.com/526732
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477534}
[modify] https://crrev.com/c06dfa08f681ef8052a1aed514db88a56499a5f5/mojo/edk/system/node_controller.cc
[modify] https://crrev.com/c06dfa08f681ef8052a1aed514db88a56499a5f5/mojo/edk/system/node_controller.h
[modify] https://crrev.com/c06dfa08f681ef8052a1aed514db88a56499a5f5/mojo/edk/system/ports/BUILD.gn
[modify] https://crrev.com/c06dfa08f681ef8052a1aed514db88a56499a5f5/mojo/edk/system/ports/node.cc
[modify] https://crrev.com/c06dfa08f681ef8052a1aed514db88a56499a5f5/mojo/edk/system/ports/node.h
[modify] https://crrev.com/c06dfa08f681ef8052a1aed514db88a56499a5f5/mojo/edk/system/ports/node_delegate.h
[modify] https://crrev.com/c06dfa08f681ef8052a1aed514db88a56499a5f5/mojo/edk/system/ports/ports_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 7 2017

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

commit 35bd9d42270d4474a102f730800c4bfa3b859146
Author: Ken Rockot <rockot@chromium.org>
Date: Wed Jun 07 20:17:49 2017

Mojo EDK: Eliminate deferred event processing

The inability for a Node's NodeDelegate to reenter the Node on
ForwardEvent() necessitated the implementation (e.g. NodeController)
accumulating forwarded events on a transient queue which would then need
to be flushed manually by any codepath that might enqueue new events.

Now that Node supports reentrancy this is no longer necessary and so
this CL changes NodeController::ForwardEvent() to pass local events
directly back into the Node.

BUG=726489

Change-Id: I7667935f992ec83186268eaf52b3261a1300d7cb
Reviewed-on: https://chromium-review.googlesource.com/526494
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477744}
[modify] https://crrev.com/35bd9d42270d4474a102f730800c4bfa3b859146/mojo/edk/system/node_controller.cc
[modify] https://crrev.com/35bd9d42270d4474a102f730800c4bfa3b859146/mojo/edk/system/node_controller.h
[modify] https://crrev.com/35bd9d42270d4474a102f730800c4bfa3b859146/tools/metrics/histograms/histograms.xml

Cc: -rsch...@chromium.org
Owner: rockot@google.com
Cc: -roc...@chromium.org rockot@google.com

Sign in to add a comment