Perfhost for ubuntu trusty

Perfhost for ubuntu trusty

Committed: https://crrev.com/7bddaa0d8a4768d9b3d9fa5fbd9e0a56a99b5303
Cr-Commit-Position: refs/heads/master@{#292930}

[pasko, 2014-08-15 13:50:31]

PTaL

I already uploaded the binaries (perfhost_lucid and perfhost_trusty) to the
cloud storage, hence it is possible to try this CL by cherry-picking locally.

I did not remove the old perfhost.sha1 in case there are some dependencies I
don't know about, which is easy with Python :) I'm going to remove it in a
followup CL.

I'm also not sure about the layering rules in telemetry code, so I've put
GetPerfhostName() to android_profiling_helper, and calling it from within the
linux platform backend looks awkward. Feel free to suggest a better place.

[Sami, 2014-08-15 14:08:55]

Thanks Egor. Would you mind updating tools/profile_chrome/perf_controller.py too
while you're at it?

https://codereview.chromium.org/474933005/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/linux_platform_backend.py (right):

https://codereview.chromium.org/474933005/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:70: if
application == 'perfhost':
It feels like we should keep the trusty/lucid logic out of this class and
instead just have the clients pass in the right name, WDYT? See for example how
webdriver_desktop_browser_finder.py deals with 32/64 bit.

https://codereview.chromium.org/474933005/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/474933005/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:106: except:
Is there are more specific exception we could catch?

[pasko, 2014-08-18 14:47:47]

PTAL

> Would you mind updating tools/profile_chrome/perf_controller.py too
while you're at it?

I have changes to introduce GetPerfhostName() to perf_controller.py in this very
CL, did you mean some other updates to perf_controller?

https://codereview.chromium.org/474933005/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/linux_platform_backend.py (right):

https://codereview.chromium.org/474933005/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:70: if
application == 'perfhost':
On 2014/08/15 14:08:55, Sami wrote:
> It feels like we should keep the trusty/lucid logic out of this class and
> instead just have the clients pass in the right name, WDYT? See for example
how
> webdriver_desktop_browser_finder.py deals with 32/64 bit.

I was thinking about having "perfhost_lucid" and "perfhost_trusty" as different
Applications (for CanLaunchApplication(), LaunchApplication(),
InstallApplication(). I did not find a good way to keep this boolean around and
reuse the GetHostPlatform().GetOSVersionName() functionality. Do you have
something specific in mind?

This class already has some knowledge about interactions between installation
procedures and the OS (for ipfw and avconv), adding more knowledge does not make
it too much worse.

The abstraction is leaky though, we both encourage to LaunchApplication(by name)
and run it using PATH with subprocess.*. Using only LaunchApplication would be
cleaner, I guess.

https://codereview.chromium.org/474933005/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/474933005/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:106: except:
On 2014/08/15 14:08:55, Sami wrote:
> Is there are more specific exception we could catch?

Done. It would be device_errors.CommandFailedError.

[Sami, 2014-08-18 14:58:00]

On 2014/08/18 14:47:47, pasko wrote:
> PTAL
> 
> > Would you mind updating tools/profile_chrome/perf_controller.py too
> while you're at it?
> 
> I have changes to introduce GetPerfhostName() to perf_controller.py in this
very
> CL, did you mean some other updates to perf_controller?

Gah, I just need to learn to read :) Thanks!
 
> I was thinking about having "perfhost_lucid" and "perfhost_trusty" as
different
> Applications (for CanLaunchApplication(), LaunchApplication(),
> InstallApplication(). I did not find a good way to keep this boolean around
and
> reuse the GetHostPlatform().GetOSVersionName() functionality. Do you have
> something specific in mind?
> 
> This class already has some knowledge about interactions between installation
> procedures and the OS (for ipfw and avconv), adding more knowledge does not
make
> it too much worse.
> 
> The abstraction is leaky though, we both encourage to LaunchApplication(by
name)
> and run it using PATH with subprocess.*. Using only LaunchApplication would be
> cleaner, I guess.

Could we just always pass in the result from
android_profiling_helper.GetPerfhostName() to CanLaunchApplication and
InstallApplication? Or are there some call sites where that is difficult?

[pasko, 2014-08-18 15:34:01]

On 2014/08/18 14:58:00, Sami wrote:
> Could we just always pass in the result from
> android_profiling_helper.GetPerfhostName() to CanLaunchApplication and
> InstallApplication? Or are there some call sites where that is difficult?

That's my immediate thought too .. but then it would require adding
"perfhost_lucid" and "perfhost_trusty" to _HOST_APPLICATIONS in
android_platform_backend.py, which leaks the implementation details of
GetPerfhostName(). Ugly.

[Sami, 2014-08-18 15:49:13]

On 2014/08/18 15:34:01, pasko wrote:
> On 2014/08/18 14:58:00, Sami wrote:
> > Could we just always pass in the result from
> > android_profiling_helper.GetPerfhostName() to CanLaunchApplication and
> > InstallApplication? Or are there some call sites where that is difficult?
> 
> That's my immediate thought too .. but then it would require adding
> "perfhost_lucid" and "perfhost_trusty" to _HOST_APPLICATIONS in
> android_platform_backend.py, which leaks the implementation details of
> GetPerfhostName(). Ugly.

Right...I still kinda like that option better as opposed to adding a dependency
from linux_platform_backend to android_profiling_helper.

I guess alternatively we could add the right perfhost to _HOST_APPLICATION using
android_profiling_helper?

[pasko, 2014-08-18 17:21:01]

On 2014/08/18 15:49:13, Sami wrote:
> On 2014/08/18 15:34:01, pasko wrote:
> > On 2014/08/18 14:58:00, Sami wrote:
> > > Could we just always pass in the result from
> > > android_profiling_helper.GetPerfhostName() to CanLaunchApplication and
> > > InstallApplication? Or are there some call sites where that is difficult?
> > 
> > That's my immediate thought too .. but then it would require adding
> > "perfhost_lucid" and "perfhost_trusty" to _HOST_APPLICATIONS in
> > android_platform_backend.py, which leaks the implementation details of
> > GetPerfhostName(). Ugly.
> 
> Right...I still kinda like that option better as opposed to adding a
dependency
> from linux_platform_backend to android_profiling_helper.

I am not sure I understand the ideology of android_profiling_helper, so I don't
have much preference.

> I guess alternatively we could add the right perfhost to _HOST_APPLICATION
using
> android_profiling_helper?

I don't really understand how to do it without breaking more layering. Is it
something like adding a method like AddApplication() to telemetry.core.platform?

[Sami, 2014-08-18 17:27:02]

On 2014/08/18 17:21:01, pasko wrote:
> On 2014/08/18 15:49:13, Sami wrote:
> > On 2014/08/18 15:34:01, pasko wrote:
> > > On 2014/08/18 14:58:00, Sami wrote:
> > > > Could we just always pass in the result from
> > > > android_profiling_helper.GetPerfhostName() to CanLaunchApplication and
> > > > InstallApplication? Or are there some call sites where that is
difficult?
> > > 
> > > That's my immediate thought too .. but then it would require adding
> > > "perfhost_lucid" and "perfhost_trusty" to _HOST_APPLICATIONS in
> > > android_platform_backend.py, which leaks the implementation details of
> > > GetPerfhostName(). Ugly.
> > 
> > Right...I still kinda like that option better as opposed to adding a
> dependency
> > from linux_platform_backend to android_profiling_helper.
> 
> I am not sure I understand the ideology of android_profiling_helper, so I
don't
> have much preference.
> 
> > I guess alternatively we could add the right perfhost to _HOST_APPLICATION
> using
> > android_profiling_helper?
> 
> I don't really understand how to do it without breaking more layering. Is it
> something like adding a method like AddApplication() to
telemetry.core.platform?

I was thinking something like this (in android_platform_backend.py):

from telemetry.core.platform.profiler import android_profiling_helper

_HOST_APPLICATIONS = [
    'avconv',
    'ipfw',
    android_profiling_helper.GetPerfhostName()
    ]

Or did I miss some dependency here?

[vmiura, 2014-08-26 17:04:56]

Which version of Perf is this based on?

For the perf_to_tracing.py script to work, we need this change:

https://github.com/vmiura/linux/commit/e1fe871e4a33712ad4964a70904d5d59188e3cc2

[chromium-reviews, 2014-08-26 17:08:30]

I believe that's the patch we merged into this binary -- right Egor? I
think Egor had to adapt it to make it apply cleanly to a newer version of
perf.

- Sami


2014-08-26 18:04 GMT+01:00 <vmiura@chromium.org>:

> Which version of Perf is this based on?
>
> For the perf_to_tracing.py script to work, we need this change:
>
> https://github.com/vmiura/linux/commit/e1fe871e4a33712ad4964a70904d5d
> 59188e3cc2
>
>
> https://codereview.chromium.org/474933005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pasko, 2014-08-28 13:04:41]

Sorry for not finishing it before the vacation. Now I am back.

On 2014/08/26 17:08:30, chromium-reviews wrote:
> I believe that's the patch we merged into this binary -- right Egor? I
> think Egor had to adapt it to make it apply cleanly to a newer version of
> perf.

Unfortunately no, this is based on the vanilla v3.13 kernel as described in the
README.chromium, and does *not* include the patches from vmiura@. I tried to
rebase all 4 of them mechanically, but then actually I need to know more about
how it works to make a proper rebase. This patch only supports the basic
--profiler=perf usecase in telemetry, advanced perf_to_tracing.py is for later
:/

This should be better than nothing.

Victor, can you rebase or make a short writeup of what the changes are doing? :)

[pasko, 2014-08-28 15:47:42]

PTAL

On 2014/08/18 17:27:02, Sami wrote:
> On 2014/08/18 17:21:01, pasko wrote:
> > On 2014/08/18 15:49:13, Sami wrote:
> > > On 2014/08/18 15:34:01, pasko wrote:
> > > > On 2014/08/18 14:58:00, Sami wrote:
> > > > > Could we just always pass in the result from
> > > > > android_profiling_helper.GetPerfhostName() to CanLaunchApplication and
> > > > > InstallApplication? Or are there some call sites where that is
> difficult?
> > > > 
> > > > That's my immediate thought too .. but then it would require adding
> > > > "perfhost_lucid" and "perfhost_trusty" to _HOST_APPLICATIONS in
> > > > android_platform_backend.py, which leaks the implementation details of
> > > > GetPerfhostName(). Ugly.
> > > 
> > > Right...I still kinda like that option better as opposed to adding a
> > dependency
> > > from linux_platform_backend to android_profiling_helper.
> > 
> > I am not sure I understand the ideology of android_profiling_helper, so I
> don't
> > have much preference.
> > 
> > > I guess alternatively we could add the right perfhost to _HOST_APPLICATION
> > using
> > > android_profiling_helper?
> > 
> > I don't really understand how to do it without breaking more layering. Is it
> > something like adding a method like AddApplication() to
> telemetry.core.platform?
> 
> I was thinking something like this (in android_platform_backend.py):
> 
> from telemetry.core.platform.profiler import android_profiling_helper
> 
> _HOST_APPLICATIONS = [
>     'avconv',
>     'ipfw',
>     android_profiling_helper.GetPerfhostName()
>     ]
> 
> Or did I miss some dependency here?

Oh yes, I am too used to treat these global-allcaps as constants, but in python
we can actually put a function call there... Done.

There is still the problem remaining that the knowledge about perfhost names
should be available for LinuxPlatformBackend if it wants to output an error
message. There seems to be no good place to encapsulate this knowledge in the
common dependencies of android_profiling_helper and linux_platform_backend. So
I'm introducing _POSSIBLE_PERFHOST_APPLICATIONS = ['perfhost_lucid',
'perfhost_trusty']. The good thing is that perfhost_trusty is git-greppable
after the change.

[Sami, 2014-08-28 16:36:49]

Thanks Egor, the installation mechanism looks pretty good now. Just one question
about the error message.

https://codereview.chromium.org/474933005/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_platform_backend.py (right):

https://codereview.chromium.org/474933005/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:84: message =
('You may proceed with making a symlink to /usr/bin/perf from '
Is this a fallback for when the installation breaks with trusty+1? If so, would
it be better to fail loudly instead with this hard to understand error message?
Using system perf instead of our patched one will break some use cases in very
non-obvious ways.

[pasko, 2014-08-28 17:47:32]

https://codereview.chromium.org/474933005/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_platform_backend.py (right):

https://codereview.chromium.org/474933005/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:84: message =
('You may proceed with making a symlink to /usr/bin/perf from '
On 2014/08/28 16:36:49, Sami wrote:
> Is this a fallback for when the installation breaks with trusty+1?

I guess this will probably be the main case of this being printed. Also, some
people use non-ubuntu-based distributions, they'd observe this message. There
would be other rare cases like the executable bit was messed up on the file, or
the symlink that was hackishly made earlier does not point at the right place.

> If so, would it be better to fail loudly instead with this hard to understand
error message?
> Using system perf instead of our patched one will break some use cases in very
> non-obvious ways.

By 'loudly' do you mean to print a stack trace and immediately stop? It's
probably a good idea. Would you like to make the message shorter without
mentioning the symlinking?

[Sami, 2014-08-28 17:57:50]

On 2014/08/28 17:47:32, pasko wrote:
>
https://codereview.chromium.org/474933005/diff/60001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/platform/linux_platform_backend.py
(right):
> 
>
https://codereview.chromium.org/474933005/diff/60001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/platform/linux_platform_backend.py:84: message
=
> ('You may proceed with making a symlink to /usr/bin/perf from '
> On 2014/08/28 16:36:49, Sami wrote:
> > Is this a fallback for when the installation breaks with trusty+1?
> 
> I guess this will probably be the main case of this being printed. Also, some
> people use non-ubuntu-based distributions, they'd observe this message. There
> would be other rare cases like the executable bit was messed up on the file,
or
> the symlink that was hackishly made earlier does not point at the right place.
> 
> > If so, would it be better to fail loudly instead with this hard to
understand
> error message?
> > Using system perf instead of our patched one will break some use cases in
very
> > non-obvious ways.
> 
> By 'loudly' do you mean to print a stack trace and immediately stop? It's
> probably a good idea. Would you like to make the message shorter without
> mentioning the symlinking?

Yes, I'd prefer a stack trace. We already have a bunch of scripts that fail
badly on other linux distros (e.g., build/install-build-deps.sh), so I don't
know how much effort we want to put into making this particular use case work.
Keep it simple and all that :)

[pasko, 2014-08-28 18:20:01]

On 2014/08/28 17:57:50, Sami wrote:
> On 2014/08/28 17:47:32, pasko wrote:
> >
>
https://codereview.chromium.org/474933005/diff/60001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/core/platform/linux_platform_backend.py
> (right):
> > 
> >
>
https://codereview.chromium.org/474933005/diff/60001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/core/platform/linux_platform_backend.py:84:
message
> =
> > ('You may proceed with making a symlink to /usr/bin/perf from '
> > On 2014/08/28 16:36:49, Sami wrote:
> > > Is this a fallback for when the installation breaks with trusty+1?
> > 
> > I guess this will probably be the main case of this being printed. Also,
some
> > people use non-ubuntu-based distributions, they'd observe this message.
There
> > would be other rare cases like the executable bit was messed up on the file,
> or
> > the symlink that was hackishly made earlier does not point at the right
place.
> > 
> > > If so, would it be better to fail loudly instead with this hard to
> understand
> > error message?
> > > Using system perf instead of our patched one will break some use cases in
> very
> > > non-obvious ways.
> > 
> > By 'loudly' do you mean to print a stack trace and immediately stop? It's
> > probably a good idea. Would you like to make the message shorter without
> > mentioning the symlinking?
> 
> Yes, I'd prefer a stack trace. We already have a bunch of scripts that fail
> badly on other linux distros (e.g., build/install-build-deps.sh), so I don't
> know how much effort we want to put into making this particular use case work.
> Keep it simple and all that :)

I did a 'raise Exception' in 2 places for consistency. PTAL and let me know if
there is a better exception class for this job.

[Sami, 2014-08-28 18:27:49]

lgtm, thanks! |Exception| seems like a good fit here.

[pasko, 2014-08-29 07:32:55]

On 2014/08/28 18:27:49, Sami wrote:
> lgtm, thanks! |Exception| seems like a good fit here.

Thank you for review Sami, your fast responses are great!

[pasko, 2014-08-29 07:33:00]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2014-08-29 07:33:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pasko@chromium.org/474933005/80001

[commit-bot: I haz the power, 2014-08-29 07:47:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48014)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/53346)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-29 07:48:12]

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

[commit-bot: I haz the power, 2014-08-29 07:48:13]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[pasko, 2014-08-29 08:04:59]

Ok ok, thank you CQ for letting me know that I should rebase. Will do it in an
hour.

[pasko, 2014-08-29 09:06:59]

pasko@chromium.org changed reviewers:
+ tonyg@chromium.org

[pasko, 2014-08-29 09:18:26]

This clashed with a few changes by Tony:
https://codereview.chromium.org/515603002 ([Telemetry] Clean up host platform
hack in AndroidPlatformBackend.)
https://codereview.chromium.org/510943002 ([Telemetry] Implement
CanLaunchApplication and InstallApplication for Android.)

From the description I read that this is in preparation for the support of the
Trepn profiler.

It will take longer to rebase, since the --profiler=perf is working via some
other code paths which I need to find out.. :)

[pasko, 2014-09-02 12:14:26]

On 2014/08/29 09:18:26, pasko wrote:
> This clashed with a few changes by Tony:
> https://codereview.chromium.org/515603002 ([Telemetry] Clean up host platform
> hack in AndroidPlatformBackend.)
> https://codereview.chromium.org/510943002 ([Telemetry] Implement
> CanLaunchApplication and InstallApplication for Android.)
> 
> From the description I read that this is in preparation for the support of the
> Trepn profiler.
> 
> It will take longer to rebase, since the --profiler=perf is working via some
> other code paths which I need to find out.. :)

The rebase appears to be easy, in the new World there is just no need to change
android_platform_backend.py since it does not care about perfhost any more.
Nice.
I assume the L G T M from 4 days ago still applies after this kind of change,
but you may want to have another look, I'm firing non-CQ tries now.

[Sami, 2014-09-02 12:46:35]

Still lgtm.

[pasko, 2014-09-02 13:35:23]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2014-09-02 13:36:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pasko@chromium.org/474933005/140001

[commit-bot: I haz the power, 2014-09-02 14:36:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-02 15:27:33]

Committed patchset #8 (id:140001) as 68873da8b1b835dd4e08fccf17c4baab3c347148

[commit-bot: I haz the power, 2014-09-10 03:18:37]

Patchset 8 (id:??) landed as
https://crrev.com/7bddaa0d8a4768d9b3d9fa5fbd9e0a56a99b5303
Cr-Commit-Position: refs/heads/master@{#292930}