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

RUMM-1278 Screen Refresh Rate vitals #522

Merged
merged 4 commits into from
Jun 29, 2021

Conversation

buranmert
Copy link
Contributor

@buranmert buranmert commented Jun 17, 2021

What and why?

We will be reading the refresh rate during app's lifetime in order to detect UI hitches.

How?

iOS renders UI in the main run loop, this is a RunLoop instance. We want to be notified when a new frame is drawn.
CADisplayLink is a special timer type which listens to frame paints in a given run loop.
We add a CADisplayLink instance to RunLoop.main so that it calls our method every time a new frame is drawn.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@buranmert buranmert self-assigned this Jun 17, 2021
@buranmert buranmert force-pushed the buranmert/RUMM-1278-refresh-rate-vital branch 2 times, most recently from 5d0c775 to 3be6499 Compare June 23, 2021 16:51
@buranmert buranmert marked this pull request as ready for review June 23, 2021 16:54
@buranmert buranmert requested a review from a team as a code owner June 23, 2021 16:54
@buranmert buranmert force-pushed the buranmert/RUMM-1278-refresh-rate-vital branch 3 times, most recently from 5c0916d to 6772478 Compare June 23, 2021 17:29
@buranmert buranmert force-pushed the buranmert/RUMM-1278-refresh-rate-vital branch from 6772478 to 67f4237 Compare June 23, 2021 17:44
@buranmert buranmert force-pushed the buranmert/RUMM-1278-refresh-rate-vital branch from 8bf3f03 to c7f0310 Compare June 24, 2021 10:37
@buranmert buranmert requested a review from ncreated June 24, 2021 12:14
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

I left thread synchronization question as IMO there is an unnecessary penalty on the main thread and I don't clearly see how we will further read these value form secondary thread in RUM scope.

@xgouchet how do you synchronize publishing this vital from main thread and reading it from RUM scope thread on Android?

Sources/Datadog/RUM/RUMVitals/VitalRefreshRateReader.swift Outdated Show resolved Hide resolved
@xgouchet
Copy link
Member

I left thread synchronization question as IMO there is an unnecessary penalty on the main thread and I don't clearly see how we will further read these value form secondary thread in RUM scope.

@xgouchet how do you synchronize publishing this vital from main thread and reading it from RUM scope thread on Android?

On Android the RUM View Scope doesn't read from it. What happens is the view gets notified when a new aggregation is available and stores its value locally, and when it sends the event it sends it with the latest local value it knows

After blocking UI thread, we must wait a bit
in order for the reader to read once again before assertions
@buranmert buranmert requested a review from ncreated June 28, 2021 13:07
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Looks very good 👌

@buranmert buranmert merged commit 5e3fb26 into master Jun 29, 2021
@buranmert buranmert deleted the buranmert/RUMM-1278-refresh-rate-vital branch June 29, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants