1130187 - Optimize devicemanager use in Android xpcshell tests

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Geoff Brown [:gbrown]]

I noticed some non-optimal use of devicemanager calls during Android xpcshell tests. Eliminating a remote call can save as much as 300 ms; if that applies to each test case, we can easily save several minutes per test job.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: eliminate extra trips to device

I have tried to eliminate extra trips to the device between tests:
 - instead of separate device manager operations to remove a directory and re-create it, invoke a script to do both
 - don't check for file/directory existence unless necessary
 - don't bother killing cmd[0] -- that's just the shell script that calls xpcshell

Comparing https://treeherder.mozilla.org/#/jobs?repo=try&revision=503709d3b505&exclusion_state=all (base, no change) to https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f01e5fe89dd&exclusion_state=all (with this patch), I see these reductions in job time:

Android 2.3 X1  4445 s -> 3982 s
Android 2.3 X2  5258 s -> 3955 s
Android 2.3 X3   424 s ->  333 s
Android 4.0 X   2539 s -> 1900 s
Android x86 X    902 s ->  783 s

I'm pretty happy with that, and I see no sign of new failures. But I don't want to risk destabilizing the tests, so if any of these changes seem dangerous, please call it out.

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 8560139 [details] [diff] [review]
eliminate extra trips to device

Review of attachment 8560139 [details] [diff] [review]:
-----------------------------------------------------------------

The more I look at this, the better it feels. I have some comments but nothing major. Feel free to take or leave. I'm not familiar with the xpcshell tests and would appreciate it if ted could look this over too.

::: testing/xpcshell/remotexpcshelltests.py
@@ +48,5 @@
>  
>  
>      def setupTempDir(self):
>          # make sure the temp dir exists
> +        self.device.mkDir(self.remoteTmpDir)

This only works because mkDir only fails if the shell command reports a read only file system. If mkDir ever acts like the normal shell command mkdir and returns a failure if the directory already existed this might bite.

This is only called once per test run, right? How much do we save here? One remote call to the device per test run?

@@ +67,5 @@
>          return pluginsDir
>  
>      def setupProfileDir(self):
> +        self.device.shellCheckOutput([remoteJoin(self.remoteBinDir, "resetdir"), self.profileDir]);
> +

I'm inclined to make a method and use it here and below. You could also cache the path to resetdir if you think it is worth it.

def resetDir(self, path):
    self.device.shellCheckOutput([remoteJoin(self.remoteBinDir, "resetdir"), path]);

@@ -148,1 @@
>          self.device.killProcess("xpcshell")

cmd[0] is the shell executing the xpcshell wrapper?

@@ +312,5 @@
> +        # The directory may not exist initially, so rm may fail. 'rm -f' is not
> +        # supported on some Androids. Similarly, 'test' and 'if [ -d ]' are not
> +        # universally available, so we just ignore errors from rm.
> +        f.write("rm -r $1\n")
> +        f.write("mkdir $1\n")

Do you think it is worth quoting $1 just in case someone passed an argument with spaces?

nit: I think it would be easier to read if the comments were contiguous and if we used writelines on a list.

@@ +314,5 @@
> +        # universally available, so we just ignore errors from rm.
> +        f.write("rm -r $1\n")
> +        f.write("mkdir $1\n")
> +        f.close()
> +        remoteWrapper = remoteJoin(self.remoteBinDir, "resetdir")

cache this and reuse in the possible resetDir method?

Comment 3

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #2)
> ::: testing/xpcshell/remotexpcshelltests.py
> @@ +48,5 @@
> >      def setupTempDir(self):
> >          # make sure the temp dir exists
> > +        self.device.mkDir(self.remoteTmpDir)
> 
> This only works because mkDir only fails if the shell command reports a read
> only file system. If mkDir ever acts like the normal shell command mkdir and
> returns a failure if the directory already existed this might bite.
> 
> This is only called once per test run, right? How much do we save here? One
> remote call to the device per test run?

setupTempDir is called once per test, in between each xpcshell, so savings here can be significant: http://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/testing/xpcshell/runxpcshelltests.py#l603. The desktop version calls mkdtemp().

I agree the mkdir might cause trouble if the dir exists. I haven't found a reliable way to check for directory existence in a remote script, so I think the thing to do is delete the directory (ignoring error on delete, in case the directory does not exist) and re-create it -- just like I've done everywhere else in this patch!

> @@ -148,1 @@
> >          self.device.killProcess("xpcshell")
> 
> cmd[0] is the shell executing the xpcshell wrapper?

cmd[0] is "xpcw", the shell script written at http://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/testing/xpcshell/remotexpcshelltests.py#l296.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: eliminate extra trips to device

Clean-up inspired by :bc's first review -- thanks :bc!

:ted -- Anything to add? Any concerns?

Comment 5

[Geoff Brown [:gbrown]]

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0551e43e4c8&exclusion_state=all. Time savings seems similar to Comment 1.

Comment 6

[Bob Clary [:bc] (inactive)]

Comment on attachment 8561396 [details] [diff] [review]
eliminate extra trips to device

Review of attachment 8561396 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/xpcshell/remotexpcshelltests.py
@@ +309,5 @@
>          os.remove(localWrapper)
> +
> +        # Removing and re-creating a directory is a common operation which
> +        # can be implemented more efficiently with a shell script.
> +        localWrapper = tempfile.mktemp()

Do we want to use mkstemp here and above?
https://docs.python.org/2/library/tempfile.html#tempfile.mkstemp

Comment 7

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #6)
> Do we want to use mkstemp here and above?

I'll leave that for another bug -- I think there are a few things like that to be cleaned up.

Comment 8

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f39857e8a948

I'm happy to incorporate any future feedback into another patch, but wanted to get this checked in now.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f39857e8a948

Comment 10

[Geoff Brown [:gbrown]]

Comment on attachment 8561396 [details] [diff] [review]
eliminate extra trips to device

Clearing stale request...