Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

GPU/CPU usage problem - updateFromDisplayLink deadlock #15342

Closed
JustinGanzer opened this issue Aug 8, 2019 · 40 comments
Closed

GPU/CPU usage problem - updateFromDisplayLink deadlock #15342

JustinGanzer opened this issue Aug 8, 2019 · 40 comments
Assignees
Labels
bug Core The cross-platform C++ core, aka mbgl high priority iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage

Comments

@JustinGanzer
Copy link

Hello Mapbox community and team!

I'll be providing sample code of a ViewController with a MapView that when zoomed in to the lowest tiles and back up again somehow initiates a very odd behaviour starting with the Mapbox-iOS-SDK 5.1.0. Lower versions will not result in this error, higher versions such as 5.2.0 still do.
After zooming in and back out the gpu and cpu are constantly at roughly 60% usage and will never cease to go below this even though the I'm not interacting with the device in any way.

Due to the update being called over over, all MapAnnotations are constantly updated as well, which makes it impossible to use drag and drop as they flicker between the new and old position, which in comparison is only a minor inconvenience.

This error does not appear on an empty MapView. It only appears after adding a source and layer that I'd normally use to show icons on the map. I'll follow up this thread once I figure out what exactly is causing this issue. Do note however, that the very same code runs into no issues with any SDK version below 5.1.0 Hopefully someone can figure out what changed. As always, I'm happy to help try out whatever I can to help nail down the reason for error.

Steps to reproduce

  1. Present the controller provided or any other with a MapView that includes a symbol style/layer
  2. Zoom all the way in, Zoom all the way out
  3. Go over to the performance logging and watch the never ending CPU/GPU usage

Expected behavior

MapView will stop rendering things once no more updates are made, such as movement.

Actual behavior

MapView is called/calls updateFromDisplayLink over and over.

Configuration

Mapbox SDK versions:
5.1.0+ and up
iOS/macOS versions:
Tested only on 12.3 and up, sorry i didn't have older versions available.
Device/simulator models:
iPhone 5S simulated with 12.3.1

Xcode version:
Version 10.2.1 (10E1001)

Here the inverted call stack:

Screenshot 2019-08-07 at 16 18 27

Result in SDK versions 5.1.0 and up:

Screenshot 2019-08-07 at 16 15 58

Result in SDK versions 5.0.0 and lower:

Screenshot 2019-08-08 at 11 59 21

The ViewController which lets this error be reproduced every single time once zoomed in all the way and back out:

import Mapbox

class TestMapController: UIViewController, MGLMapViewDelegate {
    
    var mapView: MGLMapView!
    
    override func viewDidLoad() {
        super.viewDidLoad()
        
        mapView = MGLMapView(frame: view.bounds, styleURL: MGLStyle.lightStyleURL)
        mapView.autoresizingMask = [.flexibleWidth, .flexibleHeight]
        mapView.tintColor = .darkGray
        mapView.delegate = self
        view.addSubview(mapView)
        
    }
    
    func mapView(_ mapView: MGLMapView, didFinishLoading style: MGLStyle) {
        let source = MGLShapeSource(identifier: "pictures", features: [], options: nil)
        let blueBoxImage = TestMapController.singleColoredImage(of: .blue, and: CGSize(width: 50, height: 50))
        style.setImage(blueBoxImage, forName: "picture")
        let layer = TestMapController.getLayer(for: source)
        style.addSource(source)
        style.addLayer(layer)
    }
    
    static func singleColoredImage(of color: UIColor, and size: CGSize) -> UIImage {
        let format = UIGraphicsImageRendererFormat()
        return UIGraphicsImageRenderer(size: size, format: format).image { (context) in
            color.setFill()
            context.fill(CGRect(origin: .zero, size: size))
        }
    }
    
    static func getLayer(for source: MGLShapeSource) -> MGLStyleLayer {
        let layer = MGLSymbolStyleLayer(identifier: "pictures", source: source)
        layer.iconAllowsOverlap = NSExpression(forConstantValue: true)
        layer.minimumZoomLevel = 6.0
        let nameOfPictures = "picture"
        let matching = "'\(nameOfPictures)', '\(nameOfPictures)'"
        let formatImageName = "MGL_MATCH(highlight, \(matching), '\(nameOfPictures)')"
        let functionImage = NSExpression(format: formatImageName)
        layer.iconImageName = functionImage
        layer.keepsIconUpright = NSExpression(forConstantValue: 1)
        let formatScale = "mgl_interpolate:withCurveType:parameters:stops:($zoomLevel, 'exponential', 1, %@)"
        layer.iconScale = NSExpression(format: formatScale, [6: 0.3, 11: 0.6, 14: 0.88])
        return layer
    }
}

@LukasPaczos LukasPaczos added the iOS Mapbox Maps SDK for iOS label Aug 8, 2019
@chloekraw chloekraw added the performance Speed, stability, CPU usage, memory usage, or power usage label Aug 21, 2019
@RomainQuidet
Copy link
Contributor

RomainQuidet commented Sep 10, 2019

Hi, we face the same bug and this is truly a huge issue. We can't upgrade to SDK 5.x+ anymore because of this bug and insets bug (already opened somewhere else).
Here is a test I ran with Instruments:
SDK: Mapbox 5.3.0
Device: iphone X 12.4.1

Capture d’écran 2019-09-10 à 15 23 49

This issue is not present on SDK 5.0. It appears on 5.1 and still there in 5.3.0

@julianrex julianrex added Core The cross-platform C++ core, aka mbgl high priority labels Sep 10, 2019
@julianrex
Copy link
Contributor

/cc @tmpsantos @alexshalamov

@julianrex
Copy link
Contributor

Thanks for the report @JustinGanzer.

@julianrex julianrex added the bug label Sep 10, 2019
@RomainQuidet
Copy link
Contributor

You're welcome. I deactivate user tracking so nothing should force the map to render once done. Tested on your sample app from the framework sources, seems that there is an infinite loop somewhere.
Once rendered, the framework ask again for a rendering loop... (see the renderTreeParameters where needsRepaint is true just after a rendering).

Capture d’écran 2019-09-10 à 16 25 02

Capture d’écran 2019-09-10 à 16 24 47

@RomainQuidet
Copy link
Contributor

Seems that RenderOrchestrator::hasTransitions(TimePoint timePoint) always returns true with hasFadingTiles()from resources

@julianrex
Copy link
Contributor

Thanks for the detailed investigation - what lat/lons are you zooming in/out on?

@RomainQuidet
Copy link
Contributor

RomainQuidet commented Sep 10, 2019

South West of Paris, France
48.8214267 / 2.1670292
Do you mean it's an issue with the tiles content ? We are using our custom tiles.

@julianrex
Copy link
Contributor

Are you able to try with a default Mapbox style?

@RomainQuidet
Copy link
Contributor

RomainQuidet commented Sep 10, 2019

Doing it right now.
Done. With default Streets tiles, the CPU stay around 1% when not moving the map, even when user location is active. Sooooo it seems that the bug in the SDK is triggered by our tiles:
RenderOrchestrator::hasTransitions returns true on renderSources hasFadingTiles() from a hidden layer.
Same for @JustinGanzer : what tile / source do you use?

@pozdnyakov pozdnyakov self-assigned this Sep 10, 2019
@julianrex
Copy link
Contributor

Thanks @RomainQuidet, @pozdnyakov is now going to investigate.

@RomainQuidet
Copy link
Contributor

RomainQuidet commented Sep 10, 2019

I confirm that tile.holdForFade(); returns always true in the TilePyramid class, which is the source of the issue. We have a hidden layer displaying nothing, maybe this kind of tiles should not return true here.

@pozdnyakov
Copy link
Contributor

@RomainQuidet Thank you very much for your report and the investigation! I am looking at the issue. Meanwhile, would it be possible for you to try out a preliminary fix at #15600 and tell if it helps? Thanks!

@RomainQuidet
Copy link
Contributor

Of course I'll patch my version and see if the issue is still visible. Tomorrow for me it's the end of the day in France ;)

@JustinGanzer
Copy link
Author

JustinGanzer commented Sep 11, 2019

@RomainQuidet @julianrex , In the original post I provided a small ViewController with nothing but a MapView and a SymbolStyleLayer (which seems to be the cause for the reaction) inside. No matter what tiles or what style is used, the issue will arise once zoomed in and out again.
Without a custom layer this issue does not occur.

Thanks for the detailed investigation @RomainQuidet. It's a huge issue for us as well and hinders us from updating to 5.x.

@RomainQuidet
Copy link
Contributor

Hi @pozdnyakov, your patch #15600 seems to work on my tests. I don't see anymore the dead loop consuming CPU.

@pozdnyakov
Copy link
Contributor

@RomainQuidet Thanks! @JustinGanzer does #15600 solve the problem for you too?

@RomainQuidet
Copy link
Contributor

RomainQuidet commented Sep 11, 2019

bad news, I can't reproduce the issue on my demo app but on our production app, the renderer is still stuck in a loop using 100% CPU... And there is a memory issue associated, it's creating data until the app is killed by the system. I'm investigating.

Capture d’écran 2019-09-11 à 14 41 10

Capture d’écran 2019-09-11 à 14 41 30

@pozdnyakov This time the RenderOrchestrator::hasTransitions always returns true by placement->hasTransitions(timePoint)

@pozdnyakov
Copy link
Contributor

@pozdnyakov This time the RenderOrchestrator::hasTransitions always returns true by placement->hasTransitions(timePoint)

That's a different issue, investigating..

@pozdnyakov
Copy link
Contributor

@RomainQuidet could you give some more details on how to reproduce #15342 (comment) ?
What is the value of Placement::stale when the app is stuck?

@RomainQuidet
Copy link
Contributor

I wish I could help you more on this one, but I can't reproduce it on my demo app. I'm trying.
According to my logs, stale is false.

@pozdnyakov
Copy link
Contributor

My speculative guess is that placementChanged for some reason is always set to true in Placement::commit() and it is causing the problem. I could recommend the following:

  1. comment out the code block under
    // add the opacities from the current placement, and copy their current values from the previous placement
    and see if it helps.

  2. comment out the code block under
    // copy and update values from the previous placement that aren't in the current placement but haven't finished fading
    and see if it helps.

  3. try out the patch from [core] Fix placement for updated buckets #15308 (as its removes implicit opacities modifications) and see if it helps.

@RomainQuidet
Copy link
Contributor

Hi, Thanks for investigating this strange issue.
I'll try that tomorrow.

@RomainQuidet
Copy link
Contributor

Hi, neither 1st nor 2nd suggestions work. I can't apply your #15308 patch on my 5.3.0 branch there is too many conficts from the master branch.
I don't know how to help you more.

@astojilj
Copy link
Contributor

@RomainQuidet

Hi, we face the same bug and this is truly a huge issue. We can't upgrade to SDK 5.x+ anymore because of this bug and insets bug (already opened somewhere else).

What is the inset bug blocking the upgrade?

@RomainQuidet
Copy link
Contributor

Hi, since release 5.1, insets are not working correctly in our app, see #15574 #15233 #15198

@astojilj
Copy link
Contributor

Hi, since release 5.1, insets are not working correctly in our app, see #15574 #15233 #15198

Could you please submit an issue with problem description: which API is used and, if possible a video presenting the issue?
It is important to understand if it is related to #15574. In a way, #15233 is not describing issue - it is an analysis assumption about the cause.
As it could be related to multiple issues, e.g. 4026451 I would suggest to start analysis from issue description in order to get it fixed. Thanks.

@1ec5
Copy link
Contributor

1ec5 commented Sep 16, 2019

It is important to understand if it is related to #15574. In a way, #15233 is not describing issue - it is an analysis assumption about the cause.

My intention in filing #15233 was to report the bug that the edgePadding parameter is persisted. That’s an issue in and of itself, not just a supposed cause of another issue. (A setter’s auxiliary parameter should never have a lasting side effect. We can be sure that the edge padding in -setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler: is auxiliary because “with” appears before it in the method signature.)

@pozdnyakov
Copy link
Contributor

Hi, neither 1st nor 2nd suggestions work. I can't apply your #15308 patch on my 5.3.0 branch there is too many conficts from the master branch.
I don't know how to help you more.

@RomainQuidet Hi, do you have any updates on the issue? #15308 has landed, is it feasible for you to try out current master branch?

@RomainQuidet
Copy link
Contributor

RomainQuidet commented Sep 19, 2019

Hi, I was able to take some time to merge the master on our branch, unfortunately the infinite loop is still there, using 110+% of CPU.
I'll try to make some more logs in to help out to find what's going on.

@pozdnyakov after further investigations, I was not able to reproduce the issue on a sample but our production app.
In this app, on a navigation mapView, we quickly set the user tracking (done by setting a camera and updating the mapView) and update the mapView insets. Combination of both lead to a dead loop.
If I remove either user tracking or insets update, the mapView behave correctly.
So branch iOS 5.1+ introduced a strange bug here.

Capture d’écran 2019-09-19 à 16 43 06

@venkatchm
Copy link

venkatchm commented Oct 21, 2019

Hi,
We are also experiencing the same issue in our app.
Here is the screenshot from Xcode Energy Organiser.
Mapbox version 5.2.0.

67231930-e86b6900-f45d-11e9-8d32-603d692e80fd

@chloekraw
Copy link
Contributor

@venkatchm - thanks for the report. do you have the same implementation as others in this thread?

if not: in 5.4.0, we published a PR (#15308) as well as other performance improvements (e.g. #15600) that are intended to alleviate this problem. would you mind testing out 5.4.0 and seeing if you can reproduce the same problem?

@RomainQuidet - thanks for the information, sorry to hear that your issue isn't fully resolved, we'll keep digging and we appreciate the help.

@pozdnyakov
Copy link
Contributor

@RomainQuidet it's hard to guess the exact reason just from the screen shot, however #15086 could be a speculative fix, in case the problem is caused with infinite loop of the map update inside a transition handler. Could you pls try it out?

@RomainQuidet
Copy link
Contributor

RomainQuidet commented Oct 23, 2019 via email

@astojilj
Copy link
Contributor

astojilj commented Oct 23, 2019

@RomainQuidet

In this app, on a navigation mapView, we quickly set the user tracking (done by setting a camera and updating the mapView) and update the mapView insets.

Could you please share the code for this - values used for insets and tracking before and after. Are both called from callbacks? How frequent the change of values happens?

on a navigation mapView

Do you use navigation SDK here?

Combination of both lead to a dead loop.
If I remove either user tracking or insets update, the mapView behave correctly.

Do you still see updateFromDisplayLink called all the time?

Thanks.

@astojilj
Copy link
Contributor

@venkatchm,

In this app, on a navigation mapView, we quickly set the user tracking (done by setting a camera and updating the mapView) and update the mapView insets.

Is your code also doing this?

@venkatchm
Copy link

@astojilj We are not using navigation mapView. We render custom layers (MGLLineStyleLayer, MGLSymbolStyleLayer ) on the Map and we set the setVisibleCoordinateBounds based on predefined coordinate.

@chloekraw We see this issue in the 5.4.0 SDK. Here are the screenshots of Xcode Energy Organiser
Screenshot 2019-11-04 at 20 15 09
Screenshot 2019-11-04 at 20 15 34

@astojilj
Copy link
Contributor

astojilj commented Nov 4, 2019

@astojilj We are not using navigation mapView. We render custom layers (MGLLineStyleLayer, MGLSymbolStyleLayer ) on the Map and we set the setVisibleCoordinateBounds based on predefined coordinate.

@chloekraw We see this issue in the 5.4.0 SDK. Here are the screenshots of Xcode Energy Organiser

@venkatchm I am especially interested if there is tilt/pitch set, if edgePadding is supplied to setVisibleCoordinateBounds and if large top contentInset is used. Thanks.

@astojilj
Copy link
Contributor

astojilj commented Nov 8, 2019

@JustinGanzer,
Thank you for the reproduction code. Using this code, it was easy to reproduce it in 5.1. As I am not able to reproduce it with the latest master branch, let's close the issue and identify any further performance problems as separate issues here.

To verify it, Swift code from original report is ported to ObjC and used in iosapp (it was convenient way for me to reproduce and debug it):

diff --git a/platform/ios/app/MBXViewController.m b/platform/ios/app/MBXViewController.m
index 82a68e074..e144d4ccc 100644
--- a/platform/ios/app/MBXViewController.m
+++ b/platform/ios/app/MBXViewController.m
@@ -1553,6 +1553,26 @@ - (void)addLatLonGrid
                                                                                source:source];
     labelLayer.text = [NSExpression expressionForKeyPath:@"value"];
     [self.mapView.style addLayer:labelLayer];
+
+    source = [[MGLComputedShapeSource alloc] initWithIdentifier:@"pictures" options:nil];
+    UIImage *blueBoxImage = [self imageWithText:@"XY" backgroundColor:[UIColor blueColor]];
+    [self.mapView.style setImage:blueBoxImage forName: @"picture"];
+
+    MGLSymbolStyleLayer *layer = [[MGLSymbolStyleLayer alloc] initWithIdentifier:@"pictures" source:source];
+    layer.iconAllowsOverlap = [NSExpression expressionForConstantValue:@YES];
+    layer.minimumZoomLevel = 6.0;
+    NSString *nameOfPictures = @"picture";
+    NSString *matching = [NSString stringWithFormat:@"'%@', '%@'", nameOfPictures, nameOfPictures];
+    NSString *formatImageName = [NSString stringWithFormat:@"MGL_MATCH(highlight, %@, '%@')", matching, nameOfPictures];
+    NSExpression *functionImage = [NSExpression expressionWithFormat:formatImageName];
+    layer.iconImageName = functionImage;
+    layer.keepsIconUpright = [NSExpression expressionForConstantValue:@YES];
+
+    NSDictionary *scaleStops = @{@6: @0.3, @11: @0.6, @14: @0.88};
+    layer.iconScale = [NSExpression expressionWithFormat: @"mgl_interpolate:withCurveType:parameters:stops:($zoomLevel, 'exponential', 1, %@)", scaleStops];
+
+    [self.mapView.style addSource:source];
+    [self.mapView.style addLayer:layer];
 }
 
 - (NSString *)bestLanguageForUser

When using 5.1 release branch code, the issue is reproducible - with no interaction with the map, GPU consumption is high all the time:
Screen Shot 2019-11-08 at 12 34 41

When using the latest master, this looks resolved.

@RomainQuidet, @venkatchm,

There is a chance that the issue you are facing is not related to the issue here: it is about constant GPU re-rendering and high GPU energy consumption (green boxes at the diagram), even there is no user interaction.

Once you stop interaction with the map, does it still has high CPU/GPU usage like it was in initial report?

if not, we would likely need to address the other issues as a new one and provide further information needed to reproduce them. The fact that you see the same code in trace is just about the fact that it is the hottest code path and it is expected - we need to figure out why it is triggered more than expected.

Could you please provide more details about the style and layers your map has? If you could share this and some code, it would help us focusing on the problem. Thanks.

@astojilj astojilj closed this as completed Nov 8, 2019
@JustinGanzer
Copy link
Author

@astojilj Funny coincidence but I had just tested the newest release on various devices from ios 12.0 all the way to 13.2 with xcode 11 and 11.2. The problem appears resolved as I can not reproduce the issue in the test controller provided in my original issue message, neither in our production app. Great job!

I have however noticed a much much higher cpu usage now than before when applying a custom contentInset to the MGLMapView and animating the camera. I only mention this since you have expressed interest about it to venkatchm.

I'll open a new issue once I've managed to create a reproduce-able instance of it.

@astojilj
Copy link
Contributor

astojilj commented Nov 8, 2019

@JustinGanzer,
Thanks for help. Related to contentInset, I needed to verify it is not related to this: #15163. It is not.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl high priority iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

No branches or pull requests

9 participants