807841 - Speed up test setup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

A noticed a two things while trying to get tests running here:

We have a few sleep(1) calls in devicemanagerADB.py that essentially force every file copy operation to take 1sec. Since most of these operations are a copy/push to a temp location followed by a second copy and a delete, they take at least 3 seconds. The operations are a lot faster than that. Can we reduce this to something like 0.01sec? Doing so cuts the average time for a single file copy to .3sec for me.

We also remove files one at a time, which isn't fast either. Doing a single rm -Rf call is faster. Is there some reason we can't do that? Posting patches for both things.

Patches coming

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Sleep Patch

This decreases sleep times

Comment 2

[Wesley Johnston (:wesj)]

Attachment: delete patch

This changes removeDir to use rm -Rf. I have a feeling there are devices where this doesn't work and that's why we're not using it?

Comment 3

[William Lachance (:wlach)]

(In reply to Wesley Johnston (:wesj) from comment #2)
> Created attachment 677602 [details] [diff] [review]
> delete patch
> 
> This changes removeDir to use rm -Rf. I have a feeling there are devices
> where this doesn't work and that's why we're not using it?

I believe this method was originally copied verbatim from sutagent. I don't think there's any Android platform we care about where rm -Rf won't work.

Thanks for these patches, they should really improve the debugging experience for B2G as well.

Comment 4

[Geoff Brown [:gbrown]]

(In reply to William Lachance (:wlach) from comment #3)
> (In reply to Wesley Johnston (:wesj) from comment #2)
> > Created attachment 677602 [details] [diff] [review]
> > delete patch
> > 
> > This changes removeDir to use rm -Rf. I have a feeling there are devices
> > where this doesn't work and that's why we're not using it?
> 
> I believe this method was originally copied verbatim from sutagent. I don't
> think there's any Android platform we care about where rm -Rf won't work.

"rm -Rf" doesn't work on the tegras. "rm -r" and "rm -R" are OK.

Comment 5

[Geoff Brown [:gbrown]]

I ran "make xpcshell-tests-remote" with the sleep patch -- it ran much faster, thanks!

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

yeah, 'rm -R' is what I use on the pandas and tegras.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 677602 [details] [diff] [review]
delete patch

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

r+ assuming we change this to -r instead of -rF.  -rF doesn't work on panda or tegra.

::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
@@ +270,5 @@
>          """
>          Does a recursive delete of directory on the device: rm -Rf remoteDir
>          """
>          if (self.dirExists(remoteDir)):
> +            self._runCmd(["shell", "rm", "-rF", remoteDir])

rm -r, not rm -rF

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 677601 [details] [diff] [review]
Sleep Patch

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

I need to experiment a bit more with this.  One thing is that our tbpl based automation uses the SUTAgent, so these changes won't fix that.  That doesn't mean we won't benefit from adb testing or maybe porting the same changes to SUT.  

For this sleep, I would like to make it configurable.  Ideally we would have device profiles which could set any timing related flags/variables.  Right now I think going from 1 second to 0.01 seconds is quite a jump.

Can we put this as a devicemanagerADB level variable for sleep, we could set the default to 0.01, but if we have troubles, it could be reverted back or adjusted to a safe default level.

Comment 9

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 677601 [details] [diff] [review]
> Sleep Patch
> 
> Review of attachment 677601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I need to experiment a bit more with this.  One thing is that our tbpl based
> automation uses the SUTAgent, so these changes won't fix that.  That doesn't
> mean we won't benefit from adb testing or maybe porting the same changes to
> SUT.  
> 
> For this sleep, I would like to make it configurable.  Ideally we would have
> device profiles which could set any timing related flags/variables.  Right
> now I think going from 1 second to 0.01 seconds is quite a jump.
> 
> Can we put this as a devicemanagerADB level variable for sleep, we could set
> the default to 0.01, but if we have troubles, it could be reverted back or
> adjusted to a safe default level.

IMO, the only purpose of a sleep in a loop like this should be to prevent the process from taking up 100% CPU. From that perspective, a very short timeout should be fine. Is there something I'm missing here?

Comment 10

[William Lachance (:wlach)]

BTW, if these are going to go in, they should be committed to the mozbase repository (http://github.com/mozilla/mozbase) and then moved over to mozilla-central.

Comment 11

[Wesley Johnston (:wesj)]

Pull request:
https://github.com/mozilla/mozbase/pull/43

Comment 12

[Jeff Hammel]

Posting to https://bugzilla.mozilla.org/show_bug.cgi?id=807841 and https://github.com/mozilla/mozbase/pull/43 both for necessity and to illustrate a point.

As per https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Working_on_Mozbase_and_Contributing_Patches the current status quo is to not use pull requests.  One reason for this is that comments should be double posted in order that the issue can be followed coherently in both locations.  If we want a different policy, we should consider a different policy.  We should not subvert our own policy.

Is this issue closed or is it still active?

Comment 13

[Wesley Johnston (:wesj)]

Still active. Most of the improvement here comes from the sleep patch which I need to update.

Comment 14

[William Lachance (:wlach)]

(In reply to Jeff Hammel [:jhammel] from comment #12)
> Posting to https://bugzilla.mozilla.org/show_bug.cgi?id=807841 and
> https://github.com/mozilla/mozbase/pull/43 both for necessity and to
> illustrate a point.
> 
> As per
> https://wiki.mozilla.org/Auto-tools/Projects/
> MozBase#Working_on_Mozbase_and_Contributing_Patches the current status quo
> is to not use pull requests.  One reason for this is that comments should be
> double posted in order that the issue can be followed coherently in both
> locations.  If we want a different policy, we should consider a different
> policy.  We should not subvert our own policy.

I generally agree. In this particular case I figured that since the work had already been done, merging the pull request would be the easiest thing (and exactly equivalent to manually committing the patch as we usually do). I should have updated this bug when I did so though. Sorry about that.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch

Same sleep patch, but uses a variable. Is this what you meant?

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 678388 [details] [diff] [review]
Patch

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

thanks.

Comment 17

[William Lachance (:wlach)]

Pushed wesj's last patch. I changed "default_sleep" to "pollingInterval", as that's both more descriptive and in keeping with the style of hte module.

https://github.com/mozilla/mozbase/commit/fbc1a1d90d8da7496650184a21ec211073d6da1a

I'll be updating the version of mozdevice in m-c shortly so we can take advantage of these (and other) changes.