Closed Bug 1130187 Opened 5 years ago Closed 5 years ago

Optimize devicemanager use in Android xpcshell tests

Categories

(Testing :: XPCShell Harness, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gbrown, Assigned: gbrown)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch eliminate extra trips to device (obsolete) — Splinter Review
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.
Attachment #8560139 - Flags: review?(bob)
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?
Attachment #8560139 - Flags: review?(bob) → review+
(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.
Clean-up inspired by :bc's first review -- thanks :bc!

:ted -- Anything to add? Any concerns?
Attachment #8560139 - Attachment is obsolete: true
Attachment #8561396 - Flags: review?(bob)
Attachment #8561396 - Flags: feedback?(ted)
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
Attachment #8561396 - Flags: review?(bob) → review+
(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.
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.
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Comment on attachment 8561396 [details] [diff] [review]
eliminate extra trips to device

Clearing stale request...
Attachment #8561396 - Flags: feedback?(ted)
You need to log in before you can comment on or make changes to this bug.