[webkit-reviews] review requested: [Bug 99517] run-perf-tests should have a --profile option for easy profiling : [Attachment 176525] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has asked  for review:
Bug 99517: run-perf-tests should have a --profile option for easy profiling
https://bugs.webkit.org/show_bug.cgi?id=99517

Attachment 176525: Patch
https://bugs.webkit.org/attachment.cgi?id=176525&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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)