Bug 104891 – Add --profiler=PROFILER option to run-perf-tests to allow specifying which profiler to use on platforms with many

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

[Eric Seidel (no email)]

Add --profiler=PROFILER option to run-perf-tests to allow specifying which profiler to use on platforms with many

[Eric Seidel (no email)]

Created attachment 179228 [details]
Patch

[Eric Seidel (no email)]

As I noted in the ChangeLog, this support is all for linux, I just used /usr/bin/sample as an excuse to be able to code this up from my Mac. :)

We could definitely do nicer error handling, we definitely have the plumbing to do that now.  Right now I think it will just ignore --profile if it's specified on Windows, since it will try to create the Profiler and get back None.

[Eric Seidel (no email)]

Presumably we could also come up with a nice way to dump the available profiler names from the help.  Right now the arguments are parsed before the "platform" object is created, so it would take a bit of fiddling to be able to print the available profilers in the current version of our arguments help.

[Dirk Pranke]

Comment on attachment 179228 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/executive.py:458
> +        return subprocess.Popen(string_args, **kwargs)

I'm not sure that this is a great change ... it might be better to match the underlying syntax and make the callers map to strings instead.

[Eric Seidel (no email)]

(In reply to comment #4)
> (From update of attachment 179228 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179228&action=review
> 
> > Tools/Scripts/webkitpy/common/system/executive.py:458
> > +        return subprocess.Popen(string_args, **kwargs)
> 
> I'm not sure that this is a great change ... it might be better to match the underlying syntax and make the callers map to strings instead.

We can certainly go back to that.  It seemed that repeating the same line in 4 places was kinda silly.  And then once I did this that there is a second line for windows compat which was missing in 2 of the 4 places. :)

[Dirk Pranke]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 179228 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=179228&action=review
> > 
> > > Tools/Scripts/webkitpy/common/system/executive.py:458
> > > +        return subprocess.Popen(string_args, **kwargs)
> > 
> > I'm not sure that this is a great change ... it might be better to match the underlying syntax and make the callers map to strings instead.
> 
> We can certainly go back to that.  It seemed that repeating the same line in 4 places was kinda silly.  And then once I did this that there is a second line for windows compat which was missing in 2 of the 4 places. :)

Yup. It's up to you ...

[Eric Seidel (no email)]

Comment on attachment 179228 [details]
Patch

I'm going to leave it like this until we get into trouble.  Thanks. :)

[WebKit Review Bot]

Comment on attachment 179228 [details]
Patch

Clearing flags on attachment: 179228

Committed r137661: <http://trac.webkit.org/changeset/137661>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

Reverted r137661 for reason:

broke unit tests, run-webkit-tests

Committed r137673: <http://trac.webkit.org/changeset/137673>

[Dirk Pranke]

See, e.g.: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds/3394/steps/webkit_tests/logs/stdio
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/4050/steps/webkitpy-test/logs/stdio

the change to map args to popen to strings apparently missed some cases and/or had unexpected side effects. I haven't fully triaged it yet.

[Eric Seidel (no email)]

This is really just another symptom of us not running test-webkitpy on the EWS bots anymore, and me forgetting that that's teh case. :(

[Eric Seidel (no email)]

Apologies to all for the breakage.  Will upload a fixed patch shortly.

[Eric Seidel (no email)]

Turns out test-webkitpy was OK, but I've now added a test to catch this:

[548/1609] webkitpy.common.system.executive_unittest.ExecutiveTest.test_run_command_args_type erred:                 
  Traceback (most recent call last):
    File "/src/WebKit/Tools/Scripts/webkitpy/common/system/executive_unittest.py", line 115, in test_run_command_args_type
      executive.run_command(command_line('echo', 1))
    File "/src/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 412, in run_command
      _log.debug('"%s" took %.2fs' % (self._command_for_printing(args), time.time() - start_time))
    File "/src/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 375, in _command_for_printing
      return " ".join(escaped_args)
  TypeError: sequence item 3: expected string, int found
  
Ran 1609 tests in 3.005s

[Eric Seidel (no email)]

I'm going to break the Executive changes out into a separate patch and get that in cleanly first. :)

[Eric Seidel (no email)]

Will update this shortly with a patch which can re-land.  Thanks.

[Eric Seidel (no email)]

Created attachment 179408 [details]
Patch

[Eric Seidel (no email)]

Created attachment 179410 [details]
Updated to include perf on linux

[Ryosuke Niwa]

Comment on attachment 179410 [details]
Updated to include perf on linux

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

> Tools/Scripts/webkitpy/common/system/profiler.py:59
> +        if platform.is_mac():
> +            return 'iprofiler'
> +        if platform.is_linux():
> +            return 'perf'

Can this be a dictionary instead?
It also seem weak that we have to have two platform-to-profiler maps in available_profilers_by_name and default_profiler_name.
Can we have two dictionaries like:
platform_to_profilers = {
    'linux'; [Perf, GooglePProg],
    'mac': [IProfiler, Sample, GooglePProf]
}
and then add name() to Profiler subclasses instead?

> Tools/Scripts/webkitpy/common/system/profiler.py:163
> +class Sample(SingleFileOutputProfiler):

Should we add a test for this?

[Ryosuke Niwa]

(In reply to comment #19)
> platform_to_profilers = {
>     'linux'; [Perf, GooglePProg],
>     'mac': [IProfiler, Sample, GooglePProf]
> }

Of course, the point of it being array is so that the first item can be considered as the default for the platform.

[Eric Seidel (no email)]

(In reply to comment #19)
> (From update of attachment 179410 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179410&action=review
> 
> > Tools/Scripts/webkitpy/common/system/profiler.py:59
> > +        if platform.is_mac():
> > +            return 'iprofiler'
> > +        if platform.is_linux():
> > +            return 'perf'
> 
> Can this be a dictionary instead?
> It also seem weak that we have to have two platform-to-profiler maps in available_profilers_by_name and default_profiler_name.
> Can we have two dictionaries like:
> platform_to_profilers = {
>     'linux'; [Perf, GooglePProg],
>     'mac': [IProfiler, Sample, GooglePProf]
> }
> and then add name() to Profiler subclasses instead?

I'm not sure I follow.  We could certainly move the "name" information onto the Profiler class itself.  I'm not sure it belongs there though, since it seems specific to the command-line argument --profiler.  ProfilerFactory is kinda the bridge between the CLI and the actual Profiler classes IMO.

> > Tools/Scripts/webkitpy/common/system/profiler.py:163
> > +class Sample(SingleFileOutputProfiler):
> 
> Should we add a test for this?

Happy to.  Do you have specific parts you'd like tested?

[Eric Seidel (no email)]

(In reply to comment #20)
> (In reply to comment #19)
> > platform_to_profilers = {
> >     'linux'; [Perf, GooglePProg],
> >     'mac': [IProfiler, Sample, GooglePProf]
> > }
> 
> Of course, the point of it being array is so that the first item can be considered as the default for the platform.

Definitely possible.  You'll end up re-listing the common profilers (like GooglePProf, and presumably eventually valgrind, etc.) in each one, but that's OK.

If you can flesh out your sample code more, I'm happy to move the design closer to your suggestions.  I just don't fully understand where you're going yet. :)

[Eric Seidel (no email)]

I think I understand.  Coding up a version of rniwa's suggestion now.

[Ryosuke Niwa]

Comment on attachment 179410 [details]
Updated to include perf on linux

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

>>> Tools/Scripts/webkitpy/common/system/profiler.py:59
>>> +            return 'perf'
>> 
>> Can this be a dictionary instead?
>> It also seem weak that we have to have two platform-to-profiler maps in available_profilers_by_name and default_profiler_name.
>> Can we have two dictionaries like:
>> platform_to_profilers = {
>>     'linux'; [Perf, GooglePProg],
>>     'mac': [IProfiler, Sample, GooglePProf]
>> }
>> and then add name() to Profiler subclasses instead?
> 
> I'm not sure I follow.  We could certainly move the "name" information onto the Profiler class itself.  I'm not sure it belongs there though, since it seems specific to the command-line argument --profiler.  ProfilerFactory is kinda the bridge between the CLI and the actual Profiler classes IMO.

So these two static methods can be re-wrriten as:
def default_profile_name(cls, platform):
    return platform_to_profilers.get(platform.os_name, [None])[0];

def available_profilers_by_name(cls, platform):
    profilers = {}
    for profiler in platform_to_profilers.get(platform.os_name, [GooglePProf]):
        profilers[profilers.name] = profiler
    return profilers

I think it’s much cleaner.

[Ryosuke Niwa]

Hm… on my second thought, shouldn’t default_profiler_name default to gprof given that it’s available everywhere? If not, then adding it always in available_profilers_by_name doesn’t seem right.

[Eric Seidel (no email)]

Created attachment 179417 [details]
Updated per rniwa's suggestions

[WebKit Review Bot]

Comment on attachment 179417 [details]
Updated per rniwa's suggestions

Clearing flags on attachment: 179417

Committed r137714: <http://trac.webkit.org/changeset/137714>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.