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

Reduce CPU load by reducing view update frequency #231

Merged
merged 34 commits into from
Nov 8, 2017

Conversation

mattrubin
Copy link
Owner

@mattrubin mattrubin commented Nov 7, 2017

Based on the excellent work by @beaucollins in PR #224. My changes on top of that PR can be seen here.


Currently, the app animates the progress ring and keeps token passwords up-to-date by using CADisplayLink to regenerate the view model and update the view at 60 frames per second. This is needlessly inefficient, and causes excessive processor load. Instead of updating the view every frame, it is sufficient to generate a new view model only when a token password has updated, and to use Core Animation to drive the progress ring.

Animatable Progress Ring

The drawing code for OTPProgressRing has been moved to a custom CALayer with an animatable progress property. The TokenListViewModel's ringProgress property has been replaced with a ProgressRingViewModel which specifies the start and end times of the current cycle of the animation. These values are used to configure a CABasicAnimation which drives the ring animation, removing the need for constant updates from a CADisplayLink. The progress ring now only needs to be updated with a new view model when a new cycle of the animation should begin.

View Model Expiration Date

The method which generates a view model for the token list (and for the root component) now returns a tuple containing both the view model and a nextRefreshTime. The AppController's display link has been replaced with a refresh timer, which is configured to fire at the nextRefreshTime and trigger an update of the view with a new view model.

The app also now triggers a view update on applicationWillEnterForeground, to ensure the UI is up-to-date when the app returns from the background.

Future Performance Considerations

These changes reduce idle CPU usage from around 40% to less than 10%. The remaining CPU load is almost entirely caused by animating and redrawing the progress ring. Ideas for optimizing the drawing of the progress ring are discussed below.

beaucollins and others added 22 commits October 30, 2017 11:31
CADisplayLink is being used to perform view model state changes at 60 fps presumably to keep
the progress ring animating at 60 fps. Nothing else in the view model needs to change that
frequently
View model now knows when the next refresh for tokens should happen. Allows the
TokenListViewController to animate the progress view from it's current progress without
relying on CADisplayLink
TokenListViewController schedules a dispatch to refresh tokens when the next period commences
This allows the progress to be animatable with CoreAnimation
When the app comes back from the background, the progress animation needs to be added again.
Instead of relying on the side effect of a no-op action to trigger a view model update, instead have the AppController update the view directly using the latest view model.
When using a TimeInterval since "now" to control the progress ring and trigger the view model refresh, the timing becomes susceptible to skew based on differences between the time when the interval is calculated and the time when the interval is applied to a timer or animation.
@beaucollins
Copy link
Collaborator

I was considering using two CAShapeLayer instances to show the rings.

My assumption is that it will cut the work in half since it only calculates one path per progress change instead of two:

  • ProgressLayer: child layers
    • CAShapeLayer - partial ring, changes with progress change
    • CAShapeLayer - full ring, only changes if the size of the view changes

@mattrubin
Copy link
Owner Author

@beaucollins That sounds like a good idea for optimizing the draw call. After commenting out the draw implementation, it does seem like some of the CPU load is caused just by the CAAnimation updating the progress property, so we may not be able to get this below 3-4%.

Thank you for all your work on this!

@mattrubin mattrubin changed the title Reduces Application CPU Usage Reduce CPU load by reducing view update frequency Nov 8, 2017
@beaucollins
Copy link
Collaborator

I gave the multiple layer idea a go and the CPU usage didn’t change noticeably.

Probably can’t squeeze much more out of it.

Changes look great.

@beaucollins
Copy link
Collaborator

beaucollins commented Nov 8, 2017

So it looks like using a couple of CAShapeLayers to draw the rings dropped the CPU to 1% on my simulator. But the CABasicTransition stopped updating the progress property value. It correctly triggered display updates for the animation frames but the progress property itself wouldn’t change.

@mattrubin mattrubin added this to the 2.0.2 milestone Nov 8, 2017
@codecov
Copy link

codecov bot commented Nov 8, 2017

Codecov Report

Merging #231 into develop will increase coverage by 0.46%.
The diff coverage is 78.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #231      +/-   ##
==========================================
+ Coverage    37.94%   38.4%   +0.46%     
==========================================
  Files           36      36              
  Lines         2148    2182      +34     
==========================================
+ Hits           815     838      +23     
- Misses        1333    1344      +11
Impacted Files Coverage Δ
Authenticator/Source/TokenListViewModel.swift 100% <ø> (ø) ⬆️
Authenticator/Source/OTPAppDelegate.swift 51.16% <0%> (-3.84%) ⬇️
Authenticator/Source/SearchField.swift 84.84% <100%> (ø) ⬆️
Authenticator/Source/TokenListViewController.swift 62.65% <100%> (+0.23%) ⬆️
Authenticator/Source/Root.swift 42.24% <100%> (+0.5%) ⬆️
Authenticator/Source/AppController.swift 22.58% <59.09%> (+2.26%) ⬆️
Authenticator/Source/TokenList.swift 79.62% <84%> (+1.41%) ⬆️
Authenticator/Source/OTPProgressRing.swift 61.53% <87.87%> (+43.01%) ⬆️
Authenticator/Source/RootViewController.swift 22.88% <0%> (-10.17%) ⬇️
Authenticator/Source/UITableView+Updates.swift 96.29% <0%> (-3.71%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28679c5...3b7f86a. Read the comment docs.

@mattrubin mattrubin merged commit 2421fa8 into develop Nov 8, 2017
@mattrubin mattrubin deleted the performance-refactor branch November 8, 2017 13:37
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.

None yet

2 participants