Closed Bug 1055014 Opened 5 years ago Closed 5 years ago

Avoid pushing unnecessary files during b2g xpcshell setup


(Testing :: XPCShell Harness, defect)

Not set


(Not tracked)



(Reporter: chmanchester, Assigned: ahal)




(1 file, 3 obsolete files)

Each chunk of b2g emulator tests is pretty slow due to time spent pushing files. It looks like not all of them are relevant to the set of running tests.
I started by trying to push only the files necessary for an individual chunk, but this seems to save enough time on un-chunked runs to make the chunks unnecessary.


And with verbose mode on, so we can see what each file is doing (orange due to known failures in logs that usually aren't included):

I don't think this is a great solution, but it seems pretty effective (saves about 20 minutes per run). ahal, what do you think?
Attachment #8474566 - Flags: feedback?(ahalberstadt)
Assignee: nobody → cmanchester
Comment on attachment 8474566 [details] [diff] [review]
Bug Only push relevant files to device for xpcshell b2g tests.

Review of attachment 8474566 [details] [diff] [review]:

Huh, something regressed either in mozdevice or mozrunner ( I think. It used to be the case that pushing busybox to the emulator made the 'unzip' command available. When unzip is unavailable, mozdevice should automatically zip the contents of a directory up, push that one single file, and then unzip it again on the device side. This simple optimization saved us about ~20 minutes in setup for xpcshell in practice.

I think if we find where this regression happened and fix it, then this patch shouldn't be needed. If for some reason we can't fix it, then I guess it is better than nothing.. I'll take a look to see if I can figure out what happened.
Attachment #8474566 - Flags: feedback?(ahalberstadt) → feedback-
I think I know what's going on. Mozdevice calls a '_verifyZip' function when it is instantiated to test whether or not the device has 'unzip' capability. My theory is that ever since the mozrunner/ refactor, creates a second dm instance, before the one in mozrunner. When we install busybox via mozrunner, this instance has already called '_verifyZip' and therefore still thinks that the unzip command is unavailable.

We can fix this, by checking whether it is available in 'pushDir' itself. Here's a try run to test if this theory is correct:
My theory was at least partially true as dm._useZip was getting set to True in pushDir, but there seems to be an exception, at which point dm._useZip gets set back to False and we recursively call pushDir again:

This meant my patch caused infinite recursion. Here's a new try run that fixes that, and should also tell us why there is an exception in pushDir:
Wlach pointed out on irc that dm.pushDir no longer falls back to pushing one file at a time. So zipping things up is likely not even a perf win anymore. Either way we should always use dm.pushDir. I think the b2g 'setupTestDir' function was a hack to fix a problem that no longer exists.
Attachment #8474566 - Attachment is obsolete: true
Great, it looks like that gives us about the same speedup. Thanks ahal!
Assignee: cmanchester → nobody
Attachment #8474727 - Flags: review?(cmanchester)
Attachment #8474589 - Attachment is obsolete: true
Assignee: nobody → ahalberstadt
Comment on attachment 8474727 [details] [diff] [review]
Remove b2g specific setupTestDir function

Review of attachment 8474727 [details] [diff] [review]:

Thanks again for looking into this.
Attachment #8474727 - Flags: review?(cmanchester) → review+
I think this patch will fix the speed issue as well as not cause the intermittent (which I still have no idea why it is happening). Basically we were hitting an exception in the zip/unzip code path of pushDir 100% of the time (ever since we switched to using mozprocess).

I'll wait for some try retriggers before landing.
Attachment #8475316 - Flags: review?(wlachance)
Comment on attachment 8475316 [details] [diff] [review]
Fix exception in pushDir

What do you think about making return 'self', so that the original code works? Other than that, this looks reasonable, though I hope we don't have to keep the zip code around much longer.
Attachment #8475316 - Flags: review?(wlachance) → review+
(In reply to William Lachance (:wlach) from comment #11)
> What do you think about making return 'self', so that the
> original code works?

Yeah, I like that idea.
Try + retriggers are green and we still have the 20 minute speedup:
Attachment #8474727 - Attachment is obsolete: true
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.