Bug 164792 – [Linux] run-benchmark should be able to execute the benchmarks with more browsers

• Status: UNCONFIRMED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

Currently on Linux to use the run-benchmark script you have to pass --platform gtk and the only browser available is minibrowser.

I intend to:

  *  Rename platform gtk to linux, because I feel it looks more correct to run "--platform linux --browser firefox" than "--platform gtk --browser firefox" (for example)
  *  re-factorize the code and create the required classes for running the following browsers on platform linux: minibrowser-gtk, chrome (chromium), firefox and epiphany.

[Carlos Alberto Lopez Perez]

Created attachment 294912 [details]
Patch

[WebKit Commit Bot]

Attachment 294912 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:85:  [LinuxBrowserDriver.close_browsers] Instance of 'Popen' has no 'kill' member  [pylint/E1101] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Alberto Lopez Perez]

(In reply to comment #2)
> Attachment 294912 [details] did not pass style-queue:
> 
> 
> ERROR:
> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.
> py:85:  [LinuxBrowserDriver.close_browsers] Instance of 'Popen' has no
> 'kill' member  [pylint/E1101] [5]
> Total errors found: 1 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Filed: https://bugs.webkit.org/show_bug.cgi?id=164813

[Carlos Alberto Lopez Perez]

ping reviewers?

[Michael Catanzaro]

Comment on attachment 294912 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:35
> +try:
> +    import psutil
> +except ImportError:
> +    pass

I see you print a warning down below, but I'd do so here too.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:87
> +                    _log.warning('python psutil not found, cant check for '
> +                                 'still-alive browser childs to kill.')

cant -> can't

childs -> child processes (or "children" but that's less clear here)

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:119
> +        from gi.repository import Gtk
> +        screen = Gtk.Window().get_screen()
> +        return screen.get_monitor_geometry(screen.get_primary_monitor())

These functions don't exist in GTK+ 4. Can you rewrite it to avoid deprecations? That will make somebody's life easier in the distant future when nothing else in WebKit still depends on GTK+ 3. :)

gdk_screen_get_primary_monitor has been deprecated since version 3.22 and should not be used in newly-written code.
Use gdk_display_get_primary_monitor() instead

gdk_screen_get_monitor_geometry has been deprecated since version 3.22 and should not be used in newly-written code.
Use gdk_monitor_get_geometry() instead

You'll have to test for version 3.22 as the replacement functions are brand new. If it's inconvenient for you, you don't have to; it's not a blocker, just a nice thing to do for whomever has to port this in the future. Could be you or me. :p

Comment on attachment 294912 [details]
Patch

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

> Tools/ChangeLog:18
> +        This depends on psutil, so this will only be done if psutil is installed.

This is a nice feature for testing some changes to run-benchmark without losing opening tabs in the browser.
However, when testing the performance, it would be better to just run one browser instance to avoid some noise. Furthermore, ideally, "close_browsers" should be consistent between different platforms. It would be nicer to make it optional and can be specified by arguments.
Let me know what you think.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:-110
> -                self._browser_driver.close_browsers()

We should not remove this line as we want to relaunch browser between each iteration.

(In reply to comment #6)
> Comment on attachment 294912 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294912&action=review
> 
> > Tools/ChangeLog:18
> > +        This depends on psutil, so this will only be done if psutil is installed.
> 
> This is a nice feature for testing some changes to run-benchmark without
> losing opening tabs in the browser.
> However, when testing the performance, it would be better to just run one
> browser instance to avoid some noise. Furthermore, ideally, "close_browsers"
> should be consistent between different platforms. It would be nicer to make
> it optional and can be specified by arguments.
> Let me know what you think.
> 
> > Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:-110
> > -                self._browser_driver.close_browsers()
> 
> We should not remove this line as we want to relaunch browser between each
> iteration.
Ignore this comment. As we call close_browsers() in _run_one_test().

Comment on attachment 294912 [details]
Patch

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

>> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:-110
>> -                self._browser_driver.close_browsers()
> 
> We should not remove this line as we want to relaunch browser between each iteration.

Ignore this comment. As we've caledl close_browsers() in _run_one_test already.

[Carlos Alberto Lopez Perez]

(In reply to comment #6)
> Comment on attachment 294912 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294912&action=review
> 
> > Tools/ChangeLog:18
> > +        This depends on psutil, so this will only be done if psutil is installed.
> 
> This is a nice feature for testing some changes to run-benchmark without
> losing opening tabs in the browser.
> However, when testing the performance, it would be better to just run one
> browser instance to avoid some noise. Furthermore, ideally, "close_browsers"
> should be consistent between different platforms. It would be nicer to make
> it optional and can be specified by arguments.
> Let me know what you think.
> 

Currently (before this patch) a "killall $browsername" command is executed, which causes all process with name "$browsername" to be terminated.

This is undesired, specially when the user is testing. If I want to test the script run-benchmark with chrome it won't make me happy that when it ends it kills my chrome browser session.

So what this patch changes is:

 * The browsers executed by run-benchmark are started always on a new window, with a temporal profile and a temporal HOME.
 * The pid of the browser process executed is tracked, so when close_browsers() is executed only that pid is kill.
 * It after sending the kill signal to that pid, there are still alive children process, then those are killed also to ensure that not unwanted left-over browser process are left.

So the idea is that now run-benchmark don't messes with the user browser process. All browser process started by the script are executed independently of the user ones, and are closed without interfering on the browser process not directly started by the script.

Comment on attachment 294912 [details]
Patch

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

>>>> Tools/ChangeLog:18
>>>> +        This depends on psutil, so this will only be done if psutil is installed.
>>> 
>>> This is a nice feature for testing some changes to run-benchmark without losing opening tabs in the browser.
>>> However, when testing the performance, it would be better to just run one browser instance to avoid some noise. Furthermore, ideally, "close_browsers" should be consistent between different platforms. It would be nicer to make it optional and can be specified by arguments.
>>> Let me know what you think.
>> 
>> Ignore this comment. As we call close_browsers() in _run_one_test().
> 
> Currently (before this patch) a "killall $browsername" command is executed, which causes all process with name "$browsername" to be terminated.
> 
> This is undesired, specially when the user is testing. If I want to test the script run-benchmark with chrome it won't make me happy that when it ends it kills my chrome browser session.
> 
> So what this patch changes is:
> 
>  * The browsers executed by run-benchmark are started always on a new window, with a temporal profile and a temporal HOME.
>  * The pid of the browser process executed is tracked, so when close_browsers() is executed only that pid is kill.
>  * It after sending the kill signal to that pid, there are still alive children process, then those are killed also to ensure that not unwanted left-over browser process are left.
> 
> So the idea is that now run-benchmark don't messes with the user browser process. All browser process started by the script are executed independently of the user ones, and are closed without interfering on the browser process not directly started by the script.

Yes, I understand the pain of losing opened tabs/sessions in the browser.
As you know, there are 2 use cases for this script:
1. Developing features for run-benchmark script. As you mentioned, killing all chrome is not a desired behavior. During development, we do not actually care about the scores of benchmarks and it is fine to keep the existing browser process running.
2. Testing performance of benchmarks. In this scenario, we care about the final benchmark score and want to get rid of potential impact introduced by running another browser process.
As the second scenario is the major purpose of this script, personally, I would like to kill all browsers by default.
However, I agree with you that it is undesired for developing purpose. So I would recommend to make "killing testing browser process only" as an optional argument. And we can turn on this feature by passing argument like '--keep-existing-browser' (or you can come up with more descriptive argument name).

[Carlos Alberto Lopez Perez]

(In reply to comment #5)
> Comment on attachment 294912 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294912&action=review
> 
> > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:35
> > +try:
> > +    import psutil
> > +except ImportError:
> > +    pass
> 
> I see you print a warning down below, but I'd do so here too.
> 

This is intended because the way run-benchmark currently works. It loads all the main classes to check the platforms and browsers defined. Printing the warning when the file is loaded would cause unwanted noise on other platforms.

> > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:87
> > +                    _log.warning('python psutil not found, cant check for '
> > +                                 'still-alive browser childs to kill.')
> 
> cant -> can't
> 
> childs -> child processes (or "children" but that's less clear here)
> 
> > Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:119
> > +        from gi.repository import Gtk
> > +        screen = Gtk.Window().get_screen()
> > +        return screen.get_monitor_geometry(screen.get_primary_monitor())
> 
> These functions don't exist in GTK+ 4. Can you rewrite it to avoid
> deprecations? That will make somebody's life easier in the distant future
> when nothing else in WebKit still depends on GTK+ 3. :)
> 
>  has been deprecated since version 3.22 and
> should not be used in newly-written code.
> Use gdk_display_get_primary_monitor() instead
> 
> gdk_screen_get_monitor_geometry has been deprecated since version 3.22 and
> should not be used in newly-written code.
> Use gdk_monitor_get_geometry() instead
> 
> You'll have to test for version 3.22 as the replacement functions are brand
> new. If it's inconvenient for you, you don't have to; it's not a blocker,
> just a nice thing to do for whomever has to port this in the future. Could
> be you or me. :p

Does this fix the warnings for you?

--- a/Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py
+++ b/Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py
@@ -114,6 +114,18 @@ class LinuxBrowserDriver(BrowserDriver):
         # trying to import the Gtk module on the global scope of this
         # file to avoid ImportError errors on other platforms.
         # Python imports are cached and only run once, so this should be ok.
-        from gi.repository import Gtk
-        screen = Gtk.Window().get_screen()
-        return screen.get_monitor_geometry(screen.get_primary_monitor())
+        import gi
+        try:
+            gi.require_version('Gtk', '3.0')
+            gi.require_version('Gdk', '3.0')
+        except ValueError:
+            gi.require_version('Gtk', '4.0')
+            gi.require_version('Gdk', '4.0')
+        from gi.repository import Gtk, Gdk
+        if Gtk.get_major_version() == 3 and Gtk.get_minor_version() < 22:
+            screen = Gtk.Window().get_screen()
+            return screen.get_monitor_geometry(screen.get_primary_monitor())
+        else:
+            display = Gdk.Display.get_default()
+            monitor = display.get_primary_monitor()
+            return monitor.get_geometry()

[Carlos Alberto Lopez Perez]

(In reply to comment #10)
> 2. Testing performance of benchmarks. In this scenario, we care about the
> final benchmark score and want to get rid of potential impact introduced by
> running another browser process.
> As the second scenario is the major purpose of this script, personally, I
> would like to kill all browsers by default.
> However, I agree with you that it is undesired for developing purpose. So I
> would recommend to make "killing testing browser process only" as an
> optional argument. And we can turn on this feature by passing argument like
> '--keep-existing-browser' (or you can come up with more descriptive argument
> name).

I agree with the idea that when generating performance-related data the machine should not be doing other thing that running the tests.

However in this scenario is not only important to kill browsers by default, but to ensure that no other process are running. I don't think this script can reasonably guarantee that. Even if we run "killall chrome" when running chrome tests, there still can be a firefox browser open, or any other process using the CPU. I think is the admin/user of the bot/machine the one that should take care of ensuring this happens rather than relying on a simple killall.

But I'm ok with having a switch to make this behaviour conditional.

Maybe the switch should default to not doing a killall?, that way developers testing this script don't get their browsers killed by mistake. 
And I can ensure the gtk+ perf bot runs with the switch enabled by setting the required arguments on BuildSlaveSupport/build.webkit.org-config/master.cfg.

what do you think?

[Daniel Bates]

Comment on attachment 294912 [details]
Patch

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

I did not review this patch for correctness. I noticed some very minor issues.

> Tools/ChangeLog:11
> +        It add also drivers for chrome (chromium), firefox, epiphany and minibrowser-gtk (previously minibrowser).

Nit: chrome => Chrome
chromium => Chromium
firefox => Firefox
epiphany => Epiphany
minibrowser => MiniBrowser

(These are proper names).

> Tools/ChangeLog:17
> +        We also check for the browser childs and we kill them if they are still alive after the browser has been killed.

browser childs?

> Tools/ChangeLog:21
> +        (RunBenchmarkTests.start): The platform is now autodetected and the minibrowser driver is renamed to minibrowser-gtk.

Nit: minibrowser => MiniBrowser

> Tools/ChangeLog:23
> +        (BenchmarkRunner._run_benchmark): Fix a bug: close_browsers was called twice. Its already called inside BenchmarkRunner._run_one_test

Nit: Two colons (:) on the same line :(. Missing a period at the end of this sentence.

> Tools/ChangeLog:52
> +        (load_subclasses): The loading of subclasses was still (even after r196979) sensible to the order in which the modules are loaded.

sensitive?

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:52
> +            raise ValueError('Cant find executable for browser %s. Seached list: %s'
> +                              % (self.browser_name, self.process_search_list))

This is OK as-is. From my understanding we prefer to use String.format() instead of the % operator to build up formatted string as the former is compatible with Python 3 and the latter is not. And we seem to hope to move to Python 3 one day...

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:73
> +                    _log.info('Killing browser %s with pid %d and cmd: %s'
> +                               % (self.browser_name, self._browser_process.pid,
> +                                  ' '.join(main_browser_process.cmdline()).strip()
> +                                  or main_browser_process.name()))

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:80
> +                            _log.info('Killing still alive %s child with pid %d and cmd: %s'
> +                                       % (self.browser_name, browser_child.pid,
> +                                          ' '.join(browser_child.cmdline()).strip()
> +                                          or browser_child.name()))

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:84
> +                    _log.info('Killing browser %s with pid %d'
> +                               % (self.browser_name, self._browser_process.pid))

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:97
> +        _log.info('Executing: %s' % (' '.join(exec_args)))

Ditto.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:102
> +    def _get_first_executable_path_from_list(self, searchlist):

Nit: searchlist => search_list

(We separate words in variable/function/class names with underscores).

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_epiphany_driver.py:36
> +                                   '--profile=%s' % (self._temp_profiledir),

Is it necessary to pass the option name and value as one argument. I mean, we are using subprocess to execute this command without the shell. So, can we write this as two arguments similar to what we did in LinuxFirefoxDriver: '--profile', self._temp_profiledir?

If not, from my understanding we prefer to use String.format() instead of the % operator to build up formatted string as the former is compatible with Python 3 and the latter is not. And we seem to hope to move to Python 3 one day...

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_minibrowsergtk_driver.py:38
> +        self._browser_arguments.append('--geometry=%sx%s' % (self._screen_size().width, self._screen_size().height))

This is OK as-is. From my understanding we prefer to use String.format() instead of the % operator to build up formatted string as the former is compatible with Python 3 and the latter is not. And we seem to hope to move to Python 3 one day...

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:17
> +def getplatform():

Maybe a better name for this function would be default_platform?

> Tools/Scripts/webkitpy/benchmark_runner/run_benchmark.py:23
> +def getdefaultbrowser():

Maybe a better name for this function would be default_browser?

[Michael Catanzaro]

(In reply to comment #11)
> Does this fix the warnings for you?
> 
> ---
> a/Tools/Scripts/webkitpy/benchmark_runner/browser_driver/
> linux_browser_driver.py
> +++
> b/Tools/Scripts/webkitpy/benchmark_runner/browser_driver/
> linux_browser_driver.py
> @@ -114,6 +114,18 @@ class LinuxBrowserDriver(BrowserDriver):
>          # trying to import the Gtk module on the global scope of this
>          # file to avoid ImportError errors on other platforms.
>          # Python imports are cached and only run once, so this should be ok.
> -        from gi.repository import Gtk
> -        screen = Gtk.Window().get_screen()
> -        return screen.get_monitor_geometry(screen.get_primary_monitor())
> +        import gi
> +        try:
> +            gi.require_version('Gtk', '3.0')
> +            gi.require_version('Gdk', '3.0')
> +        except ValueError:
> +            gi.require_version('Gtk', '4.0')
> +            gi.require_version('Gdk', '4.0')
> +        from gi.repository import Gtk, Gdk
> +        if Gtk.get_major_version() == 3 and Gtk.get_minor_version() < 22:
> +            screen = Gtk.Window().get_screen()
> +            return screen.get_monitor_geometry(screen.get_primary_monitor())
> +        else:
> +            display = Gdk.Display.get_default()
> +            monitor = display.get_primary_monitor()
> +            return monitor.get_geometry()

I didn't actually test. I wouldn't use the try/except for gi.require_version as we haven't actually tested with GTK+ 4, but otherwise it looks sane.

[Michael Catanzaro]

(In reply to comment #13)
> This is OK as-is. From my understanding we prefer to use String.format()
> instead of the % operator to build up formatted string as the former is
> compatible with Python 3 and the latter is not. And we seem to hope to move
> to Python 3 one day...

Seems to work just fine:

#!/usr/bin/python3

print('I have %d apples and %d oranges' % (2, 12))

[Carlos Alberto Lopez Perez]

(In reply to comment #14)
> (In reply to comment #11)
> > Does this fix the warnings for you?
> > 
> > ---
> > a/Tools/Scripts/webkitpy/benchmark_runner/browser_driver/
> > linux_browser_driver.py
> > +++
> > b/Tools/Scripts/webkitpy/benchmark_runner/browser_driver/
> > linux_browser_driver.py
> > @@ -114,6 +114,18 @@ class LinuxBrowserDriver(BrowserDriver):
> >          # trying to import the Gtk module on the global scope of this
> >          # file to avoid ImportError errors on other platforms.
> >          # Python imports are cached and only run once, so this should be ok.
> > -        from gi.repository import Gtk
> > -        screen = Gtk.Window().get_screen()
> > -        return screen.get_monitor_geometry(screen.get_primary_monitor())
> > +        import gi
> > +        try:
> > +            gi.require_version('Gtk', '3.0')
> > +            gi.require_version('Gdk', '3.0')
> > +        except ValueError:
> > +            gi.require_version('Gtk', '4.0')
> > +            gi.require_version('Gdk', '4.0')
> > +        from gi.repository import Gtk, Gdk
> > +        if Gtk.get_major_version() == 3 and Gtk.get_minor_version() < 22:
> > +            screen = Gtk.Window().get_screen()
> > +            return screen.get_monitor_geometry(screen.get_primary_monitor())
> > +        else:
> > +            display = Gdk.Display.get_default()
> > +            monitor = display.get_primary_monitor()
> > +            return monitor.get_geometry()
> 
> I didn't actually test. I wouldn't use the try/except for gi.require_version
> as we haven't actually tested with GTK+ 4, but otherwise it looks sane.

Well, I actually tested it on a fedora 25 chroot and the only warning it printed was because not doing a gi.require_version() before importing Gtk or Gdk.

So if the goal is to avoid warnings, I don't see how I can do it without calling gi.require_version() before trying the import.

[Michael Catanzaro]

It's fine as-is, I just don't see much value in providing compatibility with both GTK+ 3 and GTK+ 4 at the same time in this little script when just GTK+ 3 will do. I don't mean to get rid of gi.require_version entirely, but to unconditionally require 3.0.

[Carlos Alberto Lopez Perez]

(In reply to comment #15)
> (In reply to comment #13)
> > This is OK as-is. From my understanding we prefer to use String.format()
> > instead of the % operator to build up formatted string as the former is
> > compatible with Python 3 and the latter is not. And we seem to hope to move
> > to Python 3 one day...
> 
> Seems to work just fine:
> 
> #!/usr/bin/python3
> 
> print('I have %d apples and %d oranges' % (2, 12))

(In reply to comment #15)
> (In reply to comment #13)
> > This is OK as-is. From my understanding we prefer to use String.format()
> > instead of the % operator to build up formatted string as the former is
> > compatible with Python 3 and the latter is not. And we seem to hope to move
> > to Python 3 one day...
> 
> Seems to work just fine:
> 
> #!/usr/bin/python3
> 
> print('I have %d apples and %d oranges' % (2, 12))

It seems the % operator for string formatting was meant to be deprecated with python 3 but that never happened in the end.

There is PEP3101 discouraging its usage, but nothing more than that.

It seems last python 3 even has a new string format option (interpolated strings), so the result is that there are 3 ways of formatting strings. One that only works on python >= 3.6 (interpolated strings), and two ways that work with any version of python: the % operator and string.format.


I personally find String.format() unnecessarily verbose.

In any case I would change the format strings on this patch as asked by Daniel (i guess it will make at least the code style more consistent)

[Carlos Alberto Lopez Perez]

Created attachment 295618 [details]
Patch

Patch for landing addressing comments.

[Carlos Alberto Lopez Perez]

(In reply to comment #12)
> (In reply to comment #10)
> > 2. Testing performance of benchmarks. In this scenario, we care about the
> > final benchmark score and want to get rid of potential impact introduced by
> > running another browser process.
> > As the second scenario is the major purpose of this script, personally, I
> > would like to kill all browsers by default.
> > However, I agree with you that it is undesired for developing purpose. So I
> > would recommend to make "killing testing browser process only" as an
> > optional argument. And we can turn on this feature by passing argument like
> > '--keep-existing-browser' (or you can come up with more descriptive argument
> > name).
> 
> I agree with the idea that when generating performance-related data the
> machine should not be doing other thing that running the tests.
> 
> However in this scenario is not only important to kill browsers by default,
> but to ensure that no other process are running. I don't think this script
> can reasonably guarantee that. Even if we run "killall chrome" when running
> chrome tests, there still can be a firefox browser open, or any other
> process using the CPU. I think is the admin/user of the bot/machine the one
> that should take care of ensuring this happens rather than relying on a
> simple killall.
> 
> But I'm ok with having a switch to make this behaviour conditional.
> 
> Maybe the switch should default to not doing a killall?, that way developers
> testing this script don't get their browsers killed by mistake. 
> And I can ensure the gtk+ perf bot runs with the switch enabled by setting
> the required arguments on
> BuildSlaveSupport/build.webkit.org-config/master.cfg.
> 
> what do you think?

Regarding this suggestion I will open a new bug after landing this patch.
It has to touch also the osx_browser_driver, so better do it on a different bug.

[WebKit Commit Bot]

Attachment 295618 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_browser_driver.py:83:  [LinuxBrowserDriver.close_browsers] Instance of 'Popen' has no 'kill' member  [pylint/E1101] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #12)
> (In reply to comment #10)
> > 2. Testing performance of benchmarks. In this scenario, we care about the
> > final benchmark score and want to get rid of potential impact introduced by
> > running another browser process.
> > As the second scenario is the major purpose of this script, personally, I
> > would like to kill all browsers by default.
> > However, I agree with you that it is undesired for developing purpose. So I
> > would recommend to make "killing testing browser process only" as an
> > optional argument. And we can turn on this feature by passing argument like
> > '--keep-existing-browser' (or you can come up with more descriptive argument
> > name).
> 
> I agree with the idea that when generating performance-related data the
> machine should not be doing other thing that running the tests.
> 
> However in this scenario is not only important to kill browsers by default,
> but to ensure that no other process are running. I don't think this script
> can reasonably guarantee that. Even if we run "killall chrome" when running
> chrome tests, there still can be a firefox browser open, or any other
> process using the CPU. I think is the admin/user of the bot/machine the one
> that should take care of ensuring this happens rather than relying on a
> simple killall.
> 
> But I'm ok with having a switch to make this behaviour conditional.
> 
> Maybe the switch should default to not doing a killall?, that way developers
> testing this script don't get their browsers killed by mistake. 
> And I can ensure the gtk+ perf bot runs with the switch enabled by setting
> the required arguments on
> BuildSlaveSupport/build.webkit.org-config/master.cfg.
> 
> what do you think?

We could try, but my major concern is changing the default behavior seems risky. As it may potentially break things rely on this script.
If we make "killall" as default, we don't need to worry about that. Furthermore, as running this script requires several arguments already, it wouldn't be too much pain to turn on an extra switch.

[WebKit Commit Bot]

Comment on attachment 295618 [details]
Patch

Clearing flags on attachment: 295618

Committed r209090: <http://trac.webkit.org/changeset/209090>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(In reply to comment #22)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > 2. Testing performance of benchmarks. In this scenario, we care about the
> > > final benchmark score and want to get rid of potential impact introduced by
> > > running another browser process.
> > > As the second scenario is the major purpose of this script, personally, I
> > > would like to kill all browsers by default.
> > > However, I agree with you that it is undesired for developing purpose. So I
> > > would recommend to make "killing testing browser process only" as an
> > > optional argument. And we can turn on this feature by passing argument like
> > > '--keep-existing-browser' (or you can come up with more descriptive argument
> > > name).
> > 
> > I agree with the idea that when generating performance-related data the
> > machine should not be doing other thing that running the tests.
> > 
> > However in this scenario is not only important to kill browsers by default,
> > but to ensure that no other process are running. I don't think this script
> > can reasonably guarantee that. Even if we run "killall chrome" when running
> > chrome tests, there still can be a firefox browser open, or any other
> > process using the CPU. I think is the admin/user of the bot/machine the one
> > that should take care of ensuring this happens rather than relying on a
> > simple killall.
> > 
> > But I'm ok with having a switch to make this behaviour conditional.
> > 
> > Maybe the switch should default to not doing a killall?, that way developers
> > testing this script don't get their browsers killed by mistake. 
> > And I can ensure the gtk+ perf bot runs with the switch enabled by setting
> > the required arguments on
> > BuildSlaveSupport/build.webkit.org-config/master.cfg.
> > 
> > what do you think?
> 
> We could try, but my major concern is changing the default behavior seems
> risky. As it may potentially break things rely on this script.
> If we make "killall" as default, we don't need to worry about that.
> Furthermore, as running this script requires several arguments already, it
> wouldn't be too much pain to turn on an extra switch.

Yes, I think this are sensible arguments.

Breaking current behaviour without a good argument is usually a bad idea. And you are right that users of this script should have no trouble by passing an extra argument to avoid the killall.

I have opened bug 165173 for fixing this.