workloads/geekbench: Add support for Geekbench command-line build #1278

eliftopuz · 2025-02-12T14:57:31Z

Geekbench command-line build is added for Android. Geekbench apks allow to user to run the tests altogether. Using the command-line build, a single test or multiple tests can be specified.

metin-arm · 2025-02-12T15:07:55Z

wa/workloads/geekbench/__init__.py

@@ -20,10 +20,11 @@
 import json


Please follow Linux kernel coding guideline regarding the commit message.

Describe your changes in imperative mood, e.g. “make xyzzy do frotz” instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to do frotz”, as if you are giving orders to the codebase to change its behaviour.

I'd go for 2013-2025 to keep the same format, but yes ideally we should be updating the copyright year each time we update a file (although as you can see we are a little out of date...).

metin-arm · 2025-02-12T15:14:12Z

wa/workloads/geekbench/__init__.py

+    def init_resources(self, context):
+        # Necessary files to run the benchmark will be retrieved from WA as a tar file.
+        # WA will look for 'gb_cli_artifacts_<version>.tar' file to deploy them to the
+        # working directory. If there is no specified version, it will look for version 6.3.0 by default.


This line (and few more lines below) is too long (106 chars). Please ensure your changes are PEP-8 compliant. autopep8 command might be helpful.

I was using maxLineLength=120, I didn't see any rule for max length in the WA documentation. Also in the same file I can see that some lines > 100.

https://peps.python.org/pep-0008/#maximum-line-length says 79 & 72, but I think 100 is OK in general.

metin-arm · 2025-02-12T15:16:37Z

wa/workloads/geekbench/__init__.py

+            super(GeekbenchCmdline, self).initialize(context)
+            # Extract the tar file
+            self.target.execute("{} tar -xf {} -C {}".format(self.target.busybox,
+                                self.tar_file, self.target_working_directory))


This would be easier to maintain since we would not need to match spacing until (.

self.target.execute( "{} tar -xf {} -C {}".format( self.target.busybox, self.tar_file, self.target_working_directory))

metin-arm · 2025-02-12T15:17:36Z

wa/workloads/geekbench/__init__.py

@@ -20,10 +20,11 @@
 import json
 from collections import defaultdict

-from wa import ApkUiautoWorkload, Parameter
+from wa import Workload, ApkUiautoWorkload, Parameter


Workload is imported, but I'm unsure if it's used.

Workload is used from GeekbenchCmdline class

metin-arm · 2025-02-12T15:20:53Z

wa/workloads/geekbench/__init__.py

+
+    parameters = [
+        Parameter('cpumask', kind=str,
+                  default=None, override=False, mandatory=False,


Shouldn't default value be '' since the type is str?

metin-arm · 2025-02-12T15:35:39Z

wa/workloads/geekbench/__init__.py

+        if self.upload:
+            self.params += '--upload '
+        else:
+            self.params += '--no-upload '


self.params += '--upload ' if self.upload else '--no-upload ' would be slightly more Pythonic.

metin-arm · 2025-02-12T15:39:31Z

wa/workloads/geekbench/__init__.py

+        extension = os.path.splitext(self.output_file)[1][1:]
+        if self.output_file and extension not in self.allowed_extensions:
+            msg = 'No allowed extension specified. Allowed extensions: {}'
+            raise ValueError(msg.format(', '.join(self.allowed_extensions)))


Would this be more appropriate to build error message in a single place?

msg = f"No allowed extension specified. Allowed extensions: {', '.join(self.allowed_extensions)}" raise ValueError(msg)

metin-arm · 2025-02-12T15:41:21Z

wa/workloads/geekbench/__init__.py

+            for match in matches:
+                scores.append(int(re.search(r'\d+', match).group(0)))
+            if self.section == 4:
+                context.add_metric("OpenCL Score " + self.workloads[workload], scores[0])


It can also be Vulkan in addition to OpenCL, right?

OpenCL and Vulkan has different sections. I changed this part a bit. You can check the updated version.

metin-arm · 2025-02-12T15:42:12Z

wa/workloads/geekbench/__init__.py

+
+    def extract_results(self, context):
+        super(GeekbenchCmdline, self).extract_results(context)
+        # Extract results on the target


The comment should be swapped with the previous line?

Is this comment still pending?

No, I did change the comment with the previous line.

metin-arm · 2025-02-12T15:43:21Z

wa/workloads/geekbench/__init__.py

+
+    @once
+    def finalize(self, context):
+        # Clean up the working directory


You can drop this comment. It does not add much value.

marcbonnici · 2025-02-12T21:22:46Z

wa/workloads/geekbench/__init__.py

@@ -20,10 +20,11 @@
 import json


I'd go for 2013-2025 to keep the same format, but yes ideally we should be updating the copyright year each time we update a file (although as you can see we are a little out of date...).

marcbonnici · 2025-02-12T21:29:49Z

wa/workloads/geekbench/__init__.py

+
+    parameters = [
+        Parameter('cpumask', kind=str,
+                  default='', override=False, mandatory=False,


Unless we're setting override or mandatory we don't need to explicitly need to set these values for these Parameters.

I will remove those options.

marcbonnici · 2025-02-12T21:32:57Z

wa/workloads/geekbench/__init__.py

+        self.deployable_assets = [''.join(['gb_cli_artifacts', '_', self.version, '.tar'])]
+
+        # Create a working directory
+        self.target.working_directory = self.target.path.join(self.target.working_directory, f'gb_cli-{self.version}')


We shouldn't be trying to change the working directory of the target from a workload.
Is this meant to be self.target_working_directory?

I will change this part.
But I am still changing the working_directory in the agenda file when I am running the workload. Otherwise it gives me a permission error and cannot execute the binary.

Changing the working_directory from the agenda is fine as this would be a conscious decision by the user and would impact all workloads equally.

Otherwise it gives me a permission error and cannot execute the binary.

If these are binaries that require execution permission, is there a reason they need to be in the working directory? Or could we use target.install for this instead?

Hi Marc,
I did change this part and now I am extracting the assets in the executables_directory. Also there is no need to change working_directory in the agenda file.

marcbonnici · 2025-02-12T21:33:34Z

wa/workloads/geekbench/__init__.py

+
+        # Create a working directory
+        self.target.working_directory = self.target.path.join(self.target.working_directory, f'gb_cli-{self.version}')
+        self.target.execute("mkdir -p {}".format(self.target.working_directory))


Same as above.

marcbonnici · 2025-02-12T21:36:24Z

wa/workloads/geekbench/__init__.py

+    def validate(self):
+        super(GeekbenchCmdline, self).validate()


If we're not doing anything workload specific here do we need this?

I will remove that one.

scojac01 · 2025-02-13T09:24:14Z

Does Geekbench command line not require a license to run? Should we be upstreaming workloads that have such a dependency?

lukaszluba-arm · 2025-02-27T09:08:19Z

Hi Scott,

Yes, it needs a license.
This dependency is easy to solve and we can ask our partners to get their own license (internally in different teams - we don't have that issue).

On the other hand... Not having the GB6 cmd line version prevents us from being efficient in our experiments (and aligned across different teams that everyone can reproduce exactly the same scenario).

The APK from public store doesn't allow you to run single sub-test and be more precised how it should be run.
This pull request will be beneficial for a lot of use cases that we (and our partners) investigate.

scojac01 · 2025-02-27T09:16:39Z

Hi Scott,

Yes, it needs a license.

This dependency is easy to solve and we can ask our partners to get their own license (internally in different teams - we don't have that issue).

On the other hand... Not having the GB6 cmd line version prevents us from being efficient in our experiments (and aligned across different teams that everyone can reproduce exactly the same scenario).

The APK from public store doesn't allow you to run single sub-test and be more precised how it should be run. This pull request will be beneficial for a lot of use cases that we (and our partners) investigate.

If we agree that the dependency is for users of WA to sort themselves then that's fine. We have a geekbench cli workload that we have used for years and have not upstreamed because we were under the impression that such a thing was frowned upon.

marcbonnici · 2025-03-01T00:59:46Z

wa/workloads/geekbench/__init__.py

+    def run(self, context):
+        super(GeekbenchCmdline, self).run(context)
+        taskset = f"taskset {self.cpumask}" if self.cpumask else ""
+        cmd = '{} {}/{} {}'.format(taskset, self.target_exec_directory, self.binary_name, self.params)


We should use target.join() instead of hardcoding a /.

marcbonnici · 2025-03-01T01:12:26Z

wa/workloads/geekbench/__init__.py

+        super(GeekbenchCmdline, self).setup(context)
+
+        self.params = ''
+        if self.section == 4:


IIUC the 3 options for section are 1,4,9, is there a reason that we have 3 separate if statements all of which include the same self.params += '--section {} '.format(self.section) line?
Would it make more sense to only add the core explicitly for section 1?

marcbonnici · 2025-03-01T21:16:20Z

wa/workloads/geekbench/__init__.py

+
+    def extract_results(self, context):
+        super(GeekbenchCmdline, self).extract_results(context)
+        # Extract results on the target


Is this comment still pending?

marcbonnici · 2025-03-01T23:05:35Z

If we agree that the dependency is for users of WA to sort themselves then that's fine. We have a geekbench cli workload that we have used for years and have not upstreamed because we were under the impression that such a thing was frowned upon.

Hi @scojac01,

That's interesting, I had thought we had set precedence for this back with the geekbench-corporate workload, in that if we can assume that it is reasonable that a non-trivial amount of other users would be able to source the appropriate workloads, then it could still be beneficial to upstream the workload.

Are you aware of of substantial differences between the availability of the corporate vs the cli flavours of the workloads?

Add Geekbench command-line build workload for Android targets. Geekbench apks allow to user to run the tests altogether. Using the command-line, a single test or multiple tests can be specified. Signed-off-by: Elif Topuz <[email protected]>

scojac01 · 2025-03-03T13:41:31Z

If we agree that the dependency is for users of WA to sort themselves then that's fine. We have a geekbench cli workload that we have used for years and have not upstreamed because we were under the impression that such a thing was frowned upon.

Hi @scojac01,

That's interesting, I had thought we had set precedence for this back with the geekbench-corporate workload, in that if we can assume that it is reasonable that a non-trivial amount of other users would be able to source the appropriate workloads, then it could still be beneficial to upstream the workload.

Are you aware of of substantial differences between the availability of the corporate vs the cli flavours of the workloads?

Hi @marcbonnici That's fair enough. We must have missed that but happy with that precedent. The cli workload is the corporate one just triggered in a different method with a few extra params. The license should be the same so no impact there.

metin-arm reviewed Feb 12, 2025

View reviewed changes

eliftopuz force-pushed the master branch from 66db428 to 678f790 Compare February 12, 2025 19:30

marcbonnici reviewed Feb 12, 2025

View reviewed changes

eliftopuz force-pushed the master branch from 678f790 to c7378e6 Compare February 12, 2025 23:53

metin-arm approved these changes Feb 13, 2025

View reviewed changes

eliftopuz force-pushed the master branch from c7378e6 to 8752670 Compare February 25, 2025 19:55

marcbonnici reviewed Mar 1, 2025

View reviewed changes

eliftopuz force-pushed the master branch from 8752670 to c030620 Compare March 3, 2025 10:22

workloads/geekbench: Add support for Geekbench command-line build #1278

Are you sure you want to change the base?

workloads/geekbench: Add support for Geekbench command-line build #1278

Conversation

eliftopuz commented Feb 12, 2025

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

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

scojac01 commented Feb 13, 2025

lukaszluba-arm commented Feb 27, 2025

scojac01 commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcbonnici commented Mar 1, 2025

scojac01 commented Mar 3, 2025