workloads/speedometer: offline version of speedometer #1122

stephenkyle-ARM · 2020-07-15T13:57:51Z

This version replaces the previous uiauto version of Speedometer with a new version.

Supports both chrome and chromium again, this is selected with the
chrome_package parameter.
No longer needs internet access.
Version 1.0 of Speedometer is no longer supported.

setrofim · 2020-07-15T14:07:39Z

wa/workloads/speedometer/__init__.py

+
+        # Host a copy of Speedometer locally
+        subprocess.check_output(
+            "tar -xf {} -C {}".format(self.archive_asset, context.output_directory),


use Python's tarfile do not assume that tar is present on the host system.

setrofim · 2020-07-15T14:08:21Z

wa/workloads/speedometer/__init__.py

+            shell=True,
+        )
+
+        webserver_cmd = "cd {} && python3 -mhttp.server {}".format(


Do not not do this. Import http.server and start it in a thread.

setrofim · 2020-07-15T14:08:57Z

wa/workloads/speedometer/__init__.py

+            self.document_root, self.webserver_port
+        )
+
+        with open(os.devnull, "w") as dev_null:


Don't need to do this, just use stdout=None

setrofim · 2020-07-15T14:10:56Z

wa/workloads/speedometer/__init__.py

+            )
+
+        subprocess.check_output(
+            "adb reverse tcp:{0} tcp:{0}".format(self.webserver_port), shell=True


Do not use adb directly -- all interactions must go through devlib. Add an appropriate utility there. Or at least used devlib's adb_command.

setrofim · 2020-07-15T14:12:22Z

wa/workloads/speedometer/__init__.py

-    known working chrome version 80.0.3987.149
-    '''
+    known working chrome version 83.0.4103.106
+    """


and supported_platforms=['android'], as this is android-specific.

setrofim · 2020-07-15T14:18:35Z

wa/workloads/speedometer/__init__.py

+        self.target.execute(browser_launch_cmd)
+
+        # Wait 60 seconds at least, and then wait until we don't see the 'sandboxed_process' process for 10 seconds.
+        time.sleep(60)


Why sixty? You should avoid long sleeps if at all possible. Can you not poll running packages or logcat instead?

The idea is to reduce the amount of polling we do to the system while it's supposed to be running a performance-sensitive benchmark. Since we inevitably are going to need to poll in some way to work out which it has finished, a compromise was to wait at least 60 seconds - I know Speedometer usually takes around 90 seconds on current devices. Is there something intrinsically wrong with sleeping for this long? What does it affect?

Long sleeps introduce unnecessary delays, this can end up significantly increasing the over all time. Yo're also making assumptions about how long things take which may not be valid when run on a system that is vastly different from the system on which the workload is first implemented. Fixed waits are brittle.

This is why, as a rule, we generally prefer some form of polling or signalling guarded by generous timeouts. A slow low-overhead poll (e.g. with cadence of 1-2s) will typically not impact most workloads significantly enough to affect results -- given the level of backgrounds activity on a typical rich-OS system (especially Android), it's usually in the noise.

setrofim · 2020-07-15T14:19:53Z

wa/workloads/speedometer/__init__.py

+
+        countdown = 5
+        while countdown > 0:
+            busiest_line = self.target.execute("top -n1 -m1 -q -b").split("\n")[0]


Use the busybox top instead; do not rely on the top on target being present and accepting those parameters.

setrofim · 2020-07-15T14:22:39Z

wa/workloads/speedometer/__init__.py

+        while countdown > 0:
+            busiest_line = self.target.execute("top -n1 -m1 -q -b").split("\n")[0]
+            while "sandboxed_process" in busiest_line:
+                countdown = 5


I don't understand the logic of setting the countdown inside the inner loop here. And this inner loop doesn't seem to have a break condition in case "standboxed_process" never goes away.

This whole nested loop method seems unnecessarily convoluted. Can this be simplified/done via a different method? At the very least, leave a comment describing wtf is going on here.

I've liberally commented this now. :) If 'sandboxed_process' never stops being the hottest process in the system (something my comments/changes will hopefully make clearer is what I'm looking for), then we should indeed not break out of the inner loop, because the benchmark is still running.

setrofim · 2020-07-15T14:24:20Z

wa/workloads/speedometer/__init__.py

+    def teardown(self, context):
+        super(Speedometer, self).teardown(context)
+        subprocess.check_output(
+            "adb reverse --remove tcp:{}".format(self.webserver_port), shell=True


Again, adb should not be invoked directly -- do not assume it is in path. Should be done via adb_command from devlib utils.

setrofim · 2020-07-15T14:26:49Z

wa/workloads/speedometer/__init__.py

+
+        self.webserver.terminate()
+        # We have to kill the child python3 process independently.
+        webserver_cmdline = ["python3", "-mhttp.server", str(self.webserver_port)]


Again, do not invoke another python process via shell -- import the module and run in a thread.

marcbonnici · 2020-07-15T14:00:12Z

wa/workloads/speedometer/__init__.py

+        self.target.execute("uiautomator dump", as_root=True)
+        self.target.pull(
+            "/sdcard/window_dump.xml", context.output_directory, as_root=True
+        )


Can this use the working directory on the device rather than a hardcoded path?

marcbonnici · 2020-07-15T17:17:32Z

wa/workloads/speedometer/__init__.py

+        # Pull the preferences file from the device, modify it, and push it back.
+        # This is done to bypass the 'first launch' screen of the browser.


Just a thought, would it have been more robust / remove the root requirement to still use uiautomator for performing this part the setup stage? If I remember correctly as long as it had exited before the run stage started it didn't impact the score?

As I recall, the 'uiautomator dump' would only actually see the score when executed as root anyway, so I think we need to require root always.

stephenkyle-ARM · 2020-07-16T15:49:16Z

Hopefully that's all changes addressed.

On the root requirement side, perhaps it is possible to use UIAutomator again to replace the (pm-clear, launch the browser, kill it, modify the shared_prefs file) section for non-rooted devices. Is it easy enough to have UIAutomator be launched in this way just for a portion of the workload? i.e., I imagine it would be more complex than switching back to subclassing UIAutomatorApkWorkload again? On a second look, I noticed that I was now able to use uiautomator dump without as_root. I'm not sure why it hadn't worked in the past. Perhaps this is something that could be investigated later and added back in as a separate PR?

PTAL.

wa/workloads/speedometer/__init__.py

stephenkyle-ARM · 2020-07-22T08:59:35Z

Updated with the following changes:

no longer relies on old devlib behaviour of preserving ownership of a file that is overwritten with adb push
addressing Marc's feedback, the workload complains if it is unable to find any process matching the search term it uses to find the active render process. This term is normally <chrome package>:sandbox. A user can configure this search term by setting 'render_process_search_term' in the workload parameters.
we go back to executing uiautomator dump as root - I was observing it randomly not containing the score in it again (despite it clearly being on the screen).
make sure we only delete temporary files that exist during teardown, stopping crashes during recovery from a bad run.
force-stop the browser when the workload is over, to prevent it persisting during the execution of other workloads (the speedometer render process has been seen at times to consume at least 3% CPU when another browser workload is running, for example)

PTAL!

wa/workloads/speedometer/__init__.py

marcbonnici · 2020-07-22T18:28:04Z

wa/workloads/speedometer/__init__.py

+        if self.render_process_search_term is not None:
+            search_term = self.render_process_search_term
+        render_processes = [x for x in _get_all_processes() if search_term in x]
+        if len(render_processes) == 0:


pylint nit: "Do not use len(SEQUENCE) to determine if a sequence is empty (len-as-condition)"

I think this is actually a bug in the version of pylint you're using. It is designed to catch use of len(X) as the sole condition in an if statement, when len(X) == 0 is perfectly OK, and in my opinion, more readable as its intent is more explicit. This particular version of pylint (assuming you're using 1.9.2) rejects this line, but later versions do not - I didn't see this initially when I installed the latest version of pylint through pip (2.5.3). See pylint-dev/pylint#2684 for the issue which led to pylint being fixed in light of this.

Hmm I see. Yes you're correct, we're still using v1.9.2 as at the time we hit a bug in the later version but we could probably do with an upgrade by now.

setrofim

So the whole method my which workload termination is detected seem very brittle and hacky. Since you're modifying the workload anyway, and therefore are not limited by the upstream implementation, have you investigated a possibility of adding a more reliable signal that the workload is done (e.g. creating a particular file, or creating a message dialog)?

wa/workloads/speedometer/HOWTO-create-the-archive

wa/workloads/speedometer/__init__.py

setrofim · 2020-07-23T10:10:19Z

wa/workloads/speedometer/__init__.py

+        while countdown > 0:
+            busiest_line = self.target.execute("top -n1 -m1 -q -b").split("\n")[0]
+            while "sandboxed_process" in busiest_line:
+                countdown = 5


stephenkyle-ARM · 2020-07-27T18:35:42Z

Rewritten to check for a value written to the browser's LocalStorage, which we can find on the file system as root. This value indicates the benchmark is complete.

setrofim · 2020-07-30T10:09:53Z

wa/workloads/speedometer/__init__.py

-        self.gui.timeout = 1500
-        self.gui.uiauto_params['version'] = self.speedometer_version
+        self.archive_server = ArchiveServer()
+        if not target.is_rooted:


This should be done inside initialize() not __init__ -- do not rely on target connection existing when the workload object is instantiated.

setrofim · 2020-07-30T10:10:39Z

wa/workloads/speedometer/__init__.py

+        self.document_root = os.path.join(context.output_directory, "document_root")
+
+        # Host a copy of Speedometer locally
+        tarball = os.path.join(os.path.dirname(__file__), "speedometer_archive.tgz")


Use WA's resource discovery framework rather than hard-coding the location. This should also be done inside intialize() rather than setup() as it only needs to be done once per WA run, not for each iteration of the workload.

setrofim · 2020-07-30T10:12:20Z

wa/workloads/speedometer/__init__.py

+        # The browser's processes can stick around and have minor impact on
+        # other performance sensitive workloads, so make sure we clean up.
+        self.target.execute("am force-stop {}".format(self.chrome_package))
+


Add a finalize() to clean up the workload files deployed to the target if self.cleanup_assets is True

stephenkyle-ARM · 2020-08-27T15:52:24Z

Okay, changes addressed, PTAL.

stephenkyle-ARM · 2020-09-09T13:33:58Z

Friendly request for another review? :) @marcbonnici @setrofim ?

setrofim

Sorry for the delay in looking at this.

marcbonnici · 2020-08-27T16:19:07Z

wa/workloads/speedometer/__init__.py

+        local_storage_seen = False
+        benchmark_complete = False
+        while not benchmark_complete:
+            if self.target.file_exists(local_storage):


Is it likely that this directory will stop existing after it is created? Otherwise having this checked unconditionally inside the loop generates a lot of extra calls.

marcbonnici · 2020-08-27T16:24:33Z

wa/workloads/speedometer/__init__.py

+    supported_platforms = ["android"]
+
+    package_names = ["org.chromium.chrome", "com.android.chrome"]
+    regex = re.compile('text="(\d+.\d+)" resource-id="result-number"')


This fails to match on my device: the score is in content-des rather than text

<node index="3" text="" resource-id="result-number" class="android.view.View" package="com.android.chrome" content-desc="7.399" checkable="false" checked="false" clickable="false" enabled="true" focusable="false" focused="false" scrollable="false" long-clickable="false" password="false" selected="false" bounds="[99,969][982,1130]" />

I have created a new regex that will capture text or content-desc, and hopefully if the attribute orderings were to change randomly it should be protected against that too!

marcbonnici · 2020-08-27T16:28:01Z

wa/workloads/speedometer/__init__.py

-        else:
-            raise WorkloadError("The Speedometer workload has failed. No score was obtainable.")
+
+        self.ui_dump_loc = os.path.join(self.target.working_directory, "ui_dump.xml")


If there is an error during execution this may never get defined but may still get accessed in teardown. Can this be declared earlier and checked if set during teardown?

marcbonnici · 2020-08-27T16:29:14Z

wa/workloads/speedometer/__init__.py

+        iterations = 0
+        while not score_read:
+            score = self.read_score()
+
+            if score is not None:
+                context.add_metric(
+                    "Speedometer Score", score, "Runs per minute", lower_is_better=False
+                )
+                score_read = True
+            else:
+                if iterations >= 10:
+                    raise WorkloadError(
+                        "The Speedometer workload has failed. No score was obtainable."
+                    )
+                else:
+                    # Sleep and retry...
+                    time.sleep(2)
+                    iterations += 1


Out of curiosity why does this need so many attempts to detect the score? Is the output not stable?

Yep, from testing a number of devices in our farm, the uiautomator dump command seems to fail to capture the score in the first one or two runs on a number of occasions - it doesn't seem to be fixed by running as root, and it varied from device to device. Adding this iterative check helped. Perhaps we only need 5 iterations, but taking 20s didn't seem too bad to make an attempt to read the score correctly before we give up.

marcbonnici · 2020-08-27T16:33:17Z

wa/workloads/speedometer/__init__.py

+                    output = self.target.execute(
+                        grep_cmd, as_root=True, check_exit_code=False
+                    )
+                    if "Binary file {} matches" in output or report_end_id in output:


Missing a .format() here?

Removed this anyway.

marcbonnici · 2020-08-27T16:33:42Z

wa/workloads/speedometer/__init__.py

+                local_storage_seen = True
+
+                for ls_file in candidate_files:
+                    # Each local storage file is in a binary format. Depending on the grep you use, it


If this is using the busybox implementation should the behavior not be consistent?

marcbonnici · 2020-09-09T15:24:02Z

wa/workloads/speedometer/__init__.py

+        adb_command(target.adb_name, "reverse tcp:{0} tcp:{0}".format(self._port))
+
+    def stop(self, target):
+        adb_command(target.adb_name, "reverse --remove tcp:{}".format(self._port))


These fail when using a remote adb_server. I can't think how we could get around that using this approach at the moment so could we add a warning / error that this is not supported in the workloads initalize method? The current failure state is a non-obvious error.

As discussed, it seems this is an issue with devices before Android 9 when connected via TCP. Added documentation to note which Android versions are supported (9+ via TCP, 5+ via USB).

Unfortunately I do not have a sufficiently up to date target available to test this currently. However if that is the issue these calls will still require the adb_server providing to adb_command. With that parameter added are you able to verify it still works on your end?

What is adb_server set to? Sorry I'm not familiar with the usage here - is that something that can be found in self, i.e. adb_server=self.adb_server if you've set an ADB server in your config?

Ah sorry, it would set similar to they way you retrieve the adb_name ie. adb_server=target.adb_server as it would be a part of the supplied device config.

As discussed elsewhere, this isn't going to work without some kind of extra port forwarding being setup between the machine running WA and the machine running the remote ADB server - so we detect if adb_server is non-None in target, and refuse to run the workload in this case, saying it isn't supported.

marcbonnici

Can you clean up the indentation to keep pep8 happy?
https://travis-ci.org/github/ARM-software/workload-automation/jobs/726261564#L3003

marcbonnici · 2020-09-16T08:52:08Z

wa/workloads/speedometer/__init__.py

+
+        # Temporary directory used for storing the Speedometer files, uiautomator
+        # dumps, and modified XML chrome config files.
+        self.temp_dir = tempfile.TemporaryDirectory()


~~This directory should be cleaned up in finalize.~~

This version replaces the previous uiauto version of Speedometer with a new version. * Supports both chrome and chromium again, this is selected with the chrome_package parameter. * No longer needs internet access. * Version 1.0 of Speedometer is no longer supported. * Requires root: - sometimes uiautomator dump doesn't capture the score if not run as root - need to modify the browser's XML preferences file to bypass T&C acceptance screen

setrofim requested changes Jul 15, 2020

View reviewed changes

marcbonnici reviewed Jul 15, 2020

View reviewed changes

stephenkyle-ARM force-pushed the master branch from 5f91438 to 40ce4b1 Compare July 16, 2020 15:38

marcbonnici reviewed Jul 20, 2020

View reviewed changes

wa/workloads/speedometer/__init__.py Outdated Show resolved Hide resolved

stephenkyle-ARM force-pushed the master branch from 40ce4b1 to d5e448b Compare July 22, 2020 08:53

marcbonnici reviewed Jul 22, 2020

View reviewed changes

wa/workloads/speedometer/__init__.py Outdated Show resolved Hide resolved

stephenkyle-ARM force-pushed the master branch from d5e448b to f4c3f72 Compare July 22, 2020 10:45

marcbonnici reviewed Jul 22, 2020

View reviewed changes

stephenkyle-ARM force-pushed the master branch from f4c3f72 to ee65ba9 Compare July 23, 2020 09:36

setrofim requested changes Jul 23, 2020

View reviewed changes

stephenkyle-ARM force-pushed the master branch from ee65ba9 to 9345e0c Compare July 27, 2020 18:34

stephenkyle-ARM force-pushed the master branch from 9345e0c to c9b4b2f Compare July 29, 2020 17:45

setrofim requested changes Jul 30, 2020

View reviewed changes

stephenkyle-ARM force-pushed the master branch 2 times, most recently from 70c52c0 to c2e6a15 Compare August 27, 2020 15:44

stephenkyle-ARM requested a review from setrofim August 27, 2020 15:52

setrofim approved these changes Sep 9, 2020

View reviewed changes

marcbonnici requested changes Sep 9, 2020

View reviewed changes

stephenkyle-ARM force-pushed the master branch from c2e6a15 to 70785cf Compare September 10, 2020 15:19

stephenkyle-ARM requested a review from marcbonnici September 10, 2020 15:23

stephenkyle-ARM force-pushed the master branch 2 times, most recently from 6ff5be9 to 4ca1e2f Compare September 11, 2020 12:43

marcbonnici reviewed Sep 16, 2020

View reviewed changes

stephenkyle-ARM force-pushed the master branch from 4ca1e2f to 5f25ef8 Compare September 16, 2020 09:46

stephenkyle-ARM force-pushed the master branch from 5f25ef8 to ed3c954 Compare September 16, 2020 09:55

marcbonnici approved these changes Sep 16, 2020

View reviewed changes

marcbonnici merged commit 7489b48 into ARM-software:master Sep 16, 2020

		# Pull the preferences file from the device, modify it, and push it back.
		# This is done to bypass the 'first launch' screen of the browser.

workloads/speedometer: offline version of speedometer #1122

workloads/speedometer: offline version of speedometer #1122

Conversation

stephenkyle-ARM commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenkyle-ARM commented Jul 16, 2020 • edited Loading

stephenkyle-ARM commented Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

setrofim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenkyle-ARM commented Jul 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenkyle-ARM commented Aug 27, 2020

stephenkyle-ARM commented Sep 9, 2020

setrofim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcbonnici Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcbonnici left a comment

Choose a reason for hiding this comment

marcbonnici Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

stephenkyle-ARM commented Jul 16, 2020 •

edited

Loading

stephenkyle-ARM commented Jul 22, 2020 •

edited

Loading

marcbonnici Aug 27, 2020 •

edited

Loading

marcbonnici Sep 16, 2020 •

edited

Loading