Bug 103765 – run-perf-tests --profile should use iprofiler instead of instruments on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

run-perf-tests --profile should use iprofile instead of instruments on Mac

[Eric Seidel (no email)]

Created attachment 177015 [details]
Patch

[Eric Seidel (no email)]

CCing folks who likely have opinions about Mac profiling tools.

I will caution all that --profile is rather new and not the perfect experience on Mac.  "iprofiler" runs asynchronously from the tests, does its own printing to stderr, and doesn't provide us a way to dump any sort of perf summary from the commandline.  These combine to make it slightly at-odds with the --profile design (which was originally written for linux-command-line-profilers).

In any case, I think --profile will grow to be more useful, and with some help will work cleaner on Mac than it does.

[Eric Seidel (no email)]

Forgot to CC folks who have reviewed past --profile patches, sorry.

[Eric Seidel (no email)]

One final note:

As I alluded to in the ChangeLog, this is not a strict improvement from /usr/bin/instruments, but I figure that since /usr/bin/instruments seems "deprecated" it's best to move to the newer tool and iterate from there.

[Eric Seidel (no email)]

Created attachment 177018 [details]
Made IProfiler.profile_after_exit wait for iprofiler to complete before continuing

[Ryosuke Niwa]

Comment on attachment 177018 [details]
Made IProfiler.profile_after_exit wait for iprofiler to complete before continuing

rs=me.

[Adam Barth]

Comment on attachment 177018 [details]
Made IProfiler.profile_after_exit wait for iprofiler to complete before continuing

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

> Tools/Scripts/webkitpy/common/system/profiler.py:101
> +        # FIXME: Consider capturing instead of letting instruments spam to stderr directly.
> +        self._profiler_process = self._host.executive.popen(cmd)

I guess I can try it, but what does it spam to stderr?  Capturing the output and post-processing it like we do for pprof sounds like it might be a good next step.

[Eric Seidel (no email)]

Comment on attachment 177018 [details]
Made IProfiler.profile_after_exit wait for iprofiler to complete before continuing

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

>> Tools/Scripts/webkitpy/common/system/profiler.py:101
>> +        self._profiler_process = self._host.executive.popen(cmd)
> 
> I guess I can try it, but what does it spam to stderr?  Capturing the output and post-processing it like we do for pprof sounds like it might be a good next step.

iprofiler: Profiling process 7271 (DumpRenderTree) for 10 seconds
iprofiler: Profiling completed...
iprofiler: Session saved at /Users/eseidel/Projects/WebKit/WebKitBuild/Release/layout-test-results//Users/eseidel/Projects/WebKit/WebKitBuild/Release/layout-test-results/test.dtps

The output is really nice. :)  But since we don't control it is makes some things more complicated (like changing the name of the to-save-test-file-after Profiler initialization -- which I want to do so I can save these files at test/file/path.trace instead of just test.trace).

One complexity in trying to capture the output is that iprofiler will prompt the user for their password if necessary.  We'd have to make sure to route input to the iprofiler process as well.

[Eric Seidel (no email)]

Comment on attachment 177018 [details]
Made IProfiler.profile_after_exit wait for iprofiler to complete before continuing

I now realize I think its splitting out the profile in the wrong place.  Will upload a fix.

[Eric Seidel (no email)]

Created attachment 177022 [details]
Patch for landing

[Eric Seidel (no email)]

Yup.  Missed a fs.basename() call.  Fixed.

[WebKit Review Bot]

Comment on attachment 177022 [details]
Patch for landing

Clearing flags on attachment: 177022

Committed r136327: <http://trac.webkit.org/changeset/136327>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Glenn Adams]

revision http://trac.webkit.org/changeset/136327 broke test-webkitpy (on mac-mountainlion):

sn-ga-02:webkitsvn glenn$ Tools/Scripts/test-webkitpy
Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really, slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

Checking imports ...Failed to import webkitpy.common.system.profiler_unittest:
  Traceback (most recent call last):
    File "/Users/glenn/work/webkitsvn/Tools/Scripts/webkitpy/test/main.py", line 176, in _check_imports
      __import__(name)
    File "/Users/glenn/work/webkitsvn/Tools/Scripts/webkitpy/common/system/profiler_unittest.py", line 33, in <module>
      from .profiler import ProfilerFactory, Instruments, GooglePProf
  ImportError: cannot import name Instruments

[Eric Seidel (no email)]

Sorry.  Easy fix.  Will fix shortly.

[Eric Seidel (no email)]

Reopening to attach new patch.

[Eric Seidel (no email)]

Created attachment 177144 [details]
Fix test-webkitpy

[Eric Seidel (no email)]

Comment on attachment 177144 [details]
Fix test-webkitpy

Sorry.  the Commit-queue doesn't run test-webkitpy at the moment.  I think Dirk said he's working on fixing that.

[WebKit Review Bot]

Comment on attachment 177144 [details]
Fix test-webkitpy

Rejecting attachment 177144 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

/mnt/git/webkit-commit-queue/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/15120063

[Eric Seidel (no email)]

Created attachment 177146 [details]
Patch for landing

[Adam Barth]

Comment on attachment 177144 [details]
Fix test-webkitpy

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

> Tools/ChangeLog:6
> +        Unreviwed.  Updating the unittests after my previous change.

Unreviwed -> Unreviewed.

[WebKit Review Bot]

Comment on attachment 177146 [details]
Patch for landing

Clearing flags on attachment: 177146

Committed r136351: <http://trac.webkit.org/changeset/136351>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Glenn Adams]

thanks, that fixed it (comment #14)