Workloads: Add support for UIBench

[ptosi]

Add support for Android's UIBench (AOSP - frameworks/base/tests/UIBench) suite of tests as a WA workload.

Please note that this is based on ARM-software/devlib#357.

[ptosi]

I tried to keep my patch as light as possible. If required, I could also modify PackageHandler as described in the following comments.

[marcbonnici]

I think it would make sense to do your suggested changes to PackageHandler so that your solution can be utilised by future workloads rather than having to do it within the workload itself.

[jimbosjetset]

uibench might not be the most familiar workload to developers. Could we be more helpful by adding a short description of the workload and provide a url (https://android.googlesource.com/platform/frameworks/base.git/+/master/tests/UiBench/) for reference?

[ptosi]

What about the current version?

[ptosi]

Instead of having an approach where we manually write to an instance variable (.activity) before calling a method, I made activity a parameter of the method(s) concerned. IMO, it's cleaner, more robust and seems to fit pretty coherently with the purpose of the method(s).

[ptosi]

Will need to rebase for the fixup! commits and to re-order them (framework changes first, introduction of Uibench next).

[setrofim]

The methods are methods for a generic "workload" (or more generally job); Their parameters should not tie them to anything specific such as and Android activity. Please remove this chage.

[ptosi]

I have changed:

• ApkWorkload.setup()
• ApkUIWorkload.setup()
• ApkUIautoWorkload.setup()
• PackageHandler.setup()
• PackageHandler.start_activity()
• PackageHandler.restart_activity()

All of these methods deal exclusively with Android targets and their APKs. All APKs have activities:

Activities are one of the fundamental building blocks of apps on the Android platform. They serve as the entry point for a user's interaction with an app, and are also central to how a user navigates within an app or between apps.

Under these conditions, adding a feature dealing with an inherent part of any Android application to a set of classes dealing with Android applications seems reasonable; how is this change affecting the genericity of these classes or of their methods?

Please note that this PR does not modify Workload and does not compromise the polymorphism of the aforementioned classes when having to be used as generic Workloads, thanks to the use of default values for the activity parameters.

[setrofim]

The *Workload.setup() is a workload method and and should implement that signature. You're modifying it part way through the inheritance hierarchy to add another parameter; more over you remove it further down the hierarchy, so you now have

Workload.setup(self, context)
ApkWorkload.setup(self, context, activity=None)
UiBench.setup(self. context)

To me, this neither cleaner, nor more robust. And since the parameter that is being passed is an attribute of the object, and so is already available inside the method it is being passed to, I really don't see what problem this change is solving that justifies the inheritance hierarchy weirdness.

For PackageHandler the activity is part of its state, and all its methods operate on that one same activity. It's conceptually inconsistent to have some of them operate on one activity, and others on another, passed as a parameter. So again, I don't see how this is cleaner. In principle, I have no problem with moving the activity out of the PackageHandler state and having it consistently passed as a parameter (with no assumption being made inside PackageHandler that different methods are operating on the same activity), if that would result in cleaner overall design; however this not the change you're making.

[ptosi]

You are right about the ApkWorkload.setup-UiBench.setup inheritance; that's not clean and having UiBench.setup(self, context, activity=None) wouldn't be cleaner as the activity is passed through the agenda.

The "cleaner and more robust" comment referred to "calling a method with an argument" vs "calling a method without argument after having overwritten an instance variable controlling the behaviour of that method" which is what I was doing previously in this PR. I keep this point of view, however, it's now obvious that the latest change concerns more than just that.

In particular, the reason why I had to modify the setup methods is because the application is actually am started in that method (because ApkWorkload.setup calls PackageHandler.setup which calls start_activity). If, instead, ApkWorkload.run (currently a pass) was starting the application (self.apk.start_activity()), I could simply overload that method so that it passes the activity to the modified version of PackageHandler you describe (no self.activity is part of its internal state). What do you think of this approach? Why is the PackageHandler starting the application during setup (insn't there a mismatch between the meaning of a workload setup and PackageHandler.setup)?

For PackageHandler the activity is part of its state, and all its methods operate on that one same activity. It's conceptually inconsistent to have some of them operate on one activity, and others on another, passed as a parameter.

Except from (re)start_activity, which methods of PackageHandler are you referring to?

[setrofim]

The "cleaner and more robust" comment referred to "calling a method with an argument" vs "calling a method without argument after having overwritten an instance variable controlling the behaviour of that method" which is what I was doing previously in this PR.

You were modifying the state of an object form inside a method of that object; said state was then being used by another method of that object. Conceptually, there is nothing wrong with that -- that's what objects with methods are for (vs just having functions where the entire state is passed as arguments). In practice I agree that the old solution was not the most elegant. However, I still prefer it over this one -- at least, in that case, the cruftiness was contained within one workload.

The issue here is that our model for how we run apk workloads does not fit particularly well how this workload needs to be run. If this is a one-off, I am happy to live with a bit of cruft, as long as it is contained with this workload. If this is a wider issue with the model, then perhaps it needs to be reexamined and wider, more systematic, changes made. However I'd rather avoid making quick changes to the framework that weaken its conceptual integrity for the sake of making implementation of one workload a little cleaner.

In particular, the reason why I had to modify the setup methods is because the application is actually am started in that method (because ApkWorkload.setup calls PackageHandler.setup which calls start_activity). If, instead, ApkWorkload.run (currently a pass) was starting the application (self.apk.start_activity()), I could simply overload that method so that it passes the activity to the modified version of PackageHandler you describe (no self.activity is part of its internal state). What do you think of this approach? Why is the PackageHandler starting the application during setup (insn't there a mismatch between the meaning of a workload setup and PackageHandler.setup)?

I'm not sure I'm following the reasoning here. There is no mismatch in the meaning of setup for Workload and PackageHandler. run is the method that executes the part of the workload that we actually want to measure. The job of the setup method of Workload is to get the workload to that point. start_activity launches the application. For the vast majority of Android applications, we do not want to start measuring yet as the actual "workload" is not running yet -- there may be some splash screen to click through, some menus to navigate, some GUI settings to twiddle, etc. This is all done as part of the setup up to that last "click" that actually runs the "interesting" portion of the application. Hence the app is am start'd and the initial menus / gui settings done as part of setup, and run contains the actual execution of the app. For the few workloads that do run immediately on launch, we typically have not cared if we missed the first few seconds of execution.

Except from (re)start_activity, which methods of PackageHandler are you referring to?

Just those two. My point was that you could, e.g. call start_activity without the argument, starting the activity which is part of PackageHandler's state, and then call restart_activity with an argument, restarting an activity that is different from what was started previously. Basically, the activity these methods operate on should either always be passed as a (mandatory) argument, or should always be obtained from the object's state.

[ptosi]

The issue here is that our model for how we run apk workloads does not fit particularly well how this workload needs to be run. If this is a one-off, I am happy to live with a bit of cruft, as long as it is contained with this workload.

As stated in the quote from the official Android documentation I post above, activities are an integral and important part of Android applications. For this reason, ApkWorkload (and, by extension, any class concerned with starting an Android application such as PackageHandler) should support them, IMO.

The ways I was trying to patch this support into the code were arguably hacky, I agree with that. However, this is not a one-off to satisfy the exotic requirements of a workload! To illustrate this, this PR is also not the first one facing this limitation of the framework e.g. #720 had the same requirements.

For the vast majority of Android applications, we do not want to start measuring yet as the actual "workload" is not running yet -- there may be some splash screen to click through, some menus to navigate, some GUI settings to twiddle, etc.

This is exactly why this workload for UIBench uses activities: if you try launching the app (which effectively means starting in the default activity), you will be presented with a menu of all the "tests" the suite provides. However, the application provides an activity (entrypoint) for each one of these tests! Isn't it cleaner to ask the app to start a given test directly, by passing the name of the corresponding activity to am start than to start the app (opening the menu), launch an automation (after having had to implement it) that finds the right entry in the menu and simulate a screen input? By using activities as they were intended to be used, we completely get rid of the need to automate the menu of UIBench!

Basically, the activity these PackageHandler methods operate on should either always be passed as a (mandatory) argument, or should always be obtained from the object's state.

I had considered passing the activity to the constructor but then, we're unable to validate the passed activity by comparing it with the list of available activities extracted from the APK, .apk_info.activities as apk_info is not available until initialize. But now that I think of it, it's probably cleaner to do this and delay the check until then or even until right before the call to am start(?)

I'm not sure I'm following the reasoning here.

I had misinterpreted "run" as "run this [command/application]" (as in subprocess.run) as opposed to "this is the run we capture". Indeed, in most cases, it makes sense to have the applauch take place during the setup phase. The point I was trying to make is what I've explained above: I believe that the framework should support activities and I've struggled to neatly integrate them for the reasons mentioned above.

[setrofim]

To illustrate this, this PR is also not the first one facing this limitation of the framework e.g. #720 had the same requirements.

The issue highlighted that PR has already been addressed (pretty much in the way that the PR was solving it) by 753786a.

By using activities as they were intended to be used, we completely get rid of the need to automate the menu of UIBench!

Yes, I agree it would be better to use the activity directly, and that's what we've done in the past for the workloads that supported this. But so far, we could get way with launching them from setup. The issue, as I understand it, is not specifying which activity to launch (which is already supported); but having it launched as part of run rather than setup. Or am I misunderstanding the problem?

I had misinterpreted "run" as "run this [command/application]" (as in subprocess.run) as opposed to "this is the run we capture".

FWIW, we do document the intended functionality of each method here: https://workload-automation.readthedocs.io/en/latest/api/workload.html

[ptosi]

Rebased as required to use the ApkWorkload.activity field which was merged after opening this PR.

[marcbonnici]

Would it be worth adding in validation of the activity using the functionality that was added in ARM-software/devlib#357?

[ptosi]

Would it be worth adding in validation of the activity using the functionality that was added in ARM-software/devlib#357?

Yes and initially (before I rebased it), it was done in this PR.

The reason why I haven't added it back is that I believe this test should be added in the PackageHandler right before the call to am start. However, because of how ARM-software/devlib#357 was implemented (partially due to a lack of knowledge about APKs and Android apps on my side), I think it might only be capturing activities that are in the namespace of the main application (notice the . in the activity regex) which doesn't necessarily include all activities.

Therefore, a test such as if activity in self.apk_info.activities would break for workloads that provide activities in other namespaces (or even absolute names) and I believe I have seen such workloads in WA.

My idea was to investigate that and open another PR to fix the activity regex so that we can add the check when running a given activity. This check isn't required for the functionality that this PR attempts to introduce, though. In particular as it's not present in the current master so that some workloads are running "untested" activities.

IMO, the main advantage of that check (and the reason why I had implemented it in the first place) is that the error message could then state which activities are accepted (as opposed to the current error message coming back from the target, simply stating that the activity is invalid).

[marcbonnici]

Ok that seems fair enough, in that case happy to merge this as is until the best way to perform the validation has been decided.