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

Fixes #4466: CPU Usage performance metrics logging #4623

Merged
merged 74 commits into from
Nov 1, 2022

Conversation

Sarthak2601
Copy link
Contributor

@Sarthak2601 Sarthak2601 commented Sep 26, 2022

Explanation

Fixes #4466: CPU Usage performance metrics logging.

Introduces a CpuPerformanceSnapshotter that leverages the abilities of an actor to sequentially log cpu usage. Cpu usage is logged after a certain delay which is calculated on the basis of current Iconification (foreground/background) of the app. This iconification is updated every time the app changes state, i.e. goes from foreground to background or vice-versa.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @Sarthak2601. Sent my first batch of thoughts.

I think something else that would be really helpful here is including a detailed analysis in the opening comment demonstrating that this works as expected (e.g. by making the app simulate different types of loads on the system over different time periods to show that the computation is correct). I think we also need to make sure the solution works across multiple device images (which may require sharing a build with team members who have physical Android devices and can test the build in different configurations--you'll need to have a way to analyze the results).

I suspect the testing part can happen after this PR is merged (as part of verifying the dashboard changes), but we should at least verify it on a Google & non-Google physical device before merging.

@BenHenning BenHenning removed their assignment Sep 26, 2022
@oppiabot
Copy link

oppiabot bot commented Oct 5, 2022

Hi @Sarthak2601, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 5, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 7, 2022
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @Sarthak2601. Took a high-level pass; there are some things I'm not sure that I fully understand conceptually so PTAL at the comments. It'd be helpful to resolve these, then I can another conceptual pass before you finish up the rest of the PR.

@BenHenning BenHenning assigned Sarthak2601 and unassigned BenHenning Oct 11, 2022
@BenHenning
Copy link
Sponsor Member

Thanks @Sarthak2601. Left some more thoughts, + some in chat (regarding availableProcessors() and potential downsides to using it).

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @Sarthak2601! Just had a few comments (mostly nits and one functional clarification), but otherwise I think the tests look really good! The PR seems quite close to completion now.

@BenHenning BenHenning assigned Sarthak2601 and unassigned BenHenning Oct 29, 2022
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @Sarthak2601! Overall, the PR looks fantastic. I just had a few small follow-up comments.

@BenHenning BenHenning assigned Sarthak2601 and unassigned BenHenning Oct 31, 2022
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @Sarthak2601. Excellently done PR that completely LGTM. :)

@BenHenning
Copy link
Sponsor Member

Heads up that I think you'll need to sync to the latest develop branch before you can merge. Could you also update your latest require test to verify the exception message (since IllegalArgumentException is really generic)? No need to have me reapprove that specific change.

@BenHenning BenHenning assigned Sarthak2601 and unassigned BenHenning Nov 1, 2022
@oppiabot oppiabot bot added the PR: LGTM label Nov 1, 2022
@oppiabot
Copy link

oppiabot bot commented Nov 1, 2022

Hi @Sarthak2601, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@Sarthak2601
Copy link
Contributor Author

Heads up that I think you'll need to sync to the latest develop branch before you can merge. Could you also update your latest require test to verify the exception message (since IllegalArgumentException is really generic)? No need to have me reapprove that specific change.

Done with the change and synced to the latest develop.
Highly grateful for all the support @BenHenning!

@Sarthak2601 Sarthak2601 merged commit 929c142 into develop Nov 1, 2022
@Sarthak2601 Sarthak2601 deleted the performance-metrics-cpu-usage branch November 1, 2022 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce CPU Usage performance metrics logging
3 participants