gpu: Use mojom API for recording log messages to the host.

gpu: Use mojom API for recording log messages to the host.

This introduces mojom.GpuHost.RecordLogMessage() to replace corresponding
chrome ipc GpuHostMsg_OnLogMessage. Some notable changes:
. The log-handler is set from GpuService instead of from GpuChildThread.
  This is easier since GpuService owns the connect to mojom.GpuHost.
. The message in the host (i.e. the browser) is handled in the IO thread
  rather than the UI thread. The response is to access the thread-safe
  GpuDataManager singleton instance. So there should be no behavioural
  change.

BUG=643746
Review-Url: https://codereview.chromium.org/2763493002
Cr-Commit-Position: refs/heads/master@{#458914}
Committed: https://chromium.googlesource.com/chromium/src/+/6aafa7de713ba655b50e35b9b0d1a2d9f5078333

[sadrul, 2017-03-20 04:57:15]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 04:57:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 04:59:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 04:59:43]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sadrul, 2017-03-20 05:20:11]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 05:20:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-03-20 05:57:34]

Description was changed from

==========
gpu: Use mojom API for recording log messages to the host.

...

BUG=643746
==========

to

==========
gpu: Use mojom API for recording log messages to the host.

This introduces mojom.GpuHost.RecordLogMessage() to replace corresponding
chrome ipc GpuHostMsg_OnLogMessage. Some notable changes:
. The log-handler is set from GpuService instead of from GpuChildThread.
  This is easier since GpuService owns the connect to mojom.GpuHost.
. The message in the host (i.e. the browser) is handled in the IO thread
  rather than the UI thread. The response is to access the thread-safe
  GpuDataManager singleton instance. So there should be no behavioural
  change.

BUG=643746
==========

[commit-bot: I haz the power, 2017-03-20 05:59:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 05:59:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sadrul, 2017-03-20 15:22:44]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 15:23:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-03-20 15:28:22]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 15:28:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-03-20 15:52:16]

sadrul@chromium.org changed reviewers:
+ piman@chromium.org, tsepez@chromium.org

[sadrul, 2017-03-20 15:52:16]

tsepez@ for ipc changes.

piman@ for all of it.

Thanks!

[commit-bot: I haz the power, 2017-03-20 17:13:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 17:13:09]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Tom Sepez, 2017-03-20 17:22:59]

lgtm

[sadrul, 2017-03-20 17:54:03]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 17:54:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 19:05:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 19:05:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[piman, 2017-03-20 21:20:26]

https://codereview.chromium.org/2763493002/diff/80001/content/gpu/gpu_child_t...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/2763493002/diff/80001/content/gpu/gpu_child_t...
content/gpu/gpu_child_thread.cc:282: gpu_host->RecordLogMessage(log.severity,
log.header, log.message);
Just checking, is it ok to do this before we initialize the GpuService (below),
which is what calls GpuHost::DidInitialize?

https://codereview.chromium.org/2763493002/diff/80001/services/ui/gpu/gpu_ser...
File services/ui/gpu/gpu_service.cc (right):

https://codereview.chromium.org/2763493002/diff/80001/services/ui/gpu/gpu_ser...
services/ui/gpu/gpu_service.cc:118: base::ThreadTaskRunnerHandle::Get());
So, this has different properties than the previous code, which used the
ChildThread's thread_safe_sender() which sent things without hopping to the GPU
main thread. In particular this means that if the GPU main thread is blocked, we
won't get log messages - in particular those coming from the GpuWatchdogThread
before it forces a crash. Is there any way to keep that property?

[sadrul, 2017-03-20 21:40:16]

sadrul@chromium.org changed reviewers:
+ rockot@chromium.org

[sadrul, 2017-03-20 21:40:17]

+rockot@ for advice/opinion on thread_safe_sender() vs. use of
ThreadSafeInterfacePtrBase<> (in gpu_service.cc) Thanks!

https://codereview.chromium.org/2763493002/diff/80001/content/gpu/gpu_child_t...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/2763493002/diff/80001/content/gpu/gpu_child_t...
content/gpu/gpu_child_thread.cc:282: gpu_host->RecordLogMessage(log.severity,
log.header, log.message);
On 2017/03/20 21:20:26, piman wrote:
> Just checking, is it ok to do this before we initialize the GpuService
(below),
> which is what calls GpuHost::DidInitialize?

For now, this is OK. GpuProcessHost::DidInitialize() doesn't actually do
anything yet [1], and instead we still send the chrome-ipc for initialization
complete (above, line 278). That's the next cleanup I am going to do (i.e.
remove GpuHostMsg_Initialized in favour of GpuHost::DidInitalize())

[1]
https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_process_host.cc?...

https://codereview.chromium.org/2763493002/diff/80001/services/ui/gpu/gpu_ser...
File services/ui/gpu/gpu_service.cc (right):

https://codereview.chromium.org/2763493002/diff/80001/services/ui/gpu/gpu_ser...
services/ui/gpu/gpu_service.cc:118: base::ThreadTaskRunnerHandle::Get());
On 2017/03/20 21:20:26, piman wrote:
> So, this has different properties than the previous code, which used the
> ChildThread's thread_safe_sender() which sent things without hopping to the
GPU
> main thread. In particular this means that if the GPU main thread is blocked,
we
> won't get log messages - in particular those coming from the GpuWatchdogThread
> before it forces a crash. Is there any way to keep that property?

There is ThreadSafeInterfacePtrBase<> that I could use here instead (so I would
create a ThreadSafeGpuHostPtr out of the mojom::GpuHost object I have), but
looking at the doc [1], it looks like it would just do the thread-hop
internally. So we would still have the same issue.

[1]
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/thread_safe_int...

[sadrul, 2017-03-20 21:54:01]

I just thought of something: the gpu process needs to allocate gpu memory in the
IO thread (as it does now). To allow for that, mojom.GpuHost will move into the
IO thread from the gpu main thread (see discussion in
https://codereview.chromium.org/2701233002/#msg9). When we do that, this will no
longer be a problem I think? If that is the case, then I can move the bind to
happen in the IO thread now before we do this.

[piman, 2017-03-20 22:14:01]

On Mon, Mar 20, 2017 at 2:54 PM, <sadrul@chromium.org> wrote:

> I just thought of something: the gpu process needs to allocate gpu memory
> in the
> IO thread (as it does now). To allow for that, mojom.GpuHost will move
> into the
> IO thread from the gpu main thread (see discussion in
> https://codereview.chromium.org/2701233002/#msg9). When we do that, this
> will no
> longer be a problem I think? If that is the case, then I can move the bind
> to
> happen in the IO thread now before we do this.
>

SG overall (modulo raciness between threads wrt globals such as the log
handler), but sending from the IO thread seems like it would be equivalent
to the previous logic.


>
> https://codereview.chromium.org/2763493002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-03-21 00:56:01]

https://codereview.chromium.org/2763493002/diff/80001/services/ui/gpu/gpu_ser...
File services/ui/gpu/gpu_service.cc (right):

https://codereview.chromium.org/2763493002/diff/80001/services/ui/gpu/gpu_ser...
services/ui/gpu/gpu_service.cc:118: base::ThreadTaskRunnerHandle::Get());
On 2017/03/20 at 21:40:17, sadrul wrote:
> On 2017/03/20 21:20:26, piman wrote:
> > So, this has different properties than the previous code, which used the
> > ChildThread's thread_safe_sender() which sent things without hopping to the
GPU
> > main thread. In particular this means that if the GPU main thread is
blocked, we
> > won't get log messages - in particular those coming from the
GpuWatchdogThread
> > before it forces a crash. Is there any way to keep that property?
> 
> There is ThreadSafeInterfacePtrBase<> that I could use here instead (so I
would create a ThreadSafeGpuHostPtr out of the mojom::GpuHost object I have),
but looking at the doc [1], it looks like it would just do the thread-hop
internally. So we would still have the same issue.
> 
> [1]
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/thread_safe_int...

Well one option would be to move the GpuHostPtr to the IO thread and use a
ThreadSafeGpuHostPtr everywhere else. That would be a bummer.

I think we should probably change TSIP to do direct writes from arbitrary
threads.

That does mean existing users of TSIP would need to be updated to use some other
special-case thing, as the existing TSIP was designed to emulate
ThreadSafeSender semantics and thus requires that we PostTask to some target
TaskRunner (typically the IO thread) to preserve ordering guarantees.

[sadrul, 2017-03-22 12:12:06]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 12:12:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 13:37:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 13:37:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2017-03-22 13:54:46]

Moved the binding to live on the IO thread
(https://codereview.chromium.org/2764753004/). This should be ready for review
now. PTAL. Thanks!

[piman, 2017-03-22 21:09:42]

lgtm

[sadrul, 2017-03-22 22:44:20]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2017-03-22 22:44:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2763493002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2017-03-22 22:45:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 22:56:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490222660439400,
"parent_rev": "ce045d0704eca970da082c10b816d67d35faac4e", "commit_rev":
"6aafa7de713ba655b50e35b9b0d1a2d9f5078333"}

[commit-bot: I haz the power, 2017-03-22 22:57:31]

Description was changed from

==========
gpu: Use mojom API for recording log messages to the host.

This introduces mojom.GpuHost.RecordLogMessage() to replace corresponding
chrome ipc GpuHostMsg_OnLogMessage. Some notable changes:
. The log-handler is set from GpuService instead of from GpuChildThread.
  This is easier since GpuService owns the connect to mojom.GpuHost.
. The message in the host (i.e. the browser) is handled in the IO thread
  rather than the UI thread. The response is to access the thread-safe
  GpuDataManager singleton instance. So there should be no behavioural
  change.

BUG=643746
==========

to

==========
gpu: Use mojom API for recording log messages to the host.

This introduces mojom.GpuHost.RecordLogMessage() to replace corresponding
chrome ipc GpuHostMsg_OnLogMessage. Some notable changes:
. The log-handler is set from GpuService instead of from GpuChildThread.
  This is easier since GpuService owns the connect to mojom.GpuHost.
. The message in the host (i.e. the browser) is handled in the IO thread
  rather than the UI thread. The response is to access the thread-safe
  GpuDataManager singleton instance. So there should be no behavioural
  change.

BUG=643746

Review-Url: https://codereview.chromium.org/2763493002
Cr-Commit-Position: refs/heads/master@{#458914}
Committed:
https://chromium.googlesource.com/chromium/src/+/6aafa7de713ba655b50e35b9b0d1...
==========

[commit-bot: I haz the power, 2017-03-22 22:57:35]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6aafa7de713ba655b50e35b9b0d1...