1896106 - Marionette's processhandler for psutil should not wait for reader thread when calling `wait()` from within the `poll()` method (causing a 1s extra delay when starting the session)

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

When running Marionette tests on MacOS these days it can be seen that there is a 1s delay between each and every test. After doing some investigation I can trace this back to using psutil. This feature was added over on bug 1884401 for Firefox 127.

Specifically the delay comes from calling this line in marionette.py:

returncode = self.instance.runner.returncode

And that then drills down to waiting for the reader thread to join in Marionette's own process handler for psutil:

https://searchfox.org/mozilla-central/rev/8e885f04a0a4ff6d64ea59741c10d9b8e45d9ff8/testing/marionette/client/marionette_driver/processhandler.py#352

After comparing to the mozprocess implementation I can see that this one doesn't call into wait() for poll(), which means that we never try to join the reader threads and that the poll() method returns immediately.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually the poll() method is not available when the process handler is observing an external process like Firefox after a restart. That means we have to continue using wait() but potentially should not wait for the reader thread to have joined.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1896106 - [marionette] Don't wait for reader thread when polling psutil process.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78ea7783f16f
[marionette] Don't wait for reader thread when polling psutil process. r=webdriver-reviewers,jgraham

Comment 4

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/78ea7783f16f

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:whimboo, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox127 to wontfix.

For more information, please visit BugBot documentation.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 9401088 [details]
Bug 1896106 - [marionette] Don't wait for reader thread when polling psutil process.

Beta/Release Uplift Approval Request

• User impact if declined: Users of Marionette on MacOS will expect a 1s delay at the beginning of each test that gets run, which can accumulate quite a lot of time
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Detecting if a process is running doesn't need to wait for the readers of stdout and stderr to wait. This patch just removed these parts and made it equal to what subprocess is running.
• String changes made/needed: No
• Is Android affected?: No

Comment 7

[Florin Bilt]

Perfherder has detected a awsy performance change from push 78ea7783f16f564aa8886dd8e51b42c2971c508b.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
10%	Images	macosx1015-64-shippable-qr	fission tp6	6,642,001.27 -> 5,979,633.87

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 221

For more information on performance sheriffing please see our FAQ.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP: Bug 1896106 - [marionette] Don't wait for reader thread when polling psutil process.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1896106 - [marionette] Don't wait for reader thread when polling psutil process.

Comment 10

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Users of Marionette on MacOS will expect a 1s delay at the beginning of each test that gets run, which can accumulate quite a lot of time
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: Low
• Explanation of risk level: Detecting if a process is running doesn't need to wait for the readers of stdout and stderr to wait. This patch just removed these parts and made it equal to what subprocess is running.
• String changes made/needed: No
• Is Android affected?: no

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/a548794aa51a