|
|
Created:
3 years, 9 months ago by sadrul Modified:
3 years, 9 months ago CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, piman+watch_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptiongpu: Replace GpuMsg_CollectGraphicsInfo with mojom API.
This introduces mojom.GpuService.RequestCompleteGpuInfo() API to replace
the GpuMsg_CollectGraphicsInfo chrome ipc message.
BUG=643746
Review-Url: https://codereview.chromium.org/2753293003
Cr-Commit-Position: refs/heads/master@{#459632}
Committed: https://chromium.googlesource.com/chromium/src/+/9e029acf8a371e301aefcb51b4863137adccde68
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : fix win #Patch Set 4 : tot merge #
Total comments: 3
Patch Set 5 : Run callback on UI thread. Tot merge. #Patch Set 6 : . #Patch Set 7 : test #Patch Set 8 : . #Patch Set 9 : debug test failure #Patch Set 10 : debug test failure #Patch Set 11 : . #Patch Set 12 : fix telemetry test #Patch Set 13 : tot merge #Patch Set 14 : fix win build #Patch Set 15 : . #Patch Set 16 : . #Patch Set 17 : . #Patch Set 18 : . #Patch Set 19 : . #Patch Set 20 : . #Patch Set 21 : cleanup #Patch Set 22 : . #Patch Set 23 : more #Patch Set 24 : tot merge #Messages
Total messages: 114 (102 generated)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Description was changed from ========== gpu: Replace GpuMsg_CollectGraphicsInfo with mojom API. ... BUG=... ========== to ========== gpu: Replace GpuMsg_CollectGraphicsInfo with mojom API. This introduces mojom.GpuService.RequestCompleteGpuInfo() API to replace the GpuMsg_CollectGraphicsInfo chrome ipc message. This makes a subtle change in how the response is handled: the response in the browser used to be handled in the UI thread, but with this change, the response is handled in the IO thread. However, the callback for the response notifies the GpuDataManagerImpl instance, which can be accessed from both the IO and UI threads. So this change should not affect the actual behaviour. BUG=643746 ==========
sadrul@chromium.org changed reviewers: + piman@chromium.org, tsepez@chromium.org
tsepez@ for mojom and messages change. piman@ for all of it. Thanks!
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Messages/Mojom LGTM (equivalent functionality).
https://codereview.chromium.org/2753293003/diff/50001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2753293003/diff/50001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:433: GpuDataManagerImpl::GetInstance()->UpdateGpuInfo(gpu_info); Should we hop back to the UI thread, though? I know there's a lock around accesses to GpuDataManagerImplPrivate, but GpuDataManagerImplPrivate::UpdateGpuInfo calls back into things (e.g. GetContentClient()->SetGpuInfo ) which I'm not sure is safe to do from the IO thread.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2753293003/diff/50001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2753293003/diff/50001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:433: GpuDataManagerImpl::GetInstance()->UpdateGpuInfo(gpu_info); On 2017/03/20 21:02:36, piman wrote: > Should we hop back to the UI thread, though? > I know there's a lock around accesses to GpuDataManagerImplPrivate, but > GpuDataManagerImplPrivate::UpdateGpuInfo calls back into things (e.g. > GetContentClient()->SetGpuInfo ) which I'm not sure is safe to do from the IO > thread. Ah, that's a good point! I have moved this to hop onto the UI thread before updating GPUInfo. Would it make sense to add some DCHECKs into GpuDataManager[Impl[Private]] to make sure such functions are called from the right thread (where applicable) to avoid accidental regressions?
https://codereview.chromium.org/2753293003/diff/50001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2753293003/diff/50001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:433: GpuDataManagerImpl::GetInstance()->UpdateGpuInfo(gpu_info); On 2017/03/20 21:21:44, sadrul wrote: > On 2017/03/20 21:02:36, piman wrote: > > Should we hop back to the UI thread, though? > > I know there's a lock around accesses to GpuDataManagerImplPrivate, but > > GpuDataManagerImplPrivate::UpdateGpuInfo calls back into things (e.g. > > GetContentClient()->SetGpuInfo ) which I'm not sure is safe to do from the IO > > thread. > > Ah, that's a good point! I have moved this to hop onto the UI thread before > updating GPUInfo. > > Would it make sense to add some DCHECKs into GpuDataManager[Impl[Private]] to > make sure such functions are called from the right thread (where applicable) to > avoid accidental regressions? I think it would be useful. GpuDataManagerImplPrivate is a bit of a mess and not necessarily well behaved everywhere. It "looks" thread-safe for the most part, but I think it's not in several places (UpdateGpuInfo being one).
lgtm
The CQ bit was checked by sadrul@chromium.org
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/2753293003/#ps70001 (title: "Run callback on UI thread. Tot merge.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== gpu: Replace GpuMsg_CollectGraphicsInfo with mojom API. This introduces mojom.GpuService.RequestCompleteGpuInfo() API to replace the GpuMsg_CollectGraphicsInfo chrome ipc message. This makes a subtle change in how the response is handled: the response in the browser used to be handled in the UI thread, but with this change, the response is handled in the IO thread. However, the callback for the response notifies the GpuDataManagerImpl instance, which can be accessed from both the IO and UI threads. So this change should not affect the actual behaviour. BUG=643746 ========== to ========== gpu: Replace GpuMsg_CollectGraphicsInfo with mojom API. This introduces mojom.GpuService.RequestCompleteGpuInfo() API to replace the GpuMsg_CollectGraphicsInfo chrome ipc message. BUG=643746 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sadrul@chromium.org
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
In case you are wondering what happened to this CL: it's causing some somewhat-flaky test failures (telemetry_unittests) on windows trybots. I am actively investigating (but not having a lot of luck with) the failures.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sadrul@chromium.org
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #21 (id:390001) has been deleted
The flakiness seems to have gone away ... I will put it in the CQ and keep an eye on the subsequent CQ runs if/when this lands.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sadrul@chromium.org
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sadrul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2753293003/#ps460001 (title: "tot merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 460001, "attempt_start_ts": 1490409885886020, "parent_rev": "e04370309e452bfdbad223228b908c7e31f2809c", "commit_rev": "9e029acf8a371e301aefcb51b4863137adccde68"}
Message was sent while issue was closed.
Description was changed from ========== gpu: Replace GpuMsg_CollectGraphicsInfo with mojom API. This introduces mojom.GpuService.RequestCompleteGpuInfo() API to replace the GpuMsg_CollectGraphicsInfo chrome ipc message. BUG=643746 ========== to ========== gpu: Replace GpuMsg_CollectGraphicsInfo with mojom API. This introduces mojom.GpuService.RequestCompleteGpuInfo() API to replace the GpuMsg_CollectGraphicsInfo chrome ipc message. BUG=643746 Review-Url: https://codereview.chromium.org/2753293003 Cr-Commit-Position: refs/heads/master@{#459632} Committed: https://chromium.googlesource.com/chromium/src/+/9e029acf8a371e301aefcb51b486... ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001) as https://chromium.googlesource.com/chromium/src/+/9e029acf8a371e301aefcb51b486... |