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

Memory leak with V3.0.0 iOS SDK #3130

Closed
Dwar3xwar opened this issue Nov 25, 2015 · 33 comments · Fixed by #3448
Closed

Memory leak with V3.0.0 iOS SDK #3130

Dwar3xwar opened this issue Nov 25, 2015 · 33 comments · Fixed by #3448
Assignees
Labels
bug iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage
Milestone

Comments

@Dwar3xwar
Copy link

I've noticed a memory leak while using MGLMapView within a navigation controller.

To Recreate:

  1. Create 2 view controllers in a navigation controller, with VC A segueing to VC B, containing a MGLMapView.
  2. Go back to the original view controller, VC B is deinitialized but memory is not freed at all.

Instruments:

screen shot 2015-11-25 at 3 09 10 pm

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage labels Nov 25, 2015
@1ec5
Copy link
Contributor

1ec5 commented Nov 25, 2015

@adam-mapbox, SpriteAtlas is being leaked. Would #3049 address this issue?

@fezzik21
Copy link

I'm not sure. If anything, https://github.com/mapbox/mapbox-gl-native/compare/tmpsantos-1488_annotations_survive_style_change would perhaps fix it; but I think that only deals with leaking the SpriteAtlas that was created for that PR. So it wouldn't fix existing issues. It makes me think though that something very similar to what @tmpsantos fixed for #3049 is going on here.

@syedhali
Copy link

syedhali commented Dec 4, 2015

Hey guys, I'm also experiencing this (just updated from 2.1.2 to 3.0.0). Really love the release, but we've rolled back our app to 2.1.2 because the leak is too large for now.

@1ec5
Copy link
Contributor

1ec5 commented Dec 4, 2015

Any idea what could’ve caused this @jfirebaugh? If not, I can bisect later today.

@1ec5 1ec5 added this to the ios-v3.1.0 milestone Dec 4, 2015
@1ec5 1ec5 changed the title Memory Leak with V3.0.0 iOS SDK SpriteAtlas leak with V3.0.0 iOS SDK Dec 4, 2015
@jfirebaugh
Copy link
Contributor

Please do. I don't immediately see any issues in the code.

@jfirebaugh jfirebaugh changed the title SpriteAtlas leak with V3.0.0 iOS SDK Memory leak with V3.0.0 iOS SDK Dec 9, 2015
@jfirebaugh
Copy link
Contributor

I suspect that SpriteAtlas is a symptom of a larger, parent object being leaked.

@jfirebaugh jfirebaugh added the bug label Dec 14, 2015
@jfirebaugh
Copy link
Contributor

#3204 has a test app.

@sebyddd
Copy link

sebyddd commented Dec 18, 2015

Experiencing the same issue. Tried all obvious fixes I would think off. Deallocation, autorelease pool, singleton instance, but still nothing.

@vyshane
Copy link

vyshane commented Jan 5, 2016

Any news on this? This issue blocking our adoption of MapBox.

@1ec5
Copy link
Contributor

1ec5 commented Jan 5, 2016

Sorry for the silence on this issue. I've been trying to shepherd my PRs to completion lately. This is the next thing on my list.

@Dwar3xwar
Copy link
Author

Keep up the good work 👍

@1ec5
Copy link
Contributor

1ec5 commented Jan 6, 2016

This regression was introduced in #2922. Removing the CADisplayLink nearly eliminates the heap growth:

normalcy

1ec5 added a commit that referenced this issue Jan 6, 2016
CADisplayLink holds a strong reference to its target, forming a cycle that must be broken with -[CADisplayLink invalidate] when the animation is complete. I don’t yet have enough faith that will-change and did-change notifications are always coming from mbgl in pairs, so this change limits CADisplayLink to when MGLMapView is in the view hierarchy. It also pauses the CADisplayLink when the application is in the background. Finally, -[MGLMapView invalidate] has been renamed because that term tends not to mean “redraw” in Cocoa but is rather tied to timers.

Fixes #3130.
@1ec5
Copy link
Contributor

1ec5 commented Jan 6, 2016

I have a possible fix in #3448 and would appreciate any help verifying that redrawing continues to work in edge cases around changes in the view hierarchy or foregrounding/backgrounding.

1ec5 added a commit that referenced this issue Jan 7, 2016
CADisplayLink holds a strong reference to its target, forming a cycle that must be broken with -[CADisplayLink invalidate] when the animation is complete. I don’t yet have enough faith that will-change and did-change notifications are always coming from mbgl in pairs, so this change limits CADisplayLink to when MGLMapView is in the view hierarchy. It also pauses the CADisplayLink when the application is in the background. Finally, -[MGLMapView invalidate] has been renamed because that term tends not to mean “redraw” in Cocoa but is rather tied to timers.

Fixes #3130.
1ec5 added a commit that referenced this issue Jan 8, 2016
CADisplayLink holds a strong reference to its target, forming a cycle that must be broken with -[CADisplayLink invalidate] when the animation is complete. I don’t yet have enough faith that will-change and did-change notifications are always coming from mbgl in pairs, so this change limits CADisplayLink to when MGLMapView is in the view hierarchy. It also pauses the CADisplayLink when the view is hidden or the application is in the background. Finally, -[MGLMapView invalidate] has been renamed because that term tends not to mean “redraw” in Cocoa but is rather tied to timers.

Fixes #3130.

[ios] Also invalidate CADisplayLink on removal from window

[ios] Also shut down CADisplayLink when view is hidden
@1ec5 1ec5 closed this as completed in #3448 Jan 8, 2016
@1ec5 1ec5 removed the in progress label Jan 8, 2016
@syedhali
Copy link

Hey @1ec5 , any chance a new iOS cocoapods release is coming by the end of Jan? We've adopted 3.0.0, but we're constantly running into memory related crashes and it looks like your fix has already been integrated into master.

@1ec5
Copy link
Contributor

1ec5 commented Jan 20, 2016

@syedhali, we’re optimistically shooting for the end of next week, but as you can see from the milestone, we’ve got several issues that need to be fixed or merged first. We’ll try to get a prerelease out in the meantime. Thanks for your patience!

@ultranano
Copy link

HI @1ec5!
How much time for 3.1 on iOS?
Best regards and good work to all of us Mapbox Team

@1ec5
Copy link
Contributor

1ec5 commented Jan 25, 2016

@ultranano, almost there! In the meantime, you can try out a prerelease. We'll publish another prerelease with additional improvements today or tomorrow.

@syedhali
Copy link

@1ec5 Great work on the pre-release! Memory leak seems to be fixed :) 🎉

@ultranano
Copy link

Great @1ec5!
How I can update the prerelase with cocoapods?

@1ec5
Copy link
Contributor

1ec5 commented Jan 25, 2016

To install this weekend's prerelease via CocoaPods, point your Podfile to this Podspec. Note that, if you use Swift, you may run into #3300, which will be fixed in the next prerelease.

@chriszhang4213
Copy link

is that possible we can get a static binary release for 3.1 soon? looks like 3.01 has a huge memory leak whenever i use the map in my app, and it is not convince for us to use the CocoaPods since we don't want to make a lot of change for some other old sdk in CocoaPods,

@friedbunny
Copy link
Contributor

Have a look at the releases page for links to (pre-)release binaries.

@chriszhang4213
Copy link

@friedbunny I am now using the mapbox-ios-sdk-3.0.1-no-bitcode.zip version now, i did download the pre-release version trying to use that in our test project. just wonder if you guys can have a same kind of 3.1 no-bitcode version release soon

@1ec5
Copy link
Contributor

1ec5 commented Jan 26, 2016

@chriszhang4213, you can obtain a Bitcode-free binary by building from source (make ipackage BITCODE=NO). I’m curious what’s preventing you from adopting Bitcode in your project. Is it a particular third-party framework, perhaps, or iOS 7 support?

@chriszhang4213
Copy link

@1ec5 @friedbunny . ok, we did use some third-party framework. I am new in this part. when I download the 3.01 I follow the guide https://www.mapbox.com/ios-sdk/#binary which is working fine on my project. then I feel like you guys didn't put new release on that download link, so i went to the Pre-release 3.1 and download the dynamic framework instead of a static library. follow the read me.txt guide ...trying to make this work in my testing project.

then it shows #import "Mapbox.h" file not found...


never mind, i just figure out i should use #import < Mapbox/Mapbox.h > instead of "Mapbox.h", it works now..

@1ec5
Copy link
Contributor

1ec5 commented Jan 27, 2016

@chriszhang4213, please note that 3.1.0-pre.1 is a prerelease – we’re still readying it for a final release that’ll come out soon. We’ll update the guide at that time.

@nzhuk
Copy link

nzhuk commented Feb 1, 2016

Thanks for fixing the issue. Current pre-release version seems to be ios-v3.1.0-pre.3. Do you have some estimate of when we could expect 3.1.0 final release?

@ultranano
Copy link

Hi All! I see that for milestone 3.1 there is only one issue on going.
Do you have an estimate date for final release 3.1?

@1ec5
Copy link
Contributor

1ec5 commented Feb 8, 2016

Version 3.1.0 is out. CocoaPods has been updated, and the online installation instructions have been updated for anyone installing manually. Thanks for your patience everyone!

@jasonlosito
Copy link

I am using the latest Mapbox iOS SDK and I am still experiencing memory leaks. I have a table where you can select a location and when you tap on item, it will go to a map to display the location. When I go back and forth, the memory keeps adding up and my app crashes.

@1ec5
Copy link
Contributor

1ec5 commented Nov 14, 2016

@jasonlosito, it's likely that the memory leak you're experiencing is distinct from the issue that was reported here and long ago fixed. Please open a new issue, and be sure to link to an instruments trace and/or sample code that reproduces the problem, so we can be sure we're looking at the same thing as you are. Thank you!

@karlobutorac
Copy link

karlobutorac commented Feb 8, 2017

I'm having this exact same problem as Dwar3xwar:

  1. Create 2 view controllers in a navigation controller, with VC A segueing to VC B, containing a MGLMapView.
    2.Go back to the original view controller, VC B is deinitialized but memory is not freed at all.
    // The used memory gets around 50-70 mb bigger every time I switch between viewControllers
    I'm on the newest version

@1ec5
Copy link
Contributor

1ec5 commented Feb 8, 2017

Tracking in #7991.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging a pull request may close this issue.