Bug 103587 – Add support for run-perf-tests --chromium-android --profile

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Add support for run-perf-tests --chromium-android --profile

[Eric Seidel (no email)]

Created attachment 176622 [details]
does not work

[Eric Seidel (no email)]

I will not get a chance to work on this until next Monday.  If anyone would like to take a crack at adding --chromium-android support before then, please be my guest!

[Adam Barth]

Comment on attachment 176622 [details]
does not work

View in context: https://bugs.webkit.org/attachment.cgi?id=176622&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:693
> +        for line in ps_output.split('\n'):
> +            if line.contains(DRT_APP_PACKAGE):
> +                match = line.match(r'\S\s+(\d+)')
> +                return match.group(1)

This seems like a regexp :)

[Eric Seidel (no email)]

Created attachment 177879 [details]
Works on my machine, needs a bit more code to be general

[Eric Seidel (no email)]

Created attachment 177902 [details]
Needs to not hard-code perfhost_linux, otherwise about ready

[Eric Seidel (no email)]

Created attachment 177963 [details]
now passes all unittests

[Eric Seidel (no email)]

I don't plan to work on Android stuff either Thurs or Friday, but I'll post a final patch next Monday.

Until then, this patch is useable, but is currently hard-coded to /src/chromium-android/perfhost_linux as the perfhost path. :(

[Eric Seidel (no email)]

Created attachment 177964 [details]
slightly nicer error messages

[Eric Seidel (no email)]

Created attachment 178627 [details]
Patch

[Eric Seidel (no email)]

I think this is finally ready for review. :)  It even will handle the case where you don't have perfhost_linux installed and do something reasonable.

The little feature I'd like to still see is detection of profiling=0 builds and warning the user.  That's a common got-cha I feel.

[Dirk Pranke]

Comment on attachment 178627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:503
> +                sys.exit(1)

Can we do the check_configuration() check in check_build() or check_sys_deps() rather than waiting until we try to create drivers? This will cause you to get N failures rather than just one, and we should know whether or not the environment is set up correctly before we actually try to create drivers ...

Plus, having to call sys.exit() almost certainly indicates some layering is wrong.

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 178627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:503
> > +                sys.exit(1)
> 
> Can we do the check_configuration() check in check_build() or check_sys_deps() rather than waiting until we try to create drivers? This will cause you to get N failures rather than just one, and we should know whether or not the environment is set up correctly before we actually try to create drivers ...
> 
> Plus, having to call sys.exit() almost certainly indicates some layering is wrong.

Argh, the tool lost most of my comments ... hang on, more coming.

[Dirk Pranke]

Comment on attachment 178627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:766
> +        self._delay_post_start_tasks = True  # This is a hack to not start the profiler until after the remote process is fully setup.

The flow of control between _start(), _start_once(), the subclass and the superclass is hard to follow. Can we move run_post_start_tasks() into run_test and make this problem go away? I'm not sure how things are supposed to interact with --pause-before-testing (does the profiler need to be attached?)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android_unittest.py:316
> +"""

Do we really need a stack trace this long?

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:296
> +        environment = self._adjust_environment_for_driver(environment)

I'd probably move lines 294-295 into adjust_environment_for_driver and rename it to setup_environ_for_driver() for consistency.

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:306
> +            self._profiler.attach_to_pid(self._pid_on_target())

Most ports don't use the host/target distinction, and so this method name doesn't make as much sense. Maybe call this driver_pid() or pid_to_attach_to() ?

[Philip Rogers]

This patch breaks --profile on linux :'[

[Eric Seidel (no email)]

(In reply to comment #11)
> (From update of attachment 178627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:503
> > +                sys.exit(1)
> 
> Can we do the check_configuration() check in check_build() or check_sys_deps() rather than waiting until we try to create drivers? This will cause you to get N failures rather than just one, and we should know whether or not the environment is set up correctly before we actually try to create drivers ...
> 
> Plus, having to call sys.exit() almost certainly indicates some layering is wrong.

Yes.  I'd *love* to fix this, but fixing this requires encapsulating the "how to talk to a device" logic at least a little, so that we can pass such an object to a static method on AndroidPerf for each device we want to test.

I have the design in my head, I just need to land some bits of:
https://github.com/eseidel/webkit/compare/master...device_connection
first.

I'd like to land this layering violation as part of this first-pass and then fix it in a second patch if that's OK?  Even after this lands, this feature is not ready to "announce" yet.  I plan to send a note to webkit-dev explaining how to use --profile once I've cleaned up the various profiling paths a bit more.

[Eric Seidel (no email)]

(In reply to comment #14)
> This patch breaks --profile on linux :'[

Fixed.  I had broken the environment fiddling code. :(

[Eric Seidel (no email)]

(In reply to comment #13)
> (From update of attachment 178627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android_unittest.py:316
> > +"""
> 
> Do we really need a stack trace this long?

I've removed a few lines.
 
> > Tools/Scripts/webkitpy/layout_tests/port/driver.py:296
> > +        environment = self._adjust_environment_for_driver(environment)
> 
> I'd probably move lines 294-295 into adjust_environment_for_driver and rename it to setup_environ_for_driver() for consistency.

I can't move those two lines as they set local variables which are used later in the function. :)  But I'm happy to rename the method.

> > Tools/Scripts/webkitpy/layout_tests/port/driver.py:306
> > +            self._profiler.attach_to_pid(self._pid_on_target())
> 
> Most ports don't use the host/target distinction, and so this method name doesn't make as much sense. Maybe call this driver_pid() or pid_to_attach_to() ?

Sure, I'll rename it driver_pid() (although that could be confusing as to which pid, since in host/target setups the driver has 2 pids).

[Eric Seidel (no email)]

Comment on attachment 178627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review

>>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:296
>>> +        environment = self._adjust_environment_for_driver(environment)
>> 
>> I'd probably move lines 294-295 into adjust_environment_for_driver and rename it to setup_environ_for_driver() for consistency.
> 
> I can't move those two lines as they set local variables which are used later in the function. :)  But I'm happy to rename the method.

I've renamed it, but I'm not a huge fan of setup_environ_for_driver as a name, as setup_environ_for_server takes a name and returns a whole new env dictionary.  setup_environ_for_driver takes an env dictionary, makes modifications and returns it.

[Dirk Pranke]

Comment on attachment 178627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review

>>>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:503
>>>> +                sys.exit(1)
>>> 
>>> Can we do the check_configuration() check in check_build() or check_sys_deps() rather than waiting until we try to create drivers? This will cause you to get N failures rather than just one, and we should know whether or not the environment is set up correctly before we actually try to create drivers ...
>>> 
>>> Plus, having to call sys.exit() almost certainly indicates some layering is wrong.
>> 
>> Argh, the tool lost most of my comments ... hang on, more coming.
> 
> Yes.  I'd *love* to fix this, but fixing this requires encapsulating the "how to talk to a device" logic at least a little, so that we can pass such an object to a static method on AndroidPerf for each device we want to test.
> 
> I have the design in my head, I just need to land some bits of:
> https://github.com/eseidel/webkit/compare/master...device_connection
> first.
> 
> I'd like to land this layering violation as part of this first-pass and then fix it in a second patch if that's OK?  Even after this lands, this feature is not ready to "announce" yet.  I plan to send a note to webkit-dev explaining how to use --profile once I've cleaned up the various profiling paths a bit more.

Can't you just move this block of code and _find_or_create_symfs() to the ChromiumAndroidPort class? It looks like the only driver-specific thing here is the device serial; does *which* serial really matter?

At any rate, yes, fixing this in a second patch is okay.

[Eric Seidel (no email)]

Comment on attachment 178627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review

>>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:306
>>> +            self._profiler.attach_to_pid(self._pid_on_target())
>> 
>> Most ports don't use the host/target distinction, and so this method name doesn't make as much sense. Maybe call this driver_pid() or pid_to_attach_to() ?
> 
> Sure, I'll rename it driver_pid() (although that could be confusing as to which pid, since in host/target setups the driver has 2 pids).

Actually, I've gone back on this too.  We need some sort of distinction in the driver code between target and host, even if most drivers don't care about it.  Alternatively I could make a HostedDriver subclass at some point to put all this stuff in.  Right now the base driver is very confused about what pids its talkign to and seems to always use the host pids, even though for things like sampling/crashes it probably wants the device pids.

[Dirk Pranke]

(In reply to comment #20)
> (From update of attachment 178627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:306
> >>> +            self._profiler.attach_to_pid(self._pid_on_target())
> >> 
> >> Most ports don't use the host/target distinction, and so this method name doesn't make as much sense. Maybe call this driver_pid() or pid_to_attach_to() ?
> > 
> > Sure, I'll rename it driver_pid() (although that could be confusing as to which pid, since in host/target setups the driver has 2 pids).
> 
> Actually, I've gone back on this too.  We need some sort of distinction in the driver code between target and host, even if most drivers don't care about it.  Alternatively I could make a HostedDriver subclass at some point to put all this stuff in.  Right now the base driver is very confused about what pids its talkign to and seems to always use the host pids, even though for things like sampling/crashes it probably wants the device pids.

It's hard for me to say w/o looking at the code in question, though you could certainly be right ... what code not in chromium_android.py needs to know about this distinction?

[Eric Seidel (no email)]

(In reply to comment #19)
> (From update of attachment 178627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review
> Can't you just move this block of code and _find_or_create_symfs() to the ChromiumAndroidPort class? It looks like the only driver-specific thing here is the device serial; does *which* serial really matter?
> 
> At any rate, yes, fixing this in a second patch is okay.

It makes sense to me that the logic for "what" checks to do should be in AndroidPerf.  Since it should know what tools it needs.

We don't create an AndroidPerf until we create a Driver, currently the Profiler classes are used once per profile and discarded.  Currently (in this patch) AndroidPerf has to duplicate a lot of logic from other ChromiumAndroid classes about how to talk to ADB.  It has to hold a adb_path/device_serial pair.

We need to check each device to make sure it's setup for perf, since the Driver is designed to shard across multiple devices.  So which device_serial does matter. :)

Once we have the adb_path/device_serial pair in its own object, then it's easy to pass that object to class methods on AndroidPerf and have those class methods be able to validate that the passed in "device" object has the right stuff.

[Eric Seidel (no email)]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 178627 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review
> It's hard for me to say w/o looking at the code in question, though you could certainly be right ... what code not in chromium_android.py needs to know about this distinction?

The chromium android port class mostly ignores the baseclass and overrides everything it needs to to work.

Better (in my mind), long-term solutions are to make base.Port (or maybe a new base-class) be host-aware.  So that the details of talking to and Android device are contained in the android-classes, and the details of Chromium are in the chromium classes, and the details of how to talk to a hosted device are generic.  Maybe this is a dream though. :)

[Eric Seidel (no email)]

Comment on attachment 178627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review

>>>>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:306
>>>>> +            self._profiler.attach_to_pid(self._pid_on_target())
>>>> 
>>>> Most ports don't use the host/target distinction, and so this method name doesn't make as much sense. Maybe call this driver_pid() or pid_to_attach_to() ?
>>> 
>>> Sure, I'll rename it driver_pid() (although that could be confusing as to which pid, since in host/target setups the driver has 2 pids).
>> 
>> Actually, I've gone back on this too.  We need some sort of distinction in the driver code between target and host, even if most drivers don't care about it.  Alternatively I could make a HostedDriver subclass at some point to put all this stuff in.  Right now the base driver is very confused about what pids its talkign to and seems to always use the host pids, even though for things like sampling/crashes it probably wants the device pids.
> 
> It's hard for me to say w/o looking at the code in question, though you could certainly be right ... what code not in chromium_android.py needs to know about this distinction?

It's also possible that this is the wrong layer for this.  Maybe we should have some sort of popen wrapper around adb shell, which knows how to give you a local_pid vs. remote_pid, or host_pid vs. target_pid.

ChromiumAndroid currently keeps all the details of being hosted to itself.  base.Port doesn't have to know any of them, making for some kinda odd control flow.  I was attempting to move away from some of that here, but maybe that's premature.

[Eric Seidel (no email)]

Created attachment 179115 [details]
Patch

[Eric Seidel (no email)]

This is what the output looks like:

%run-perf-tests --profile --chromium-android Bindings/first-child.html
Running 1 tests
ANDROID_SYMFS not set, using /src/WebKit/Source/WebKit/chromium/webkit/Release/layout-test-results/symfs
Updating symfs libary (/src/WebKit/Source/WebKit/chromium/webkit/Release/layout-test-results/symfs/data/app-lib/org.chromium.native_test-1/libDumpRenderTree.so) from built copy (/src/WebKit/out/Release/lib/libDumpRenderTree.so)
Running Bindings/first-child.html (1 of 1)
Couldn't record kernel reference relocation symbol
Symbol resolution may be skewed if relocation was used (e.g. kexec).
Check /proc/kallsyms permission or run as root.
Finished: 33.602788 s

[ perf record: Woken up 6 times to write data ]
[ perf record: Captured and wrote 1.453 MB /data/perf.data (~63495 samples) ]
    48.56%  SignalSender  [unknown]             [.] 0x77f12d38      
    12.09%  SignalSender  libDumpRenderTree.so  [.] WebCore::V8SVGComponentTransferFunctionElement::toNative(v8::Handle<v8::Object>)
    11.26%  SignalSender  libDumpRenderTree.so  [.] WebCore::NodeV8Internal::firstChildAttrGetter(v8::Local<v8::String>, v8::AccessorInfo const&)
     9.69%  SignalSender  libDumpRenderTree.so  [.] bool WebCore::DOMDataStore::holderContainsWrapper<v8::AccessorInfo>(v8::AccessorInfo const&, WebCore::ScriptWrappable*)
     9.10%  SignalSender  libDumpRenderTree.so  [.] v8::Handle<v8::Value> WebCore::toV8Fast<v8::AccessorInfo, WebCore::MutationEvent>(WebCore::Node*, v8::AccessorInfo const&, WebCore::MutationEvent*)
     5.91%  SignalSender  libDumpRenderTree.so  [.] std::list<gpu::gles2::VertexAttribManager::VertexAttribInfo*, std::allocator<gpu::gles2::VertexAttribManager::VertexAttribInfo*> >::begin() const
     0.57%  SignalSender  [kernel]              [k] 0xc05fa448      
     0.27%  SignalSender  libc.so               [.] 0x3bc6c         
     0.20%  SignalSender  libDumpRenderTree.so  [.] v8::internal::ShortCircuitConsString(v8::internal::Object**)
     0.10%  SignalSender  libdvm.so             [.] 0x55ab6         

To view the full profile, run:
perf report -i /src/WebKit/Source/WebKit/chromium/webkit/Release/layout-test-results/test-34.data --symfs /src/WebKit/Source/WebKit/chromium/webkit/Release/layout-test-results/symfs

[Dirk Pranke]

Comment on attachment 178627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178627&action=review

>>>>>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:306
>>>>>> +            self._profiler.attach_to_pid(self._pid_on_target())
>>>>> 
>>>>> Most ports don't use the host/target distinction, and so this method name doesn't make as much sense. Maybe call this driver_pid() or pid_to_attach_to() ?
>>>> 
>>>> Sure, I'll rename it driver_pid() (although that could be confusing as to which pid, since in host/target setups the driver has 2 pids).
>>> 
>>> Actually, I've gone back on this too.  We need some sort of distinction in the driver code between target and host, even if most drivers don't care about it.  Alternatively I could make a HostedDriver subclass at some point to put all this stuff in.  Right now the base driver is very confused about what pids its talkign to and seems to always use the host pids, even though for things like sampling/crashes it probably wants the device pids.
>> 
>> It's hard for me to say w/o looking at the code in question, though you could certainly be right ... what code not in chromium_android.py needs to know about this distinction?
> 
> It's also possible that this is the wrong layer for this.  Maybe we should have some sort of popen wrapper around adb shell, which knows how to give you a local_pid vs. remote_pid, or host_pid vs. target_pid.
> 
> ChromiumAndroid currently keeps all the details of being hosted to itself.  base.Port doesn't have to know any of them, making for some kinda odd control flow.  I was attempting to move away from some of that here, but maybe that's premature.

I'm still not a big fan of _pid_on_target(). Is pid_to_attach_to() too redundant?

Otherwise, I take your point but it's hard for me to comment on the appropriateness of the approach w/ seeing code. I would keep as much of the hosting stuff in ChromiumAndroid for now as you can. Once we have the code actually working, we can then refactor things into the appropriate places.

[Dirk Pranke]

Don, you might want to take a look at this patch ... we're looking at what might make sense to restructure the Port and Driver code to be more aware of the host / target distinction for running w/ mobile devices.

Any thoughts?

[WebKit Review Bot]

Comment on attachment 179115 [details]
Patch

Clearing flags on attachment: 179115

Committed r137523: <http://trac.webkit.org/changeset/137523>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

I should note that this is a first-pass implementation and there are many things yet that I would like to improve.

Noteably:
- The output is not particularly clear to those new to the perf tool.
- The code-flow (as discussed above with dpranke) is kinda awkward between Driver and ChromiumDriver
- The sys.exit(1) is a layering violation to be solved soon.
- This code can't detect yet if you built with profiling=0, a common cause of failure.

If you encounter other issues with --profile, I would love to hear about them.  Please file bugs and CC me!

[Don Olmstead]

The patch as noted is a first pass implementation and seems fine to me.

I like the idea of having a differentiation between the host and target, I'm just not sure what the requirements are for the Chromium Android port. For us we use a server that interacts with however many devices. We ship off the tests by interacting with devices through a RESTful API over Python. So I'm guessing there's a bit more indirection in our case.

It was relatively straightforward to pigeon hole in this by subclassing the driver and port. Definitely a bit hacky but nothing too nasty.