GNOME Bugzilla – Bug 644265
perf: standardize window configurations
Last modified: 2011-03-12 00:37:35 UTC
One of the big limitations of the performance collection is that everybody has been running the tests with different window setups. This patch set fixes that.
Created attachment 182904 [details] [review] gnome-shell-jhbuild: Fix race condition when launching DConf manually We need to connect to the NameOwnerChanged signal before we execute the DConf binary. Refactor things so that we connect to the signal when we first get the bus object. Split the code for waiting for a D-Bus name into a wait_for_dbus_name() function for latter reuse.
Created attachment 182905 [details] [review] gnome-shell-perf-helper: add server for creating test windows Add a small D-Bus server program that the performance tests can use to create well-specificed sets of windows.
Created attachment 182906 [details] [review] Use gnome-shell-perf-helper to control windows during perf tests * Run gnome-shell-perf-helper during performance tests * Use MUTTER_WM_CLASS_FILTER to omit all other windows * Add new Scripting methods: createTestWindow, waitTestWindows, destroyTestWindows * Create a single 640x480 test window for testing overview animation performance.
Created attachment 182907 [details] [review] scripting.js: small cleanups - Fix missing trailing semicolons - Catch when something results in metrics not being to avoid hard-to-debug JSON parse errors.
Created attachment 182908 [details] [review] perf: Add metrics for different window configurations Add metrics: overviewFps5Windows overviewFps10Windows overviewFps5Maximzed overviewFps10Maximized overviewFps5Alpha overviewFps10Alpha To have more numbers to show how performance varies with different numbers of windows and different types of windows (maximized, with an alpha channel.)
Review of attachment 182904 [details] [review]: ::: src/gnome-shell-jhbuild.in @@ +91,3 @@ + global _bus + if _bus is None: + dbus_loop = DBusGMainLoop() I think we're just using this for side effects, so it's probably ok to not be an explicit global. @@ +118,3 @@ + + def on_timeout(): + print "\nFailed to start %s: timed out" % (name,) s/name/wait_name/
Review of attachment 182905 [details] [review]: ::: src/shell-perf-helper.c @@ +309,3 @@ + + /* Since we depend on this, avoid the possibility of lt-gnome-shell-perf-helper */ + g_set_prgname("gnome-shell-perf-helper"); Missing a space.
Review of attachment 182906 [details] [review]: ::: src/gnome-shell-jhbuild.in @@ +178,3 @@ + + self_dir = os.path.dirname(os.path.abspath(sys.argv[0])) + running_from_source_tree = os.path.exists(os.path.join(self_dir, 'gnome-shell-jhbuild.in')) This is duplicating other code, no? I moved it to the top in a different patch.
(In reply to comment #6) > Review of attachment 182904 [details] [review]: > > ::: src/gnome-shell-jhbuild.in > @@ +91,3 @@ > + global _bus > + if _bus is None: > + dbus_loop = DBusGMainLoop() > > I think we're just using this for side effects, so it's probably ok to not be > an explicit global. The larger section of code is: global _bus if _bus is None: dbus_loop = DBusGMainLoop() _bus = dbus.SessionBus(mainloop=dbus_loop) return _bus since _bus is assigned to in the function, it is bound locally without the explicit global. The fact that it is referred to before being assigned to doesn't matter. > @@ +118,3 @@ > + > + def on_timeout(): > + print "\nFailed to start %s: timed out" % (name,) > > s/name/wait_name/ Yeah.
(In reply to comment #8) > Review of attachment 182906 [details] [review]: > > ::: src/gnome-shell-jhbuild.in > @@ +178,3 @@ > + > + self_dir = os.path.dirname(os.path.abspath(sys.argv[0])) > + running_from_source_tree = os.path.exists(os.path.join(self_dir, > 'gnome-shell-jhbuild.in')) > > This is duplicating other code, no? I moved it to the top in a different > patch. Going to skip fixing this for now - I have a more massive refactoring of the python launcher code in progress which will fix address, but I don't want to have random global variables passing around through the current code - currently the only variable that is used globally is 'options'.
Review of attachment 182907 [details] [review]: Looks obviously fine
Review of attachment 182908 [details] [review]: ::: js/perf/core.js @@ +71,3 @@ + for (let i = 0; i < 2 * WINDOW_CONFIGS.length; i++) { + if ((i % 2) == 0) { + let config = WINDOW_CONFIGS[i / 2]; This could use a comment like /* Results in 0..WINDOW_CONFIGS.length, but run the overview in between each one */ @@ +162,3 @@ + // numbers without mipmap generation. + if (overviewShowCount % 2 == 0) { + let config = WINDOW_CONFIGS[(overviewShowCount / 2) - 1]; Won't this index -1 if overviewShowCount == 0? I'm not sure if that can happen but it would seem sane to me to have a guard.
(In reply to comment #12) > Review of attachment 182908 [details] [review]: > > ::: js/perf/core.js > @@ +71,3 @@ > + for (let i = 0; i < 2 * WINDOW_CONFIGS.length; i++) { > + if ((i % 2) == 0) { > + let config = WINDOW_CONFIGS[i / 2]; > > This could use a comment like > > /* Results in 0..WINDOW_CONFIGS.length, but run the overview in between each > one */ Went with: // We go to the overview twice for each configuration; the first time // to calculate the mipmaps for the windows, the second time to get // a clean set of numbers. > > @@ +162,3 @@ > + // numbers without mipmap generation. > + if (overviewShowCount % 2 == 0) { > + let config = WINDOW_CONFIGS[(overviewShowCount / 2) - 1]; > > Won't this index -1 if overviewShowCount == 0? I'm not sure if that can happen > but it would seem sane to me to have a guard. Changed the comment to: // Other than overviewLatencyFirst, we collect FPS metrics the second // we show each window configuration. overviewShowCount is 1,2,3... to make clear why the - 1 can't matter (the increment of overviewShowCount is a few lines above.)
Attachment 182904 [details] pushed as 1954a02 - gnome-shell-jhbuild: Fix race condition when launching DConf manually Attachment 182905 [details] pushed as c6a2814 - gnome-shell-perf-helper: add server for creating test windows Attachment 182906 [details] pushed as 821583a - Use gnome-shell-perf-helper to control windows during perf tests Attachment 182907 [details] pushed as 44a1bc5 - scripting.js: small cleanups Attachment 182908 [details] pushed as 239aa7b - perf: Add metrics for different window configurations