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.
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.
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.
Created attachment 171341 [details] proof of concept
Created attachment 172405 [details] I have a plan to clean this up, just moving to my other machine
Created attachment 174529 [details] Works on Mac, will fix Linux next week
FYI instruments has a command line tool called iProfiler.
Thanks! I was unaware of that tool. That seems like a much nicer interface than the "instruments" tool.
Created attachment 176054 [details] Ready for first review, likely needs more refinement
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?
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. :)
Comment on attachment 176054 [details] Ready for first review, likely needs more refinement Can we add a test?
(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).
(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.
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
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
Created attachment 176525 [details] Patch
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
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.
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)
Sorry, didn't mean to override your flag chagnes. I'll be uploading a new one shortly.
Created attachment 176536 [details] Patch
(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.
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.
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.
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?
Created attachment 176546 [details] This patch should actually apply
(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 :).
It sounds like you'd like did_stop renamed. Happy to rename it to profile_after_exit per your suggestion.
Created attachment 176556 [details] renamed did_stop to profile_after_exit
Thanks! Sorry if I wasn't clearer before, that was indeed all I was asking for :).
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>
All reviewed patches have been landed. Closing bug.
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. :)
(In reply to comment #6) > FYI instruments has a command line tool called iProfiler. Wrote a patch to move to iprofiler in bug 103765.