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

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Eric Seidel <eric at webkit.org>'s
request 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 Dirk Pranke <dpranke at chromium.org>
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.