Bug 99517 – run-perf-tests should have a --profile option for easy profiling

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

run-perf-tests should have a --profile option for easy profiling

On Mac we'd use instruments.
On Linux, presumably perf or pprof (if that's possible from our tcmalloc implementation).
On windows, I'm not sure.

It's possible we'd need to make thees options more-specific (like --instruments, vs. --pprof, etc.) like they were for run-sunspider.

The goal would be to make it very easy for those not-necessarily familiar with the profiling tools on the various platforms to get useful perf data about their webkit changes.

[Eric Seidel (no email)]

DYLD_FRAMEWORK_PATH=~/Projects/WebKit/WebKitBuild/Release instruments -t "resources/TimeProfile20us.tracetemplate" ../../WebKitBuild/Release/DumpRenderTree ../Dromaeo/dom-query.html

works for Mac.  It's not yet clear to me the exact design I want here.  Presumably we'd hand the Driver some sort of delegate object and it would know how to call out to it to adjust things like it's command line, or right after it started, or adjust it's environment.

For Instruments, we can either:
- Wrap the DRT call in an instruments call OR
- Use 'instruments -p PID' to attach after the fact
In either case we'll need to provide a template file, as configuring instruments from the command line doesn't seem practical.

For perf, I believe we need to wrap the call in "perf record", similar to the first option for instruments:
https://perf.wiki.kernel.org/index.php/Main_Page

For pprof, we just need to set an environment variable.

For valgrind/callgrind we would need to wrap the call.

[Eric Seidel (no email)]

The goal with this would not be to support every option, but rather to give the simplest possible profile, and those who want more advanced options can pass --pause-before-testing or --wrapper themselves.

[Eric Seidel (no email)]

Created attachment 171341 [details]
proof of concept

[Eric Seidel (no email)]

Created attachment 172405 [details]
I have a plan to clean this up, just moving to my other machine

[Eric Seidel (no email)]

Created attachment 174529 [details]
Works on Mac, will fix Linux next week

[Stephanie Lewis]

FYI instruments has a command line tool called iProfiler.

[Eric Seidel (no email)]

Thanks!  I was unaware of that tool.  That seems like a much nicer interface than the "instruments" tool.

[Eric Seidel (no email)]

Created attachment 176054 [details]
Ready for first review, likely needs more refinement

[Dirk Pranke]

Comment on attachment 176054 [details]
Ready for first review, likely needs more refinement

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

> Tools/Scripts/webkitpy/common/profiler.py:52
> +class GooglePProf(Profiler):

It kinda feels like this should be in webkitpy/common/system instead, since the choice of profiler will vary by platform.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:185
> +        return GooglePProf(self.host.workspace, self.host.executive, self.results_directory(), self._path_to_driver(), identifier)

If this is platform-specific, maybe we should have a SystemHost.create_profiler(results_directory, path_to_executable, identifier) instead?

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:143
> +        self._profiler = self._port.create_profiler()

won't this always run things under profilers?

[Eric Seidel (no email)]

Comment on attachment 176054 [details]
Ready for first review, likely needs more refinement

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

>> Tools/Scripts/webkitpy/common/profiler.py:52
>> +class GooglePProf(Profiler):
> 
> It kinda feels like this should be in webkitpy/common/system instead, since the choice of profiler will vary by platform.

That's fine.  My logic here was that these things sit on top of basic system services (like executive, environment, scm, etc.), but in the same sense that SCM is a platform-agnostic wrapper around SCM, this too is a platform-agnostic wrapper around profiling and thus I can see it fitting in system/

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:185
>> +        return GooglePProf(self.host.workspace, self.host.executive, self.results_directory(), self._path_to_driver(), identifier)
> 
> If this is platform-specific, maybe we should have a SystemHost.create_profiler(results_directory, path_to_executable, identifier) instead?

That's fine too, and that would get rid of two of the args. :)

>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:143
>> +        self._profiler = self._port.create_profiler()
> 
> won't this always run things under profilers?

Opps!  I meant to add an if before uploading, but forgot. :)

[Ryosuke Niwa]

Comment on attachment 176054 [details]
Ready for first review, likely needs more refinement

Can we add a test?

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 176054 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176054&action=review
> 
> >> Tools/Scripts/webkitpy/common/profiler.py:52
> >> +class GooglePProf(Profiler):
> > 
> > It kinda feels like this should be in webkitpy/common/system instead, since the choice of profiler will vary by platform.
> 
> That's fine.  My logic here was that these things sit on top of basic system services (like executive, environment, scm, etc.), but in the same sense that SCM is a platform-agnostic wrapper around SCM, this too is a platform-agnostic wrapper around profiling and thus I can see it fitting in system/
>

Yeah, it's not 100% clear to me whether this should be in system or not. I would be fine w/ it being here as well. Just remember that layout_tests/port isn't allowed to require Host objects (only SystemHosts).

[Eric Seidel (no email)]

(In reply to comment #11)
> (From update of attachment 176054 [details])
> Can we add a test?

Certainly!  I'm grateful for the review feedback, will upload a new patch with modifications tomorrow.

[WebKit Review Bot]

Comment on attachment 176054 [details]
Ready for first review, likely needs more refinement

Attachment 176054 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15003328

[Daniel Bates]

Comment on attachment 176054 [details]
Ready for first review, likely needs more refinement

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

> Tools/Scripts/webkitpy/common/profiler.py:74
> +# FIXME: iprofile is a newer commandline interface to replace /usr/bin/instruments.

Nit: commandline => command-line

[Eric Seidel (no email)]

Created attachment 176525 [details]
Patch

[Adam Barth]

Comment on attachment 176525 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/profiler.py:39
> +        return GooglePProf(host.workspace, host.executive, executable_path, output_dir, identifier)

I wonder if we should have a host.platform.is_linux() branch here and then raise a not implemented exception otherwise.

> Tools/Scripts/webkitpy/common/system/profiler.py:61
> +    def __init__(self, workspace, executive, executable_path, output_dir, output_suffix, identifier=None, ):

Why is there a ", " at the end of this declaration?

> Tools/Scripts/webkitpy/common/system/profiler.py:80
> +        profile_lines = profile_text.split('\n')[:100]

Why 100?

> Tools/Scripts/webkitpy/common/system/profiler.py:86
> +        first_sample_index = 0
> +        for index, line in enumerate(profile_lines):
> +            if line.startswith("Total"):
> +                first_sample_index = index + 1
> +                break
> +        print "\n".join(profile_lines[first_sample_index:first_sample_index + 10])

This whole thing seems like it could be improved.  Can't you do this with a single multiline regular expression?  You can search for Total at the beginning of a line followed by 0-10 additional lines.

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:332
> +

This change seems suprious

[Dirk Pranke]

Comment on attachment 176525 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/profiler.py:86
>> +        print "\n".join(profile_lines[first_sample_index:first_sample_index + 10])
> 
> This whole thing seems like it could be improved.  Can't you do this with a single multiline regular expression?  You can search for Total at the beginning of a line followed by 0-10 additional lines.

I'm not actually even clear what did_stop() is supposed to be doing ... it seems like the idea is to wait until something has exited and then run pprof, right? Maybe this should be named to profile_after_exit() or something? Does instruments log automatically on the mac, and that's why the mac did_stop() is a no-op?

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:307
> +            self._profiler.did_stop()

maybe just put this inside the if self._server_process: block, as if self._profiler: \ self._profiler.did_stop() ? The was_running flag seems unnecessary.

[Eric Seidel (no email)]

Comment on attachment 176525 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/profiler.py:39
>> +        return GooglePProf(host.workspace, host.executive, executable_path, output_dir, identifier)
> 
> I wonder if we should have a host.platform.is_linux() branch here and then raise a not implemented exception otherwise.

It's possible to use google-pprof on any platform. :)  All you need is a binary built with a modern tcmalloc is my understanding.  tcmalloc signs up for SIG_PROF when CPUPROFILE is set in the environment and writes stacks to the specified file at some pre-determined interval.  None of that is linux specific AFAIK.  I'm not sure what sort of callback it uses on Windows, but my impression was that it works there too?

>> Tools/Scripts/webkitpy/common/system/profiler.py:61
>> +    def __init__(self, workspace, executive, executable_path, output_dir, output_suffix, identifier=None, ):
> 
> Why is there a ", " at the end of this declaration?

Fixed.

>> Tools/Scripts/webkitpy/common/system/profiler.py:86
>> +        print "\n".join(profile_lines[first_sample_index:first_sample_index + 10])
> 
> This whole thing seems like it could be improved.  Can't you do this with a single multiline regular expression?  You can search for Total at the beginning of a line followed by 0-10 additional lines.

Fixed (and tested)

[Eric Seidel (no email)]

Sorry, didn't mean to override your flag chagnes.  I'll be uploading a new one shortly.

[Eric Seidel (no email)]

Created attachment 176536 [details]
Patch

[Adam Barth]

(In reply to comment #18)
> (From update of attachment 176525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176525&action=review
> 
> >> Tools/Scripts/webkitpy/common/system/profiler.py:86
> >> +        print "\n".join(profile_lines[first_sample_index:first_sample_index + 10])
> > 
> > This whole thing seems like it could be improved.  Can't you do this with a single multiline regular expression?  You can search for Total at the beginning of a line followed by 0-10 additional lines.
> 
> I'm not actually even clear what did_stop() is supposed to be doing ... it seems like the idea is to wait until something has exited and then run pprof, right? Maybe this should be named to profile_after_exit() or something? Does instruments log automatically on the mac, and that's why the mac did_stop() is a no-op?

Yeah, this class is a little funny since it has two virtual methods and each subclass overrides a different one.  I get where Eric is going, but I'm not sure we have enough examples to work out the right design.

> > Tools/Scripts/webkitpy/layout_tests/port/driver.py:307
> > +            self._profiler.did_stop()
> 
> maybe just put this inside the if self._server_process: block, as if self._profiler: \ self._profiler.did_stop() ? The was_running flag seems unnecessary.

Looks like Eric addressed this comment.

[Adam Barth]

Comment on attachment 176536 [details]
Patch

My guess is that we'll need to iterate on the design of the Profiler class as we get more examples of profilers, but I think it makes sense to start with this patch.

[Eric Seidel (no email)]

Comment on attachment 176525 [details]
Patch

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

>>>>> Tools/Scripts/webkitpy/common/system/profiler.py:86
>>>>> +        print "\n".join(profile_lines[first_sample_index:first_sample_index + 10])
>>>> 
>>>> This whole thing seems like it could be improved.  Can't you do this with a single multiline regular expression?  You can search for Total at the beginning of a line followed by 0-10 additional lines.
>>> 
>>> I'm not actually even clear what did_stop() is supposed to be doing ... it seems like the idea is to wait until something has exited and then run pprof, right? Maybe this should be named to profile_after_exit() or something? Does instruments log automatically on the mac, and that's why the mac did_stop() is a no-op?
>> 
>> Fixed (and tested)
> 
> Yeah, this class is a little funny since it has two virtual methods and each subclass overrides a different one.  I get where Eric is going, but I'm not sure we have enough examples to work out the right design.

Correct.  instruments on the mac prints information on where it saved the profile to stdout.  The did_stop for pprof just makes it nice to see the profiles of many tests.  What you're seeing is two separate designs merged into one class. :)  I suspect we'll iterate many times from here.

>>> Tools/Scripts/webkitpy/layout_tests/port/driver.py:307
>>> +            self._profiler.did_stop()
>> 
>> maybe just put this inside the if self._server_process: block, as if self._profiler: \ self._profiler.did_stop() ? The was_running flag seems unnecessary.
> 
> Looks like Eric addressed this comment.

Ah true!  Fixed.

[Dirk Pranke]

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

> Tools/Scripts/webkitpy/common/system/profiler.py:57
> +    def did_stop(self):

I still don't know what "did_stop" is supposed to do or when (and why) I'm supposed to call it ... Can we either pick a better name, or add a comment or a doc string or something? Is this supposed to be called after the process is determined to have exited? only after you explicitly kill the attached process? Should we maybe change the interface to something that allows the profiler to control how the process is stopped, etc.?

Note that I'm basically fine w/ this patch as-is except for this method name, just to be clear :).

> Tools/Scripts/webkitpy/common/system/profiler_unittest.py:82
> +"""

Is it possible to make this test output smaller without losing any essential points?

[Eric Seidel (no email)]

Created attachment 176546 [details]
This patch should actually apply

[Dirk Pranke]

(In reply to comment #24)
> >>> I'm not actually even clear what did_stop() is supposed to be doing ... it seems like the idea is to wait until something has exited and then run pprof, right? Maybe this should be named to profile_after_exit() or something? Does instruments log automatically on the mac, and that's why the mac did_stop() is a no-op?
> >> 
> >> Fixed (and tested)
> > 
> > Yeah, this class is a little funny since it has two virtual methods and each subclass overrides a different one.  I get where Eric is going, but I'm not sure we have enough examples to work out the right design.
> 
> Correct.  instruments on the mac prints information on where it saved the profile to stdout.  The did_stop for pprof just makes it nice to see the profiles of many tests.  What you're seeing is two separate designs merged into one class. :)  I suspect we'll iterate many times from here.
> 

Right, I understand how you got here and am okay with iterating the code, it's just that the patch as-is is too terse for me to understand what the heck is going on or how I could possibly judge something to be correct :).

[Eric Seidel (no email)]

It sounds like you'd like did_stop renamed.  Happy to rename it to profile_after_exit per your suggestion.

[Eric Seidel (no email)]

Created attachment 176556 [details]
renamed did_stop to profile_after_exit

[Dirk Pranke]

Thanks! Sorry if I wasn't clearer before, that was indeed all I was asking for :).

[WebKit Review Bot]

Comment on attachment 176556 [details]
renamed did_stop to profile_after_exit

Clearing flags on attachment: 176556

Committed r136051: <http://trac.webkit.org/changeset/136051>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Once I've iterated on this a few more times I'll make an announcement on webkit-dev about how to use this for easier profiling.  For now folks should feel encouraged to try this out, and please file bugs when it falls on its face in your configuration. :)

[Eric Seidel (no email)]

(In reply to comment #6)
> FYI instruments has a command line tool called iProfiler.

Wrote a patch to move to iprofiler in bug 103765.