New issue
Advanced search Search tips

Issue 866708 link

Starred by 4 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug


Sign in to add a comment

Mojo C++ Bindings: Individual messages should each dispatch through a dedicated task

Project Member Reported by roc...@chromium.org, Jul 23

Issue description

Currently the C++ bindings schedule a single wake-up task when a pipe becomes readable and no such task is already pending. The task drains the pipe completely, dispatching all available messages in series before the task completes.

This reduces scheduling granularity and is therefore generally undesirable. We should instead ensure that every message is dispatched within the extent of a dedicated task execution.

 
Blockedon: 866616
Blockedon: 869547
Blockedon: 869549
Blockedon: 869550
Blockedon: 869551
Blockedon: 869553
Cc: altimin@chromium.org
+cc altimin just FYI in case you want to track this. Lots of things blocking the change from landing, will be working through them.
Linking fix CL here for reference, as there are still several more test failures that need to be addressed: https://chromium-review.googlesource.com/c/chromium/src/+/1145692
Blockedon: 872063
Blockedon: 872065
Blockedon: 872068
Blockedon: 872069
Blockedon: 872070
Blocking: 872071
Blockedon: 872074
Blockedon: 872075
Blockedon: 872076
Blockedon: 872078
Blockedon: 872081
Blockedon: 895693
Owner: rockot@google.com
Blockedon: 896159
Blockedon: 896162
Blockedon: -872076
Blockedon: 872076
Blockedon: 916177
Project Member

Comment 27 by bugdroid1@chromium.org, Jan 16 (6 days ago)

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

commit 550d5a768e5efdb3f81d472f42dfd59bb5b83b1d
Author: Ken Rockot <rockot@google.com>
Date: Wed Jan 16 19:04:20 2019

Fix or disable tests with bad timing expectations

This disables a few tests which fail when attempting to land
https://chromium-review.googlesource.com/c/chromium/src/+/1145692.
That CL ends up changing Mojo bindings dispatch timing in subtle
but valid ways, and as such it should not actually break any correct
tests.

These tests should be fixed and re-enabled ASAP after the Mojo change is
landed.

Bug: 866708,917113
Change-Id: Ib97dbe312562adc0e02eb61c323386836bcb44f5
Reviewed-on: https://chromium-review.googlesource.com/c/1388172
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Dmitry Titov <dimich@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623318}
[modify] https://crrev.com/550d5a768e5efdb3f81d472f42dfd59bb5b83b1d/ash/highlighter/highlighter_controller_unittest.cc
[modify] https://crrev.com/550d5a768e5efdb3f81d472f42dfd59bb5b83b1d/ash/magnifier/magnification_controller_unittest.cc
[modify] https://crrev.com/550d5a768e5efdb3f81d472f42dfd59bb5b83b1d/chrome/browser/offline_pages/offline_page_request_handler_unittest.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Jan 17 (6 days ago)

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

commit 471aa794bbb9e3a98542ee2135ea61ddf4bf87a3
Author: Ken Rockot <rockot@google.com>
Date: Thu Jan 17 02:46:59 2019

[mojo] Per-message dispatch tasks

Modifies C++ bindings internals such that incoming messages, while
still read off the pipe in batch, are each dispatched with
individual tasks rather than dispatching them all in series within
the same task.

The first message in a batch is still dispatched synchronously, and
exceptions are made during sync wait operations where messages must
be (at least partially) dispatched so that the waiting endpoint can
get at its anticipated reply without allowing its calling sequence
to progress.

Also just for fun, deletes Connector::SyncWatch because it hasn't
been used in, like, forever.

TBR=jam@chromium.org

Bug: 866708
Change-Id: If56d2341c0642a0fd99a90cb60a2dfc5573e0b4e
Reviewed-on: https://chromium-review.googlesource.com/c/1145692
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623535}
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/content/common/throttling_url_loader.cc
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/ipc/ipc_mojo_bootstrap.cc
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/mojo/public/cpp/bindings/binding.h
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/mojo/public/cpp/bindings/connector.h
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/mojo/public/cpp/bindings/lib/binding_state.cc
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/mojo/public/cpp/bindings/lib/binding_state.h
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/mojo/public/cpp/bindings/lib/connector.cc
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/mojo/public/cpp/bindings/lib/multiplex_router.cc
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/mojo/public/cpp/bindings/lib/multiplex_router.h
[modify] https://crrev.com/471aa794bbb9e3a98542ee2135ea61ddf4bf87a3/third_party/blink/web_tests/TestExpectations

Project Member

Comment 29 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 492f7c2f6f96f59f8aab79317faeeaa5136de83c
Author: Ken Rockot <rockot@google.com>
Date: Fri Jan 18 20:09:26 2019

Temporarily revert Mojo back to batch dispatch

This is to avoid any surprises in M73 since we're so close to branch.

Bug: 866708, 923437
Change-Id: Ia063bd130f9124e598651c9a385ac6871cfbe729
Reviewed-on: https://chromium-review.googlesource.com/c/1422663
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#624251}
[modify] https://crrev.com/492f7c2f6f96f59f8aab79317faeeaa5136de83c/mojo/public/cpp/bindings/connector.h

Sign in to add a comment