Remove CADisplayLink

[incanus]

See background in #2922 (comment). Makes sense that as the CADisplayLink always ticks, we'd be using resources to decide if a frame needs rendered or not, even when idle.

This change seems to cause a constant (but low) CPU usage

@kkaefer Did you measure this at all? How?

/cc @adam-mapbox @tmpsantos

[adam-mapbox]

So, I admit I haven't measured this, but the CPU usage should be extremely minimal. Remember that when the CADisplayLink wakes up all it has to do is check a boolean. It's not checking for whether we should render at that moment. And the CADisplayLink run loop mechanism is always running anyway; we're merely adding, at most, about one method call and maybe two or three instructions. It would be difficult to even measure the impact.

[incanus]

Yeah, that was my hunch. If @kkaefer can confirm that he wasn't seeing anything more egregious, I think we can just close here then.

[kkaefer]

It's minimal, but it still means the CPU needs to wake up. Previously, the CPU usage was reported as "zero" when the map was idling, now it's "low".

[adam-mapbox]

Can you put that into context in terms of how big of a deal that is? Like, does it drain the battery or something?

[tmpsantos]

@adam-mapbox on Intel architectures that I'm more familiar with, it definitely would have an impact because that would prevent the CPU from going to a "deep" C-State. But anyway, it is just a quirk if not measured.

[tomtaylor]

The way I've used a CADisplayLink in the past is to start it on some kind of user interaction, and then let it remove itself when all its work is complete. Maybe that's an option here?

[1ec5]

On my 64 GB iPhone 6 running iOS 9.1, iosapp flatlines at 1% CPU usage when idle (and ticks up to 2% a couple times a minute).

[incanus]

@tomtaylor There are so many different entry points for "interaction" here — gestures, but also programmatic/animated viewport changes, implicit changes for things like user dot tracking, etc.

If we can confirm that we had 0% battery usage when idle previously, and that we don't now, we should fix that.

[1ec5]

Prior to #2922, on my 64 GB iPhone 6 running iOS 9.1, iosapp was stable at 0% CPU usage and 2 fps. As of 7288473, it’s at 1–2% CPU usage and 3 fps. This is with iosapp in portrait showing the world map at z0 with the Streets style and no user dot.

[1ec5]

Either way, Xcode reports zero energy impact at idle.

[1ec5]

Scratch that, I’m not profiling battery usage correctly.

[incanus]

Building on @tomtaylor's thoughts, a naive approach that might work here is to observe app foreground/background state and suspend the CADisplayLink as a result. But in past we've had rendering issues such as return to foreground with black map and app crashing in background, and we might run into similar things with such an approach as well.

Either way, I think we can move this off of the 3.0.0 milestone since this is only for app foreground mode and continue to experiment given the upside of much better rendering performance.

[adam-mapbox]

One thing we could consider doing is to decrease the frequency of the tick
when we don't have activity. i.e., something like: "if last time we woke
up we didn't need to render, allow more time before checking again". The
only downside of this approach is that when something does happen, like the
user panning the map, there could be a noticeable lag. Sometimes even as
little as 1/10 of a second is very noticeable in that kind of setting.

It is theoretically possible to create a system that only ticks the
CADisplayLink when the app is in the foreground. But I hesitate to do
that; I second @incanus concerns about introducing possible
hard-to-reproduce rendering issues, especially when task switching.

On Tue, Nov 17, 2015 at 1:59 PM, Minh Nguyễn notifications@github.com
wrote:

Scratch that, I’m not profiling battery usage correctly.

—
Reply to this email directly or view it on GitHub
#2985 (comment)
.

[incanus]

I made a small but impactful change to this CADisplayLink setup yesterday in a0533d8. It should have the effect of making renders happen when doing things like scrolling external views that drive the map (which was getting paused; see #3069) but it might also affect main use case rendering. Long shot, but just providing context.

[adam-mapbox]

BTW, I tried implementing my suggestion above, and it didn't seem to break anything, but it also didn't have any noticeable impact on the reported CPU usage or energy impact, so I abandoned it.

[1ec5]

It is theoretically possible to create a system that only ticks the CADisplayLink when the app is in the foreground. But I hesitate to do that; I second @incanus concerns about introducing possible hard-to-reproduce rendering issues, especially when task switching.

I did this in #3448 by calling -[CADisplayLink pause]. In rudimentary testing, that worked just fine.

I’m tempted to have the CADisplayLink run only between the MapChangeRegionWillChangeAnimated and MapChangeRegionDidChangeAnimated notifications that mbgl sends to MGLMapView, but that would require a lot of faith that the notifications are always paired and coalesced correctly. I think that should be the focus of this ticket.

[1ec5]

#3448 is on the 3.1.0 milestone; the remaining work will have to wait for the next release.

[1ec5]

We should take CADisplayLink out of the picture entirely: MGLMapView should keep track of the last render time (as a lightweight NSTimeInterval); every time mbgl calls -setNeedsGLDisplay, only call -[GLKView display] if it’s been a 60th of a second since the last render. -setNeedsGLDisplay is already being called regardless of CADisplayLink, so CPU usage should return to levels we saw before #2922; meanwhile, redraws would still be throttled at 60fps during animations.

[incanus]

To summarize this issue to date:

We should remove CADisplayLink from the picture entirely. Rather than drive renders (-[GLKView display]) via a timer of sorts like this, we should maintain an internal NSTimeInterval since the last -setNeedsGLDisplay and then call the render ourselves if it's been more than 1/60s. That way, we will a) never render more than 60fps and b) should be able to allow the CPU to go to sleep per #2985 (comment).

Next steps:

• Measure with Instruments.app or otherwise quantify a metric here, e.g. confirm that we have nonzero CPU usage currently.
• Remove CADisplayLink and implement above system.
• Measure again and ensure that we have zero CPU usage or another improvement.
• Ensure that we don't have any regressions in behavior, especially during things like app background/foreground and black screen before render refresh.

[incanus]

Measure with Instruments.app or otherwise quantify a metric here, e.g. confirm that we have nonzero CPU usage currently.

This is confirmed easily enough using the Activity Monitor instrument and filtering the process list to just Mapbox GL. Even when the (animated) user dot isn't on screen, and the map isn't doing anything visibly, CPU usage is 0.5-1.0%.

[incanus]

Having ok luck with nice, smooth renders, but:

• Seem to have more CPU usage now—working on profiling to find out why.
• Annotation views (including the user dot) don't keep up with every frame anymore. 🤔

[incanus]

The second point above seems to be related to viewport animations. That is, if the map is moved synchronously with the finger, annotation views stick just fine. If it is pan-tossed, camera-animated, or anything that doesn't tie to direct manipulation, the frame render callback doesn't fire, which doesn't call -updateAnnotationViews. Once the animation finishes, the views sync back up.

Hunch is that CADisplayLink hides whatever causes this by rendering frames every screen refresh (which is exactly what it's meant to do). We may need to track some dirty state internally in order to know that viewport changes are still in progress.

[incanus]

Render callback & -updateAnnotationViews does fire, but views sometimes don't update position?!

[1ec5]

If it is pan-tossed, camera-animated, or anything that doesn't tie to direct manipulation, the frame render callback doesn't fire, which doesn't call -updateAnnotationViews.

I’ve been assuming that having mbgl drive updates was best, because mbgl has the most context about when a redraw is needed (camera transitions, property transitions, etc.). But mbgl doesn’t know anything about Core Animation. 🤔

[incanus]

The behavior here is reminiscent of hangs in animations due to scroll view tracking and display link run modes, FWIW. Will keep debugging this.

[incanus]

Ok, pushed my WIP to 2985-jrm-wip (commenting out CADisplayLink routines so that we can easily compare where it was used formerly).

As far as I can tell, the actual transplantation of removing CADisplayLink is fairly trivial, as per that diff.

The problems that crop up are related to the user and normal annotation views. Easy way to replicate:

• User: Have the user dot on screen. Contrast moving the map with your finger (works fine) with tossing the map to coast to a stop (dot freezes until pan is complete).

  The visual update routine seems to be called properly on each frame draw, so my hypothesis here is that the UIView animation block in -updateUserLocationAnnotationViewAnimatedWithDuration: is somehow hanging or not moving forward during these pans.

• Normal: Choose Add 100 Views from the gear menu. Observe that they track properly. Then, tap the tracking mode button and (assuming you are not also in DC) as the viewport changes, the annotations eventually freeze on screen despite being way out of view until the fly animation ends. Again, the update routine seems to be called each frame.

  Hypothesis: we don't actually ever remove annotation views from the hierarchy until the annotations are removed, we just skip updating their positions. This is catching up with us now for some reason. I'm not yet sure why they sync up properly at the end; possibly because they don't exist in offscreenAnnotations as they should until the final pass.

[incanus]

Part of me also wonders if the fact that CADisplayLink is part of Core Animation, if, as @1ec5 suggests in #2985 (comment), it causes some sort of Core Animation render buffer flush which keeps views/layers in sync properly. Something that we get for free with CADisplayLink that we might have to manage manually in future.

[incanus]

WELP

All of the bugs described in #2985 (comment) happen in an unaltered master, too 🤦‍♂️

So, I believe we can implement here and get rid of CADisplayLink!

Can anyone else confirm that these bugs exist or is it just me?

Edit: Scratch this; PEBKAC. 🤦‍♂️ 🤦‍♂️

[incanus]

At the very least, removing CADisplayLink aggravates the annotation view lag we've seen before because -updateAnnoationsViews is only called after a frame is rendered and not every screen refresh as with the display link. This makes it look like the views are lagging, when in fact they are crisply updating but our <60FPS map is not.

[incanus]

Here's a slow, deliberate GIF of the above jitter.

[1ec5]

Even if removing CADisplayLink doesn’t address the jittering, we know it contributes to increased power usage. Having it on constantly, especially while using turn-by-turn navigation, could be a real performance issue in itself.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.