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

Fix memory leak caused by "GeoJSONSource#setData" #2607

Closed
Pipoupi opened this issue May 23, 2016 · 23 comments
Closed

Fix memory leak caused by "GeoJSONSource#setData" #2607

Pipoupi opened this issue May 23, 2016 · 23 comments
Assignees
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@Pipoupi
Copy link

Pipoupi commented May 23, 2016

mapbox-gl-js version: 0.18.0

Steps to Trigger Behavior

  1. Go to https://afternoon-refuge-60269.herokuapp.com/
  2. Open a task manager (on Chrome : maj+esc) : App is ~ 430mb
  3. Hit refresh data button, App is ~630mb

Expected Behavior

Memory should stay around 430mb (I guess? At least that's what is done with a smaller geojson)

Actual Behavior

Memory increases every time map.getSource('earthquakes').setData(data); is called (same with removeSource and _updateSource), application crashes after 3-4 refresh, depends on the ram available.

@mourner mourner added bug 🐞 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage labels May 23, 2016
@nrathi
Copy link

nrathi commented Jun 9, 2016

This is happening to me, too.

@lucaswoj lucaswoj removed the bug 🐞 label Jul 29, 2016
@lucaswoj lucaswoj changed the title Leak memory with setData on huge geojson Fix memory leak caused by "GeoJSONSource#setData" Jul 29, 2016
@lucaswoj
Copy link
Contributor

lucaswoj commented Jul 29, 2016

Potential duplicate of #2266

@lucaswoj lucaswoj reopened this Jul 29, 2016
@wandergis
Copy link
Contributor

I also have this problem, and my chrome will crash when run setData() several times.

@Pipoupi
Copy link
Author

Pipoupi commented Sep 13, 2016

Hi, just an update

It seems that the memory leak thing is only happening on Google Chrome. Everything works fine on FF

@polokobe
Copy link

@Pipoupi HI, I doubt the reason which caused the memory leak is the difference between chrome and FF. Is it fine to explain the reason in detail? And, chrome has been used widely, may fix it recently?

@mourner mourner self-assigned this Sep 13, 2016
@mourner
Copy link
Member

mourner commented Sep 13, 2016

I just profiled memory on the test page provided by @Pipoupi — the worker that retains the data doesn't change its retained memory size after a "refresh"; same for the main thread:

image

Combined with the hint above, I'm concluding that this must be a Chrome bug, not an issue with Mapbox GL JS code.

Until it's fixed, I'd recommend avoiding setData on such huge amounts of data, and loading GeoJSON by URL whenever possible. Closing as not actionable.

@mourner mourner closed this as completed Sep 13, 2016
@wandergis
Copy link
Contributor

wandergis commented Sep 13, 2016

As it below( with Firefox)

Test case http://wandergis.com/memory-leak-test-case/

before

image

after setData four times

image

@mourner
Copy link
Member

mourner commented Sep 13, 2016

@wandergis can you confirm this with DevTools memory profiler?

@wandergis
Copy link
Contributor

wandergis commented Sep 13, 2016

Memory profiler is in right way(around 60MB). But acturely the Firefox's memory increase so much, i don't know why.
image

@mourner
Copy link
Member

mourner commented Sep 13, 2016

@wandergis most memory should be retained in one of the worker threads, is the screenshot for such a worker or for the main thread?

@wandergis
Copy link
Contributor

I'm sorry, i don't know which one is the screenshot for. I just open my FF's Devtools,and click the screenshot button.
image

@wandergis
Copy link
Contributor

wandergis commented Sep 13, 2016

The test case is http://wandergis.com/memory-leak-test-case/ ,you can profiled memory on the test page. source code here

@jasonpepper
Copy link
Contributor

This bug is a major problem for me also. If this is indeed a Chrome & Firefox bug, has a bug report been opened with Chrome or Firefox?

@jasonpepper
Copy link
Contributor

If the problem is that the workers do not release their memory in both Firefox and Chrome, would it be possible to terminate workers to guarantee that they release their resources?

@wandergis
Copy link
Contributor

https://www.mapbox.com/bites/00318/ This demo also have a memory leak problem.

@justinlewis
Copy link

justinlewis commented Feb 1, 2017

I'm looking for some clarification?

@mourner mentioned this above -

Until it's fixed, I'd recommend avoiding setData on such huge amounts of data, and loading GeoJSON by URL whenever possible. Closing as not actionable.

This appears to be an issue with my team as well using Mapbox-gl 0.29 when adding plain GeoJSON featureCollections to setData(). If we change our back-end to provide URL's for our data rather than GeoJSON can I expect this to avoid the memory leak issue? I can take this to StackExchange but I'm guessing no one knows about this issue more than the people on this ticket.

Thanks for the awesome work!

@mourner
Copy link
Member

mourner commented Feb 1, 2017

@justinlewis I'm not sure, can you try the latest version to make sure it's still an issue?

@justinlewis
Copy link

Thanks for getting back to me. I tried the 0.32.1 version with the same memory leak issue when using plain GeoJSON.

@jasonpepper
Copy link
Contributor

I moved most of my data to vector tiles, but it seemed like the remaining memory problems went away with 0.29.0. I guess if you're still having a leak, that was wishful thinking on my part. At the time I was dealing with this issue, loading GeoJson with a URL did not help.

@justinlewis
Copy link

Thanks @x9xjdzz9. Just to be clear,

  1. It sounds like by the time this problem was addressed you had moved on to a different approach and so didn't test the fix in detail?
  2. Loading GeoJson with URL did not fix or mitigate the original issue.

These are helpful data points.

@mourner - I want to determine if the issue we are having is truly the same issue as was addressed in this ticket or if this is a new issue. I would find it hard to believe that we are the only ones running into this issue at this point. GeoJSONSource.setData() seems like a common data integration point. That said, I need to know what you might need from me to make that determination. My dev team has analyzed memory usage and has seen the same increase in memory over time as described above. This happens even when loading only 2 polygons and then letting the page sit idle. After about 6 - 10 minutes of sitting idle the page tends to crash. Can you provide some guidance as to whether you think this is the same issue or something else? I would be happy to test for anything you might need to make that determination.

@jfirebaugh
Copy link
Contributor

This happens even when loading only 2 polygons and then letting the page sit idle. After about 6 - 10 minutes of sitting idle the page tends to crash.

Can you please open a fresh issue, and include a self-contained demo that shows this issue?

@justinlewis
Copy link

Ok. I'll try to reproduce this outside of our platform. Thanks for responding!

@jasonpepper
Copy link
Contributor

@justinlewis Yes, you understood me correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

No branches or pull requests

9 participants