Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UiBench: Support multiple activities #1187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qais-yousef
Copy link
Contributor

We must specify a single activity for each possible uibench activity to
run, which is inconvenient if we want to batch run them in one go.

This is an attempt to allow some flexibility, but I realize this could
be affecting more users and while I did some auditing and did test with
various workloads (geekbench, gfxbench, pcmark) and nothing broke, but
more coverage is required still.

Major question is whether such change makes sense and whether the
approach I've taken is in the right direction?

Signed-off-by: Qais Yousef [email protected]

@qais-yousef
Copy link
Contributor Author

FWIW, this allows me to specify something like this in the agenda

activity: 'BitmapUploadActivity RenderingJitter TrivialAnimationActivity'

@marcbonnici
Copy link
Contributor

Hi, This is an interesting idea, however is this functionality you're trying to achieve specific to the uibench workload? AFAIK for most other application we'd still only want a single activity to be used?
If so would it make more sense to contain this functionality to the uibenchmark workload itself, rather than modify the parent classes?

@qais-yousef
Copy link
Contributor Author

Hi, This is an interesting idea, however is this functionality you're trying to achieve specific to the uibench workload? AFAIK for most other application we'd still only want a single activity to be used? If so would it make more sense to contain this functionality to the uibenchmark workload itself, rather than modify the parent classes?

Bad judgement from my side then. I thought better do it generically to benefit any new potential new user. Putting it in uibench only works for me. Let me go figure that and update the PR. It will certainly make testing easier :-) Thanks!

It is desired to be able to run multiple activities with a single agenda
file. We can make the @activity parameter take a list of activities
separated by white space and run them all in one go. Each activity well
run for the specified @duration.

Signed-off-by: Qais Yousef <[email protected]>
@qais-yousef qais-yousef changed the title [WIP] ApkWorkload: Support multiple activities UiBench: Support multiple activities Apr 15, 2022
@qais-yousef
Copy link
Contributor Author

I do everything in init.py for UiBench. It seems everything happens from the setup() function for UiBench. One tricky bit is that I need to modify self.apk._activity in setup. Not pretty, so happy to hear suggestions on better ways to handle this. Shall we introduce a function to allow the override of activity? As I write this I see pylint is complaining about it too.

Copy link
Contributor

@marcbonnici marcbonnici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems everything happens from the setup() function for UiBench. One tricky bit is that I need to modify self.apk._activity in setup.

I'm thinking if we have multiple activities provided then we could let setup just launch the application via the APKHandler with no activity provided and then launch each activity in the run phase of the workload to allow for instrumentation to measure the run as expected. I think perhaps as we only have a single apk here it might be easiest to generate the activity launch command in the workload similar to jankbench.
Does that sounds like it would fit your use case?

Comment on lines +42 to +43
You can provide multiple activities to run by separating them
with white space.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update this to rather than a space separated string use the list_or_string kind for this parameter? That way we will preserve the existing behavior but also allow the use of proper lists to separate the activities?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Comment on lines +69 to +72
self.apk._activity = activity
self.logger.info("Starting {} for {} seconds".format(activity, self.duration))
super(Uibench, self).setup(context)
self.target.sleep(self.duration)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By launching the activities here does this form the main part of the workload? In which case we'll want to do the in the run method to allow instrumentation to measure the device during the "active" part of the workload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has always been like this. But the difference now is that we sleep in the setup instead of run, which is your point. Let me try to follow on with your suggestion which should address this too. Thanks.

@qais-yousef
Copy link
Contributor Author

It seems everything happens from the setup() function for UiBench. One tricky bit is that I need to modify self.apk._activity in setup.

I'm thinking if we have multiple activities provided then we could let setup just launch the application via the APKHandler with no activity provided and then launch each activity in the run phase of the workload to allow for instrumentation to measure the run as expected. I think perhaps as we only have a single apk here it might be easiest to generate the activity launch command in the workload similar to jankbench. Does that sounds like it would fit your use case?

I think I follow what you say and yes it makes sense :). But I'll need to do some homework to understand it fully and come up with an updated patch. It doesn't sound like a lot of work. Thanks for the pointers.

@marcbonnici marcbonnici added the WIP Work in Progress label Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants